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

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

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