Re: [PATCH v5 4/6] media: i2c: lm3560: Convert to use OF bindings

[Svyatoslav Ryhel]

пн, 4 трав. 2026 р. о 09:36 Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> пише:
>
> Hi Svyatoslav,
>
> On Sun, May 03, 2026 at 07:44:43PM +0300, Svyatoslav Ryhel wrote:
> > Since there are no users of this driver via platform data, remove the
> > platform data support and switch to using Device Tree bindings.
> >
> > Converting to Device Tree assumes dynamic and independent registration of
> > LEDs. To monitor the configured LEDs, a bitmap has been added. This makes
> > LED cleanup more robust and less context dependent.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > ---
> >  drivers/media/i2c/lm3560.c | 143 ++++++++++++++++++++++++++-----------
> >  include/media/i2c/lm3560.h |  15 ----
> >  2 files changed, 102 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/media/i2c/lm3560.c b/drivers/media/i2c/lm3560.c
> > index 5b568ed9536b..ce4b09d1f208 100644
> > --- a/drivers/media/i2c/lm3560.c
> > +++ b/drivers/media/i2c/lm3560.c
> > @@ -9,11 +9,15 @@
> >   *                   Ldd-Mlp <ldd-mlp@xxxxxxxxxxx>
> >   */
> >
> > +#include <linux/bitmap.h>
> >  #include <linux/delay.h>
> >  #include <linux/module.h>
> >  #include <linux/i2c.h>
> >  #include <linux/slab.h>
> > +#include <linux/mod_devicetable.h>
> >  #include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/property.h>
> >  #include <linux/regmap.h>
> >  #include <linux/videodev2.h>
> >  #include <media/i2c/lm3560.h>
> > @@ -43,22 +47,33 @@ enum led_enable {
> >   * struct lm3560_flash
> >   *
> >   * @dev: pointer to &struct device
> > - * @pdata: platform data
> >   * @regmap: reg. map for i2c
> >   * @lock: muxtex for serial access.
> >   * @led_mode: V4L2 LED mode
> >   * @ctrls_led: V4L2 controls
> >   * @subdev_led: V4L2 subdev
> > + * @led_id: LED status holder
> > + * @peak: peak current
> > + * @max_flash_timeout: flash timeout
> > + * @max_flash_brt: flash mode led brightness
> > + * @max_torch_brt: torch mode led brightness
> >   */
> >  struct lm3560_flash {
> >       struct device *dev;
> > -     struct lm3560_platform_data *pdata;
> >       struct regmap *regmap;
> >       struct mutex lock;
> >
> >       enum v4l2_flash_led_mode led_mode;
> >       struct v4l2_ctrl_handler ctrls_led[LM3560_LED_MAX];
> >       struct v4l2_subdev subdev_led[LM3560_LED_MAX];
> > +
> > +     DECLARE_BITMAP(led_id, LM3560_LED_MAX);
> > +
> > +     enum lm3560_peak_current peak;
> > +     u32 max_flash_timeout;
> > +
> > +     u32 max_flash_brt[LM3560_LED_MAX];
> > +     u32 max_torch_brt[LM3560_LED_MAX];
> >  };
> >
> >  #define to_lm3560_flash(_ctrl, _no)  \
> > @@ -269,8 +284,8 @@ static int lm3560_init_controls(struct lm3560_flash *flash,
> >                               enum lm3560_led_id led_no)
> >  {
> >       struct v4l2_ctrl *fault;
> > -     u32 max_flash_brt = flash->pdata->max_flash_brt[led_no];
> > -     u32 max_torch_brt = flash->pdata->max_torch_brt[led_no];
> > +     u32 max_flash_brt = flash->max_flash_brt[led_no];
> > +     u32 max_torch_brt = flash->max_torch_brt[led_no];
> >       struct v4l2_ctrl_handler *hdl = &flash->ctrls_led[led_no];
> >       const struct v4l2_ctrl_ops *ops = &lm3560_led_ctrl_ops[led_no];
> >
> > @@ -295,9 +310,9 @@ static int lm3560_init_controls(struct lm3560_flash *flash,
> >       /* flash strobe timeout */
> >       v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FLASH_TIMEOUT,
> >                         LM3560_FLASH_TOUT_MIN,
> > -                       flash->pdata->max_flash_timeout,
> > +                       flash->max_flash_timeout,
> >                         LM3560_FLASH_TOUT_STEP,
> > -                       flash->pdata->max_flash_timeout);
> > +                       flash->max_flash_timeout);
> >
> >       /* flash brt */
> >       v4l2_ctrl_new_std(hdl, ops, V4L2_CID_FLASH_INTENSITY,
> > @@ -339,15 +354,18 @@ static const struct regmap_config lm3560_regmap = {
> >  };
> >
> >  static int lm3560_subdev_init(struct lm3560_flash *flash,
> > -                           enum lm3560_led_id led_no, char *led_name)
> > +                           enum lm3560_led_id led_no,
> > +                           struct device_node *np)
> >  {
> >       struct i2c_client *client = to_i2c_client(flash->dev);
> >       int rval;
> >
> >       v4l2_i2c_subdev_init(&flash->subdev_led[led_no], client, &lm3560_ops);
> >       flash->subdev_led[led_no].flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > -     strscpy(flash->subdev_led[led_no].name, led_name,
> > -             sizeof(flash->subdev_led[led_no].name));
> > +     snprintf(flash->subdev_led[led_no].name,
> > +              sizeof(flash->subdev_led[led_no].name),
> > +              "lm3560-led%d", led_no);
> > +     flash->subdev_led[led_no].fwnode = of_fwnode_handle(np);
> >       rval = lm3560_init_controls(flash, led_no);
> >       if (rval)
> >               goto err_out;
> > @@ -378,7 +396,7 @@ static int lm3560_init_device(struct lm3560_flash *flash)
> >
> >       /* set peak current */
> >       rval = regmap_update_bits(flash->regmap,
> > -                               REG_FLASH_TOUT, 0x60, flash->pdata->peak);
> > +                               REG_FLASH_TOUT, 0x60, flash->peak);
> >       if (rval < 0)
> >               return rval;
> >       /* output disable */
> > @@ -391,18 +409,22 @@ static int lm3560_init_device(struct lm3560_flash *flash)
> >       return rval;
> >  }
> >
> > -static void lm3560_subdev_cleanup(struct lm3560_flash *flash,
> > -                               enum lm3560_led_id led_no)
> > +static void lm3560_subdev_cleanup(struct lm3560_flash *flash)
> >  {
> > -     v4l2_async_unregister_subdev(&flash->subdev_led[led_no]);
> > -     v4l2_ctrl_handler_free(&flash->ctrls_led[led_no]);
> > -     media_entity_cleanup(&flash->subdev_led[led_no].entity);
> > +     int led_no;
> > +
> > +     for_each_set_bit(led_no, flash->led_id, LM3560_LED_MAX) {
> > +             v4l2_async_unregister_subdev(&flash->subdev_led[led_no]);
> > +             v4l2_ctrl_handler_free(&flash->ctrls_led[led_no]);
> > +             media_entity_cleanup(&flash->subdev_led[led_no].entity);
> > +     }
> >  }
> >
> >  static int lm3560_probe(struct i2c_client *client)
> >  {
> >       struct lm3560_flash *flash;
> > -     struct lm3560_platform_data *pdata = dev_get_platdata(&client->dev);
> > +     struct device_node *node;
> > +     u32 peak_ua;
> >       int rval;
> >
> >       flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
> > @@ -415,36 +437,69 @@ static int lm3560_probe(struct i2c_client *client)
> >               return rval;
> >       }
> >
> > -     /* if there is no platform data, use chip default value */
> > -     if (pdata == NULL) {
> > -             pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> > -             if (pdata == NULL)
> > -                     return -ENODEV;
> > -             pdata->peak = LM3560_PEAK_3600mA;
> > -             pdata->max_flash_timeout = LM3560_FLASH_TOUT_MAX;
> > -             /* led 1 */
> > -             pdata->max_flash_brt[LM3560_LED0] = LM3560_FLASH_BRT_MAX;
> > -             pdata->max_torch_brt[LM3560_LED0] = LM3560_TORCH_BRT_MAX;
> > -             /* led 2 */
> > -             pdata->max_flash_brt[LM3560_LED1] = LM3560_FLASH_BRT_MAX;
> > -             pdata->max_torch_brt[LM3560_LED1] = LM3560_TORCH_BRT_MAX;
> > -     }
> > -     flash->pdata = pdata;
> >       flash->dev = &client->dev;
> >       mutex_init(&flash->lock);
> >
> > +     bitmap_zero(flash->led_id, LM3560_LED_MAX);
> > +
> > +     flash->peak = LM3560_PEAK_1600mA;
> > +     rval = device_property_read_u32(flash->dev,
> > +                                     "ti,peak-current-microamp", &peak_ua);
> > +     if (!rval) {
> > +             switch (peak_ua) {
> > +             case 1600000:
> > +                     flash->peak = LM3560_PEAK_1600mA;
> > +                     break;
> > +             case 2300000:
> > +                     flash->peak = LM3560_PEAK_2300mA;
> > +                     break;
> > +             case 3000000:
> > +                     flash->peak = LM3560_PEAK_3000mA;
> > +                     break;
> > +             case 3600000:
> > +                     flash->peak = LM3560_PEAK_3600mA;
> > +                     break;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     flash->max_flash_timeout = LM3560_FLASH_TOUT_MIN * 1000;
> > +     device_property_read_u32(flash->dev, "flash-max-timeout-us",
> > +                              &flash->max_flash_timeout);
> > +     flash->max_flash_timeout /= 1000;
> > +
> >       rval = lm3560_init_device(flash);
> >       if (rval < 0)
> >               return rval;
> >
> > -     rval = lm3560_subdev_init(flash, LM3560_LED0, "lm3560-led0");
> > -     if (rval < 0)
> > -             return rval;
> > +     for_each_available_child_of_node(dev_of_node(flash->dev), node) {
>
> device_for_each_child_node(), please.
>
> > +             u32 reg;
> >
> > -     rval = lm3560_subdev_init(flash, LM3560_LED1, "lm3560-led1");
> > -     if (rval < 0) {
> > -             lm3560_subdev_cleanup(flash, LM3560_LED0);
> > -             return rval;
> > +             rval = of_property_read_u32(node, "reg", &reg);
>
> device_property_read_u32() here and elsewhere.
>

I have switched to OF equivalent deliberately because if I use
device_for_each_child_node I found no sane way to pass fwnode to
subdev_led since device_for_each_child_node releases fwnode handle it
uses and I cannot assign it to subdev_led.fwnode since it will result
in NULL pointer on loop exit

> > +             if (rval < 0)
> > +                     /* We care only about nodes with reg property */
> > +                     continue;
> > +
> > +             if (reg == LM3560_LED0 || reg == LM3560_LED1) {
> > +                     flash->max_flash_brt[reg] = LM3560_FLASH_BRT_MIN;
> > +                     of_property_read_u32(node, "flash-max-microamp",
> > +                                          &flash->max_flash_brt[reg]);
> > +
> > +                     flash->max_torch_brt[reg] = LM3560_TORCH_BRT_MIN;
> > +                     of_property_read_u32(node, "led-max-microamp",
> > +                                          &flash->max_torch_brt[reg]);
> > +
> > +                     rval = lm3560_subdev_init(flash, reg, node);
> > +                     if (rval < 0) {
> > +                             lm3560_subdev_cleanup(flash);
> > +                             return dev_err_probe(flash->dev, rval,
> > +                                                 "failed to register led%d\n",
> > +                                                 reg);
> > +                     }
> > +
> > +                     set_bit(reg, flash->led_id);
> > +             }
> >       }
> >
> >       i2c_set_clientdata(client, flash);
> > @@ -455,12 +510,17 @@ static int lm3560_probe(struct i2c_client *client)
> >  static void lm3560_remove(struct i2c_client *client)
> >  {
> >       struct lm3560_flash *flash = i2c_get_clientdata(client);
> > -     unsigned int i;
> >
> > -     for (i = LM3560_LED0; i < LM3560_LED_MAX; i++)
> > -             lm3560_subdev_cleanup(flash, i);
> > +     lm3560_subdev_cleanup(flash);
> >  }
> >
> > +static const struct of_device_id lm3560_of_match[] = {
> > +     { .compatible = "ti,lm3559" },
> > +     { .compatible = "ti,lm3560" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, lm3560_of_match);
> > +
> >  static const struct i2c_device_id lm3560_id_table[] = {
> >       { LM3559_NAME },
> >       { LM3560_NAME },
> > @@ -473,6 +533,7 @@ static struct i2c_driver lm3560_i2c_driver = {
> >       .driver = {
> >                  .name = LM3560_NAME,
> >                  .pm = NULL,
> > +                .of_match_table = lm3560_of_match,
> >                  },
> >       .probe = lm3560_probe,
> >       .remove = lm3560_remove,
> > diff --git a/include/media/i2c/lm3560.h b/include/media/i2c/lm3560.h
> > index 770d8c72c94a..b56c1ff8fd49 100644
> > --- a/include/media/i2c/lm3560.h
> > +++ b/include/media/i2c/lm3560.h
> > @@ -66,19 +66,4 @@ enum lm3560_peak_current {
> >       LM3560_PEAK_3600mA = 0x60
> >  };
> >
> > -/* struct lm3560_platform_data
> > - *
> > - * @peak :  peak current
> > - * @max_flash_timeout: flash timeout
> > - * @max_flash_brt: flash mode led brightness
> > - * @max_torch_brt: torch mode led brightness
> > - */
> > -struct lm3560_platform_data {
> > -     enum lm3560_peak_current peak;
> > -
> > -     u32 max_flash_timeout;
> > -     u32 max_flash_brt[LM3560_LED_MAX];
> > -     u32 max_torch_brt[LM3560_LED_MAX];
> > -};
> > -
> >  #endif /* __LM3560_H__ */
>
> --
> Kind regards,
>
> Sakari Ailus