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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

On Mon, May 04, 2026 at 10:40:11AM +0300, Svyatoslav Ryhel wrote:
> пн, 4 трав. 2026 р. о 09:36 Sakari Ailus <sakari.ailus@linux.intel.com> пише:
> >
> > 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@gmail.com>
> > > ---
> > >  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@list.ti.com>
> > >   */
> > >
> > > +#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

device_for_each_child_node() is indeed meant the be functionally equivalent
to for_each_available_child_of_node().

If you need to hold a reference to the child node -- as I think you do --
you'll need to call fwnode_handle_get() on it, as you'd call of_node_get()
on OF API.

> 
> > > +             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

-- 
Sakari Ailus