Re: [PATCH v3 2/4] mfd: aat2870: Derive brightness steps from max current

[Lee Jones <lee@kernel.org>]

Wrong subject line.

Please review, explain or fix the Sashiko review(s).
If this is a set, this request includes the other patches too.

> The AAT2870 allows only 32 steps of brightness, with this amount defined
> by the maximum supported current. Remove the need for max_brightness by
> calculating the allowed number of steps from the maximum current.
>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> ---
>  drivers/video/backlight/aat2870_bl.c | 30 ++++++++++------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/video/backlight/aat2870_bl.c b/drivers/video/backlight/aat2870_bl.c
> index 8b790df1e842b..eb686e315921d 100644
> --- a/drivers/video/backlight/aat2870_bl.c
> +++ b/drivers/video/backlight/aat2870_bl.c
> @@ -24,18 +24,6 @@ struct aat2870_bl_driver_data {
>  	int brightness; /* current brightness */
>  };
>
> -static inline int aat2870_brightness(struct aat2870_bl_driver_data *aat2870_bl,
> -				     int brightness)
> -{
> -	struct backlight_device *bd = aat2870_bl->bd;
> -	int val;
> -
> -	val = brightness * (aat2870_bl->max_current - 1);
> -	val /= bd->props.max_brightness;
> -
> -	return val;
> -}
> -
>  static inline int aat2870_bl_enable(struct aat2870_bl_driver_data *aat2870_bl)
>  {
>  	struct aat2870_data *aat2870
> @@ -69,10 +57,11 @@ static int aat2870_bl_update_status(struct backlight_device *bd)
>  	dev_dbg(&bd->dev, "brightness=%d, power=%d, state=%d\n",
>  		 bd->props.brightness, bd->props.power, bd->props.state);
>
> -	ret = aat2870->write(aat2870, AAT2870_BLM,
> -			     (u8)aat2870_brightness(aat2870_bl, brightness));
> -	if (ret < 0)
> -		return ret;
> +	if (brightness) {
> +		ret = aat2870->write(aat2870, AAT2870_BLM, (u8)brightness - 1);
> +		if (ret < 0)
> +			return ret;
> +	}
>
>  	if (brightness == 0) {
>  		ret = aat2870_bl_disable(aat2870_bl);
> @@ -150,10 +139,11 @@ static int aat2870_bl_probe(struct platform_device *pdev)
>  	else
>  		aat2870_bl->max_current = AAT2870_CURRENT_27_9;
>
> -	if (pdata->max_brightness > 0)
> -		bd->props.max_brightness = pdata->max_brightness;
> -	else
> -		bd->props.max_brightness = 255;
> +	/*
> +	 * The maximum number of brightness steps is determined by the max current.
> +	 * The aat2870_current enum maps directly to the allowed brightness steps.
> +	 */
> +	bd->props.max_brightness = pdata->max_current;

What happens if 'pdata->max_current' is 0?  Above, we fall back to
AAT2870_CURRENT_27_9 when assigning 'aat2870_bl->max_current'.  Shouldn't this
use 'aat2870_bl->max_current' instead, and if 'aat2870_bl->max_current' is no
longer used anywhere else, should it be dropped from 'aat2870_bl_driver_data'?

>
>  	aat2870_bl->brightness = 0;
>  	bd->props.power = BACKLIGHT_POWER_ON;
> -- 
> 2.53.0
>

-- 
Lee Jones