Re: [PATCH v5 5/6] media: i2c: lm3560: Add support for PM features

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

On Mon, May 04, 2026 at 12:37:40PM +0300, Svyatoslav Ryhel wrote:
> пн, 4 трав. 2026 р. о 10:40 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> >
> > пн, 4 трав. 2026 р. о 09:37 Sakari Ailus <sakari.ailus@linux.intel.com> пише:
> > >
> > > Hi Svyatoslav,
> > >
> > > On Sun, May 03, 2026 at 07:44:44PM +0300, Svyatoslav Ryhel wrote:
> > > > Add support for power management features to better control the LM3560
> > > > within the media framework. To achieve the desired PM support, the HWEN
> > > > GPIO and VIN power supply were added and configured into power on/off
> > > > sequences. Added PM operations along with the PM configuration setup.
> > > >
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > ---
> > > >  drivers/media/i2c/lm3560.c | 117 ++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 110 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/lm3560.c b/drivers/media/i2c/lm3560.c
> > > > index ce4b09d1f208..15741ea5684f 100644
> > > > --- a/drivers/media/i2c/lm3560.c
> > > > +++ b/drivers/media/i2c/lm3560.c
> > > > @@ -12,13 +12,16 @@
> > > >  #include <linux/bitmap.h>
> > > >  #include <linux/delay.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/gpio/consumer.h>
> > > >  #include <linux/i2c.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/mod_devicetable.h>
> > > >  #include <linux/mutex.h>
> > > >  #include <linux/of.h>
> > > > +#include <linux/pm_runtime.h>
> > > >  #include <linux/property.h>
> > > >  #include <linux/regmap.h>
> > > > +#include <linux/regulator/consumer.h>
> > > >  #include <linux/videodev2.h>
> > > >  #include <media/i2c/lm3560.h>
> > > >  #include <media/v4l2-ctrls.h>
> > > > @@ -49,6 +52,8 @@ enum led_enable {
> > > >   * @dev: pointer to &struct device
> > > >   * @regmap: reg. map for i2c
> > > >   * @lock: muxtex for serial access.
> > > > + * @hwen_gpio: line connected to HWEN pin
> > > > + * @vin_supply: line connected to IN supply (2.5V - 5.5V)
> > > >   * @led_mode: V4L2 LED mode
> > > >   * @ctrls_led: V4L2 controls
> > > >   * @subdev_led: V4L2 subdev
> > > > @@ -63,6 +68,9 @@ struct lm3560_flash {
> > > >       struct regmap *regmap;
> > > >       struct mutex lock;
> > > >
> > > > +     struct gpio_desc *hwen_gpio;
> > > > +     struct regulator *vin_supply;
> > > > +
> > > >       enum v4l2_flash_led_mode led_mode;
> > > >       struct v4l2_ctrl_handler ctrls_led[LM3560_LED_MAX];
> > > >       struct v4l2_subdev subdev_led[LM3560_LED_MAX];
> > > > @@ -177,12 +185,17 @@ static int lm3560_get_ctrl(struct v4l2_ctrl *ctrl, enum lm3560_led_id led_no)
> > > >       struct lm3560_flash *flash = to_lm3560_flash(ctrl, led_no);
> > > >       int rval = -EINVAL;
> > > >
> > > > +     if (!pm_runtime_get_if_in_use(flash->dev))
> > > > +             return 0;
> > > > +
> > > >       if (ctrl->id == V4L2_CID_FLASH_FAULT) {
> > > >               s32 fault = 0;
> > > >               unsigned int reg_val;
> > > >               rval = regmap_read(flash->regmap, REG_FLAG, &reg_val);
> > > > -             if (rval < 0)
> > > > +             if (rval < 0) {
> > > > +                     pm_runtime_put(flash->dev);
> > > >                       return rval;
> > > > +             }
> > > >               if (reg_val & FAULT_SHORT_CIRCUIT)
> > > >                       fault |= V4L2_FLASH_FAULT_SHORT_CIRCUIT;
> > > >               if (reg_val & FAULT_OVERTEMP)
> > > > @@ -192,6 +205,8 @@ static int lm3560_get_ctrl(struct v4l2_ctrl *ctrl, enum lm3560_led_id led_no)
> > > >               ctrl->cur.val = fault;
> > > >       }
> > > >
> > > > +     pm_runtime_put(flash->dev);
> > > > +
> > > >       return rval;
> > > >  }
> > > >
> > > > @@ -201,6 +216,9 @@ static int lm3560_set_ctrl(struct v4l2_ctrl *ctrl, enum lm3560_led_id led_no)
> > > >       u8 tout_bits;
> > > >       int rval = -EINVAL;
> > > >
> > > > +     if (!pm_runtime_get_if_in_use(flash->dev))
> > >
> > > This should be pm_runtime_get_if_active().
> > >
> >
> 
> For both lm3560_set_ctrl and lm3560_get_ctrl or only for lm3560_set_ctrl?

For both.

It doesn't matter much at this point as the usage_count is incremented for
each file handle.

> 
> > Noted
> >
> > > > +             return 0;
> > > > +
> > > >       switch (ctrl->id) {
> > > >       case V4L2_CID_FLASH_LED_MODE:
> > > >               flash->led_mode = ctrl->val;
> > > > @@ -246,6 +264,8 @@ static int lm3560_set_ctrl(struct v4l2_ctrl *ctrl, enum lm3560_led_id led_no)
> > > >               break;
> > > >       }
> > > >
> > > > +     pm_runtime_put(flash->dev);
> > > > +
> > > >       return rval;
> > > >  }
> > > >
> > > > @@ -409,6 +429,38 @@ static int lm3560_init_device(struct lm3560_flash *flash)
> > > >       return rval;
> > > >  }
> > > >
> > > > +static int __maybe_unused lm3560_power_off(struct device *dev)
> > > > +{
> > > > +     struct lm3560_flash *flash = dev_get_drvdata(dev);
> > > > +
> > > > +     gpiod_set_value_cansleep(flash->hwen_gpio, 0);
> > > > +     regulator_disable(flash->vin_supply);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int __maybe_unused lm3560_power_on(struct device *dev)
> > > > +{
> > > > +     struct lm3560_flash *flash = dev_get_drvdata(dev);
> > > > +     int rval;
> > > > +
> > > > +     rval = regulator_enable(flash->vin_supply);
> > > > +     if (rval < 0) {
> > > > +             dev_err(flash->dev, "failed to enable vin power supply\n");
> > > > +             return rval;
> > > > +     }
> > > > +
> > > > +     gpiod_set_value_cansleep(flash->hwen_gpio, 1);
> > > > +
> > > > +     rval = lm3560_init_device(flash);
> > > > +     if (rval < 0) {
> > > > +             lm3560_power_off(dev);
> > > > +             return rval;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  static void lm3560_subdev_cleanup(struct lm3560_flash *flash)
> > > >  {
> > > >       int led_no;
> > > > @@ -442,6 +494,17 @@ static int lm3560_probe(struct i2c_client *client)
> > > >
> > > >       bitmap_zero(flash->led_id, LM3560_LED_MAX);
> > > >
> > > > +     flash->hwen_gpio = devm_gpiod_get_optional(flash->dev, "enable",
> > > > +                                                GPIOD_OUT_LOW);
> > > > +     if (IS_ERR(flash->hwen_gpio))
> > > > +             return dev_err_probe(flash->dev, PTR_ERR(flash->hwen_gpio),
> > > > +                                  "failed to get hwen gpio\n");
> > > > +
> > > > +     flash->vin_supply = devm_regulator_get(flash->dev, "vin");
> > > > +     if (IS_ERR(flash->vin_supply))
> > > > +             return dev_err_probe(flash->dev, PTR_ERR(flash->vin_supply),
> > > > +                                  "failed to get vin-supply\n");
> > > > +
> > > >       flash->peak = LM3560_PEAK_1600mA;
> > > >       rval = device_property_read_u32(flash->dev,
> > > >                                       "ti,peak-current-microamp", &peak_ua);
> > > > @@ -469,9 +532,19 @@ static int lm3560_probe(struct i2c_client *client)
> > > >                                &flash->max_flash_timeout);
> > > >       flash->max_flash_timeout /= 1000;
> > > >
> > > > +     rval = regulator_enable(flash->vin_supply);
> > > > +     if (rval < 0)
> > > > +             return dev_err_probe(flash->dev, rval,
> > > > +                                  "failed to enable vin power supply\n");
> > > > +
> > > > +     gpiod_set_value_cansleep(flash->hwen_gpio, 1);
> > > > +
> > > >       rval = lm3560_init_device(flash);
> > > >       if (rval < 0)
> > > > -             return rval;
> > > > +             goto error_disable;
> > > > +
> > > > +     pm_runtime_set_active(flash->dev);
> > > > +     pm_runtime_enable(flash->dev);
> > > >
> > > >       for_each_available_child_of_node(dev_of_node(flash->dev), node) {
> > > >               u32 reg;
> > > > @@ -492,10 +565,10 @@ static int lm3560_probe(struct i2c_client *client)
> > > >
> > > >                       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);
> > > > +                             dev_err(flash->dev,
> > > > +                                     "failed to register led%d: %d\n",
> > > > +                                     reg, rval);
> > > > +                             goto error_clean;
> > > >                       }
> > > >
> > > >                       set_bit(reg, flash->led_id);
> > > > @@ -504,7 +577,23 @@ static int lm3560_probe(struct i2c_client *client)
> > > >
> > > >       i2c_set_clientdata(client, flash);
> > > >
> > > > +     pm_runtime_set_autosuspend_delay(flash->dev, 1000);
> > > > +     pm_runtime_use_autosuspend(flash->dev);
> > > > +     pm_runtime_idle(flash->dev);
> > > > +
> > > >       return 0;
> > > > +
> > > > +error_clean:
> > > > +     pm_runtime_disable(flash->dev);
> > > > +     pm_runtime_set_suspended(flash->dev);
> > > > +
> > > > +     lm3560_subdev_cleanup(flash);
> > > > +
> > > > +error_disable:
> > > > +     gpiod_set_value_cansleep(flash->hwen_gpio, 0);
> > > > +     regulator_disable(flash->vin_supply);
> > > > +
> > > > +     return rval;
> > > >  }
> > > >
> > > >  static void lm3560_remove(struct i2c_client *client)
> > > > @@ -512,8 +601,22 @@ static void lm3560_remove(struct i2c_client *client)
> > > >       struct lm3560_flash *flash = i2c_get_clientdata(client);
> > > >
> > > >       lm3560_subdev_cleanup(flash);
> > > > +
> > > > +     /*
> > > > +      * Disable runtime PM. In case runtime PM is disabled in the kernel,
> > > > +      * make sure to turn power off manually.
> > > > +      */
> > > > +     pm_runtime_disable(&client->dev);
> > > > +     if (!pm_runtime_status_suspended(&client->dev)) {
> > > > +             lm3560_power_off(&client->dev);
> > > > +             pm_runtime_set_suspended(&client->dev);
> > > > +     }
> > > >  }
> > > >
> > > > +static const struct dev_pm_ops lm3560_pm_ops = {
> > > > +     SET_RUNTIME_PM_OPS(lm3560_power_off, lm3560_power_on, NULL)
> > > > +};
> > > > +
> > > >  static const struct of_device_id lm3560_of_match[] = {
> > > >       { .compatible = "ti,lm3559" },
> > > >       { .compatible = "ti,lm3560" },
> > > > @@ -532,7 +635,7 @@ MODULE_DEVICE_TABLE(i2c, lm3560_id_table);
> > > >  static struct i2c_driver lm3560_i2c_driver = {
> > > >       .driver = {
> > > >                  .name = LM3560_NAME,
> > > > -                .pm = NULL,
> > > > +                .pm = pm_ptr(&lm3560_pm_ops),
> > > >                  .of_match_table = lm3560_of_match,
> > > >                  },
> > > >       .probe = lm3560_probe,
> > >
> > > --
> > > Kind regards,
> > >
> > > Sakari Ailus

-- 
Sakari Ailus