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

[Svyatoslav Ryhel]

вт, 21 квіт. 2026 р. о 22:59 Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> пише:
>
> Hi Svyatoslav,
>
> On Tue, Apr 21, 2026 at 08:32:16PM +0300, Svyatoslav Ryhel wrote:
> > вт, 21 квіт. 2026 р. о 19:44 Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> пише:
> > >
> > > Hi Shyvatoslav,
> > >
> > > On Sun, Apr 19, 2026 at 12:34:12PM +0300, Svyatoslav Ryhel wrote:
> > > > @@ -403,6 +423,60 @@ static int lm3560_init_device(struct lm3560_flash *flash)
> > > >       return rval;
> > > >  }
> > > >
> > > > +static void lm3560_power_off(struct lm3560_flash *flash)
> > > > +{
> > > > +     gpiod_set_value_cansleep(flash->hwen_gpio, 0);
> > > > +     regulator_disable(flash->vin_supply);
> > > > +}
> > > > +
> > > > +static int lm3560_power_on(struct lm3560_flash *flash)
> > > > +{
> > > > +     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(flash);
> > > > +             return rval;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int __maybe_unused lm3560_pm_runtime_resume(struct device *dev)
> > > > +{
> > > > +     struct lm3560_flash *flash = dev_get_drvdata(dev);
> > > > +
> > > > +     return lm3560_power_on(flash);
> > > > +}
> > > > +
> > > > +static int __maybe_unused lm3560_pm_runtime_suspend(struct device *dev)
> > >
> > > Could you change lm3560_power_o{n,ff}() take struct device pointer as the
> > > argument?
> > >
> >
> > What benefit it will bring? Unless you propose to use on/off functions
> > in pm directly, I don't see any benefit in this change.
>
> You get rid of two redundant wrappers.
>

Noted. Any changes required into the other patches of the patchset?

> >
> > > > +{
> > > > +     struct lm3560_flash *flash = dev_get_drvdata(dev);
> > > > +
> > > > +     lm3560_power_off(flash);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static void lm3560_subdev_cleanup(struct lm3560_flash *flash)
> > > > +{
> > > > +     unsigned int i;
> > > > +
> > > > +     for (i = LM3560_LED0; i < LM3560_LED_MAX; i++) {
> > >
> > > You could define i here.
> > >
> >
> > You mean:
> >  for (unsigned int i = LM3560_LED0; i < LM3560_LED_MAX; i++)?
> >
> > If yes, I would like to keep it as is, it looks cleaner to me, and
> > this part was transferred from remove as is.
>
> Works for me.
>
> --
> Regards,
>
> Sakari Ailus