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

[Svyatoslav Ryhel]

пн, 19 жовт. 2026 р. о 17:44 Lee Jones <lee@xxxxxxxxxx> пише:
>
> Wrong subject line.
>

What do you mean by "Wrong subject line"? Specify.

> 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@xxxxxxxxx>
> > ---
> >  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'?
>

Resolved in v4

https://lore.kernel.org/lkml/20261019100930.110954-1-clamor95@xxxxxxxxx/

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