Re: [PATCH 4/6] iio: proximity: srf04: Use pm_ptr() to remove unused struct dev_pm_ops

[Paul Cercueil <paul@xxxxxxxxxxxxxxx>]

Le lun., août 8 2022 at 12:09:35 +0200, Andy Shevchenko 
<andy.shevchenko@xxxxxxxxx> a écrit :
> On Mon, Aug 8, 2022 at 11:49 AM Paul Cercueil <paul@xxxxxxxxxxxxxxx> 
> wrote:
> >  Le lun., août 8 2022 at 11:39:56 +0200, Andy Shevchenko
> >  <andy.shevchenko@xxxxxxxxx> a écrit :
> >  > On Mon, Aug 8, 2022 at 11:35 AM Paul Cercueil 
> > <paul@xxxxxxxxxxxxxxx>
> >  > wrote:
> >  >>  Le lun., août 8 2022 at 11:28:12 +0200, Andy Shevchenko
> >  >>  <andy.shevchenko@xxxxxxxxx> a écrit :
> >  >>  > On Sun, Aug 7, 2022 at 8:46 PM Jonathan Cameron 
> > <jic23@xxxxxxxxxx>
> >  >>  > wrote:
> >  >
> >  > ...
> >  >
> >  >>  >>  In this case we can't simply use DEFINE_RUNTIME_DEV_PM_OPS()
> >  >> because
> >  >>  >>  that would provide suspend and resume functions without the
> >  >>  >>  checks the driver is doing before calling runtime_pm 
> > functions
> >  >>  >>  (whether the necessary GPIO is provided).  It may be 
> > possible to
> >  >>  >>  clean that up in future by moving the checks into the 
> > callbacks.
> >  >>  >
> >  >>  > ...
> >  >>  >
> >  >>  >>   static const struct dev_pm_ops srf04_pm_ops = {
> >  >>  >>  -       SET_RUNTIME_PM_OPS(srf04_pm_runtime_suspend,
> >  >>  >>  -                               srf04_pm_runtime_resume, 
> > NULL)
> >  >>  >>  +       RUNTIME_PM_OPS(srf04_pm_runtime_suspend,
> >  >>  >>  +                      srf04_pm_runtime_resume, NULL)
> >  >>  >>   };
> >  >>  >
> >  >>  > static DEFINE_RUNTIME_DEV_PM_OPS(...);
> >  >>  >
> >  >>  > ?
> >  >>
> >  >>  Read the commit message?
> >  >
> >  > Yes, and I'm not sure how that part is relevant. The callbacks 
> > won't
> >  > be called if pm_ptr() equals no-op, no?
> >
> >  Have a look at the definition of DEFINE_RUNTIME_DEV_PM_OPS(). I 
> > believe
> >  it does not do what you think it does.
> >
> >  What the commit message says is that using 
> > DEFINE_RUNTIME_DEV_PM_OPS()
> >  would add .suspend/.resume callbacks, which aren't provided with the
> >  current code.
>
> Effectively the use of DEFINE_RUNTIME_DEV_PM_OPS() enables system
> sleep with the same callbacks as runtime PM. I don't see any
> disadvantages of using it instead of keeping runtime PM only. That
> said, I don't see the commit message that describes that nuance. It
> rather says, as I said, something irrelevant.

Probably no disavantages, yes, but that would be a functional change, 
so I can understand why it's not done in this patchset. That doesn't 
mean it cannot be done later, though, but somebody would need to test 
it on hardware.

Cheers,
-Paul