| Message ID | 20230322160926.948687-6-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add WLED support to TPS68470 LED driver | expand |
Hi, On 3/22/23 17:09, Daniel Scally wrote: > We want to extend tps68470_brightness_get() to be usable with the > other LEDs supplied by the IC; refactor it to make that easier. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > drivers/leds/leds-tps68470.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c > index d2060fe4259d..44df175d25de 100644 > --- a/drivers/leds/leds-tps68470.c > +++ b/drivers/leds/leds-tps68470.c > @@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev > int ret = 0; > int value = 0; > > - ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); > - if (ret) > - return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n"); > - > switch (led->led_id) { > case TPS68470_ILED_A: > - value = value & TPS68470_ILEDCTL_ENA; > - break; > case TPS68470_ILED_B: > - value = value & TPS68470_ILEDCTL_ENB; > + ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); > + if (ret) > + return dev_err_probe(led_cdev->dev, ret, > + "failed to read LED status\n"); I realize this is a pre-existing problem, but I don't think we should be using dev_err_probe() in functions which are used outside the probe() path? So maybe fix this up while at it and make this: if (ret) { dev_err(led_cdev->dev, ""failed to read LED status: %d\n", ret); return ret; } > + > + value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA : > + TPS68470_ILEDCTL_ENB; > break; > + default: > + return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n"); idem. With those fixed: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > } > > return value ? LED_ON : LED_OFF; > } > > - > static int tps68470_ledb_current_init(struct platform_device *pdev, > struct tps68470_device *tps68470) > {
Morning Hans On 22/03/2023 17:22, Hans de Goede wrote: > Hi, > > On 3/22/23 17:09, Daniel Scally wrote: >> We want to extend tps68470_brightness_get() to be usable with the >> other LEDs supplied by the IC; refactor it to make that easier. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> drivers/leds/leds-tps68470.c | 17 +++++++++-------- >> 1 file changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c >> index d2060fe4259d..44df175d25de 100644 >> --- a/drivers/leds/leds-tps68470.c >> +++ b/drivers/leds/leds-tps68470.c >> @@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev >> int ret = 0; >> int value = 0; >> >> - ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); >> - if (ret) >> - return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n"); >> - >> switch (led->led_id) { >> case TPS68470_ILED_A: >> - value = value & TPS68470_ILEDCTL_ENA; >> - break; >> case TPS68470_ILED_B: >> - value = value & TPS68470_ILEDCTL_ENB; >> + ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); >> + if (ret) >> + return dev_err_probe(led_cdev->dev, ret, >> + "failed to read LED status\n"); > I realize this is a pre-existing problem, but I don't think we should > be using dev_err_probe() in functions which are used outside the probe() > path? I had thought that this was being encouraged because of the standard formatting, but actually now I re-read the comment's function it's just "OK to use in .probe() even if it can't return -EPROBE_DEFER". My bad; I'll fix it. > > So maybe fix this up while at it and make this: > > if (ret) { > dev_err(led_cdev->dev, ""failed to read LED status: %d\n", ret); > return ret; > } > >> + >> + value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA : >> + TPS68470_ILEDCTL_ENB; >> break; >> + default: >> + return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n"); > idem. idem? Sorry, I'm not following here. > > With those fixed: > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > > Regards, > > Hans > > > >> } >> >> return value ? LED_ON : LED_OFF; >> } >> >> - >> static int tps68470_ledb_current_init(struct platform_device *pdev, >> struct tps68470_device *tps68470) >> {
Hi, On 3/23/23 08:43, Dan Scally wrote: > Morning Hans > > On 22/03/2023 17:22, Hans de Goede wrote: >> Hi, >> >> On 3/22/23 17:09, Daniel Scally wrote: >>> We want to extend tps68470_brightness_get() to be usable with the >>> other LEDs supplied by the IC; refactor it to make that easier. >>> >>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>> --- >>> drivers/leds/leds-tps68470.c | 17 +++++++++-------- >>> 1 file changed, 9 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c >>> index d2060fe4259d..44df175d25de 100644 >>> --- a/drivers/leds/leds-tps68470.c >>> +++ b/drivers/leds/leds-tps68470.c >>> @@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev >>> int ret = 0; >>> int value = 0; >>> - ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); >>> - if (ret) >>> - return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n"); >>> - >>> switch (led->led_id) { >>> case TPS68470_ILED_A: >>> - value = value & TPS68470_ILEDCTL_ENA; >>> - break; >>> case TPS68470_ILED_B: >>> - value = value & TPS68470_ILEDCTL_ENB; >>> + ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); >>> + if (ret) >>> + return dev_err_probe(led_cdev->dev, ret, >>> + "failed to read LED status\n"); >> I realize this is a pre-existing problem, but I don't think we should >> be using dev_err_probe() in functions which are used outside the probe() >> path? > > > I had thought that this was being encouraged because of the standard formatting, but actually now I re-read the comment's function it's just "OK to use in .probe() even if it can't return -EPROBE_DEFER". My bad; I'll fix it. > >> >> So maybe fix this up while at it and make this: >> >> if (ret) { >> dev_err(led_cdev->dev, ""failed to read LED status: %d\n", ret); >> return ret; >> } >> >>> + >>> + value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA : >>> + TPS68470_ILEDCTL_ENB; >>> break; >>> + default: >>> + return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n"); >> idem. > > > idem? Sorry, I'm not following here. My bad I though this was something which most people understand / know: https://en.wikipedia.org/wiki/Idem So what I was trying to say is: "the same (don't use dev_err_probe()) applies here". Regards, Hans > >> >> With those fixed: >> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> >> Regards, >> >> Hans >> >> >> >>> } >>> return value ? LED_ON : LED_OFF; >>> } >>> - >>> static int tps68470_ledb_current_init(struct platform_device *pdev, >>> struct tps68470_device *tps68470) >>> { >
On 23/03/2023 09:52, Hans de Goede wrote: > Hi, > > On 3/23/23 08:43, Dan Scally wrote: >> Morning Hans >> >> On 22/03/2023 17:22, Hans de Goede wrote: >>> Hi, >>> >>> On 3/22/23 17:09, Daniel Scally wrote: >>>> We want to extend tps68470_brightness_get() to be usable with the >>>> other LEDs supplied by the IC; refactor it to make that easier. >>>> >>>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>>> --- >>>> drivers/leds/leds-tps68470.c | 17 +++++++++-------- >>>> 1 file changed, 9 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c >>>> index d2060fe4259d..44df175d25de 100644 >>>> --- a/drivers/leds/leds-tps68470.c >>>> +++ b/drivers/leds/leds-tps68470.c >>>> @@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev >>>> int ret = 0; >>>> int value = 0; >>>> - ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); >>>> - if (ret) >>>> - return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n"); >>>> - >>>> switch (led->led_id) { >>>> case TPS68470_ILED_A: >>>> - value = value & TPS68470_ILEDCTL_ENA; >>>> - break; >>>> case TPS68470_ILED_B: >>>> - value = value & TPS68470_ILEDCTL_ENB; >>>> + ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); >>>> + if (ret) >>>> + return dev_err_probe(led_cdev->dev, ret, >>>> + "failed to read LED status\n"); >>> I realize this is a pre-existing problem, but I don't think we should >>> be using dev_err_probe() in functions which are used outside the probe() >>> path? >> >> I had thought that this was being encouraged because of the standard formatting, but actually now I re-read the comment's function it's just "OK to use in .probe() even if it can't return -EPROBE_DEFER". My bad; I'll fix it. >> >>> So maybe fix this up while at it and make this: >>> >>> if (ret) { >>> dev_err(led_cdev->dev, ""failed to read LED status: %d\n", ret); >>> return ret; >>> } >>> >>>> + >>>> + value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA : >>>> + TPS68470_ILEDCTL_ENB; >>>> break; >>>> + default: >>>> + return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n"); >>> idem. >> >> idem? Sorry, I'm not following here. > My bad I though this was something which most people understand / know: > > https://en.wikipedia.org/wiki/Idem > > So what I was trying to say is: > > "the same (don't use dev_err_probe()) applies here". Ah-ha! Thanks, hadn't heard that one before. > > Regards, > > Hans > > > > > >>> With those fixed: >>> >>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >>> >>> Regards, >>> >>> Hans >>> >>> >>> >>>> } >>>> return value ? LED_ON : LED_OFF; >>>> } >>>> - >>>> static int tps68470_ledb_current_init(struct platform_device *pdev, >>>> struct tps68470_device *tps68470) >>>> {
diff --git a/drivers/leds/leds-tps68470.c b/drivers/leds/leds-tps68470.c index d2060fe4259d..44df175d25de 100644 --- a/drivers/leds/leds-tps68470.c +++ b/drivers/leds/leds-tps68470.c @@ -77,23 +77,24 @@ static enum led_brightness tps68470_brightness_get(struct led_classdev *led_cdev int ret = 0; int value = 0; - ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); - if (ret) - return dev_err_probe(led_cdev->dev, -EINVAL, "failed on reading register\n"); - switch (led->led_id) { case TPS68470_ILED_A: - value = value & TPS68470_ILEDCTL_ENA; - break; case TPS68470_ILED_B: - value = value & TPS68470_ILEDCTL_ENB; + ret = regmap_read(regmap, TPS68470_REG_ILEDCTL, &value); + if (ret) + return dev_err_probe(led_cdev->dev, ret, + "failed to read LED status\n"); + + value &= led->led_id == TPS68470_ILED_A ? TPS68470_ILEDCTL_ENA : + TPS68470_ILEDCTL_ENB; break; + default: + return dev_err_probe(led_cdev->dev, -EINVAL, "invalid LED ID\n"); } return value ? LED_ON : LED_OFF; } - static int tps68470_ledb_current_init(struct platform_device *pdev, struct tps68470_device *tps68470) {
We want to extend tps68470_brightness_get() to be usable with the other LEDs supplied by the IC; refactor it to make that easier. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- drivers/leds/leds-tps68470.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)