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

Svyatoslav Ryhel
2026-05-04T07:40:24Z

пн, 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

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