Re: [PATCH v1] video: leds: backlight: lm3533: Support getting LED sources from DT

[Svyatoslav Ryhel]

пт, 13 лист. 2026 р. о 16:53 Lee Jones <lee@xxxxxxxxxx> пише:
>
> Drop the 'video: ' part of the subject line.
>
> Please review, explain or fix the Sashiko review(s).
>
> > Add Control Bank to HVLED/LVLED muxing support based on the led-sources
> > defined in the device tree.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > Reviewed-by: Daniel Thompson (RISCstar) <danielt@xxxxxxxxxx>
> > ---
> > This was part of the original LM3533 controller DT/OF conversion patchset,
> > but it appears to have been accidentally separated during the final iteration.
> > Therefore, I am sending it as a follow-up.
> > ---
> >  drivers/leds/leds-lm3533.c          | 60 +++++++++++++++++++++++++++++
> >  drivers/video/backlight/lm3533_bl.c | 45 ++++++++++++++++++++++
>
> Why are these being bundled up?
>
> What is the dependency?  If none, please separate.
>
> >  2 files changed, 105 insertions(+)
> >
> > diff --git a/drivers/leds/leds-lm3533.c b/drivers/leds/leds-lm3533.c
> > index 7259097bd6a71..a1aeff70a90fc 100644
> > --- a/drivers/leds/leds-lm3533.c
> > +++ b/drivers/leds/leds-lm3533.c
> > @@ -26,6 +26,11 @@
> >  #define LM3533_ALS_CHANNEL_LV_MIN    1
> >  #define LM3533_ALS_CHANNEL_LV_MAX    2
> >
> > +#define LM3533_REG_OUTPUT_CONF1                      0x10
> > +#define   OUTPUT_CONF1_SHIFT                 2
> > +#define   OUTPUT_LVLED_MASK                  0x3
> > +#define LM3533_REG_OUTPUT_CONF2                      0x11
> > +#define   OUTPUT_CONF2_SHIFT                 6
> >  #define LM3533_REG_CTRLBANK_BCONF_BASE               0x1b
> >  #define LM3533_REG_PATTERN_ENABLE            0x28
> >  #define LM3533_REG_PATTERN_LOW_TIME_BASE     0x71
> > @@ -54,6 +59,9 @@ struct lm3533_led {
> >       u32 max_current;
> >       u32 pwm;
> >
> > +     int num_leds;
> > +     u32 leds[LM3533_LVCTRLBANK_MAX];
> > +
> >       bool have_als;
> >  };
> >
> > @@ -622,8 +630,36 @@ static const struct attribute_group *lm3533_led_attribute_groups[] = {
> >
> >  static int lm3533_led_setup(struct lm3533_led *led)
> >  {
> > +     u32 output_cfg_shift = 0;
> > +     u32 output_cfg_val = 0;
> > +     u32 output_cfg_mask = 0;
> >       int ret;
> >
> > +     if (led->num_leds) {
> > +             for (int i = 0; i < led->num_leds; i++) {
> > +                     if (led->leds[i] >= LM3533_LVCTRLBANK_MAX)
> > +                             continue;
> > +
> > +                     output_cfg_shift = led->leds[i] * 2;
> > +                     output_cfg_val |= led->id << output_cfg_shift;
> > +                     output_cfg_mask |= OUTPUT_LVLED_MASK << output_cfg_shift;
> > +             }
> > +
> > +             /* LVLED1, LVLED2 and LVLED3 */
> > +             ret = regmap_update_bits(led->regmap, LM3533_REG_OUTPUT_CONF1,
> > +                                      output_cfg_mask << OUTPUT_CONF1_SHIFT,
> > +                                      output_cfg_val << OUTPUT_CONF1_SHIFT);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             /* LVLED4 and LVLED5 */
> > +             ret = regmap_update_bits(led->regmap, LM3533_REG_OUTPUT_CONF2,
> > +                                      output_cfg_mask >> OUTPUT_CONF2_SHIFT,
> > +                                      output_cfg_val >> OUTPUT_CONF2_SHIFT);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       ret = lm3533_ctrlbank_set_max_current(&led->cb, led->max_current);
> >       if (ret)
> >               return ret;
> > @@ -698,6 +734,30 @@ static int lm3533_led_probe(struct platform_device *pdev)
> >
> >       device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &led->pwm);
> >
> > +     /*
> > +      * If led-sources property is not set then either this Control Bank uses
> > +      * its default LVLED or is not linked to any LVLED at all.
> > +      */
> > +     led->num_leds = device_property_count_u32(&pdev->dev, "led-sources");
> > +     if (led->num_leds > LM3533_LVCTRLBANK_MAX) {
> > +             dev_err(&pdev->dev, "num of LED sources exceeds max %d: %d\n",
>
> "Number "
>
> > +                     LM3533_LVCTRLBANK_MAX, led->num_leds);
> > +             ret = -EINVAL;
> > +             goto err_deregister;
> > +     }
>
> Could we move the 'led-sources' parsing above led_classdev_register(),
> or convert the driver to devm_led_classdev_register() and use 'return
> dev_err_probe()' here?
>

I will convert to devm_led_classdev_register() as precursor to this patch.

> > +     if (led->num_leds < 0)
> > +             led->num_leds = 0;
> > +
> > +     if (led->num_leds > 0) {
> > +             ret = device_property_read_u32_array(&pdev->dev, "led-sources",
> > +                                                  led->leds, led->num_leds);
> > +             if (ret) {
> > +                     dev_err(&pdev->dev, "failed to get led-sources\n");
> > +                     goto err_deregister;
> > +             }
>
> dev_err_probe()
>
> > +     }
> > +
> >       ret = lm3533_led_setup(led);
> >       if (ret)
> >               goto err_deregister;
> > diff --git a/drivers/video/backlight/lm3533_bl.c b/drivers/video/backlight/lm3533_bl.c
> > index adcd6ac7cf229..e8164c8528577 100644
> > --- a/drivers/video/backlight/lm3533_bl.c
> > +++ b/drivers/video/backlight/lm3533_bl.c
> > @@ -7,6 +7,7 @@
> >   * Author: Johan Hovold <jhovold@xxxxxxxxx>
> >   */
> >
> > +#include <linux/bits.h>
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> >  #include <linux/platform_device.h>
> > @@ -21,6 +22,7 @@
> >  #define LM3533_HVCTRLBANK_COUNT              2
> >  #define LM3533_BL_MAX_BRIGHTNESS     255
> >
> > +#define LM3533_REG_OUTPUT_CONF1              0x10
> >  #define LM3533_REG_CTRLBANK_AB_BCONF 0x1a
> >  #define   CTRLBANK_AB_BCONF_ALS(n)   BIT(2 * (n))
> >  #define   CTRLBANK_AB_BCONF_MODE(n)  BIT(2 * (n) + 1)
> > @@ -35,6 +37,9 @@ struct lm3533_bl {
> >       u32 max_current;
> >       u32 pwm;
> >
> > +     int num_leds;
> > +     u32 led_strings[LM3533_HVCTRLBANK_COUNT];
> > +
> >       bool have_als;
> >       bool linear;
> >  };
> > @@ -236,6 +241,8 @@ static const struct attribute_group *lm3533_bl_attribute_groups[] = {
> >  static int lm3533_bl_setup(struct lm3533_bl *bl)
> >  {
> >       int ctrlbank = lm3533_bl_get_ctrlbank_id(bl);
> > +     u32 output_cfg_val = 0;
> > +     u32 output_cfg_mask = 0;
> >       int ret;
> >
> >       ret = regmap_assign_bits(bl->regmap, LM3533_REG_CTRLBANK_AB_BCONF,
> > @@ -243,6 +250,21 @@ static int lm3533_bl_setup(struct lm3533_bl *bl)
> >       if (ret)
> >               return ret;
> >
> > +     if (bl->num_leds) {
> > +             for (int i = 0; i < bl->num_leds; i++) {
> > +                     if (bl->led_strings[i] >= LM3533_HVCTRLBANK_COUNT)
> > +                             continue;
> > +
> > +                     output_cfg_val |= ctrlbank << bl->led_strings[i];
> > +                     output_cfg_mask |= BIT(bl->led_strings[i]);
> > +             }
>
> This for loop doesn't need to be in the if statement.
>

It is in if statement because if node has no "led-sources"
LM3533_REG_OUTPUT_CONF1 should not be touched and must be left in its
default state.

> > +             ret = regmap_update_bits(bl->regmap, LM3533_REG_OUTPUT_CONF1,
> > +                                      output_cfg_mask, output_cfg_val);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       ret = lm3533_ctrlbank_set_max_current(&bl->cb, bl->max_current);
> >       if (ret)
> >               return ret;
> > @@ -320,6 +342,29 @@ static int lm3533_bl_probe(struct platform_device *pdev)
> >
> >       device_property_read_u32(&pdev->dev, "ti,pwm-config-mask", &bl->pwm);
> >
> > +     /*
> > +      * If led-sources property is not set then either this Control Bank uses
> > +      * its default HVLED or is not linked to any HVLED at all.
> > +      */
> > +     bl->num_leds = device_property_count_u32(&pdev->dev, "led-sources");
> > +     if (bl->num_leds > LM3533_HVCTRLBANK_COUNT) {
> > +             dev_err(&pdev->dev, "num of LED sources %d exceeds max %d\n",
> > +                     bl->num_leds, LM3533_HVCTRLBANK_COUNT);
> > +             return -EINVAL;
> > +     }
>
> dev_err_probe()
>
> > +
> > +     if (bl->num_leds < 0)
> > +             bl->num_leds = 0;
> > +
> > +     if (bl->num_leds > 0) {
> > +             ret = device_property_read_u32_array(&pdev->dev, "led-sources",
> > +                                                  bl->led_strings,
> > +                                                  bl->num_leds);
> > +             if (ret)
> > +                     return dev_err_probe(&pdev->dev, ret,
> > +                                          "failed to get led-sources\n");
> > +     }
> > +
> >       ret = lm3533_bl_setup(bl);
> >       if (ret)
> >               return ret;
> > --
> > 2.53.0
> >
>

Other suggestions are valid, I will apply them in v3

> --
> Lee Jones