Re: [PATCH 5/8] leds: tps68470: Refactor tps68470_brightness_get()

[Hans de Goede <hdegoede@redhat.com>]

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)
>>>   {
>