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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: Jonathan Cameron <jic23@kernel.org>,
	linux-iio <linux-iio@vger.kernel.org>,
	Gwendal Grignou <gwendal@chromium.org>,
	Andreas Klinger <ak@it-klinger.de>,
	LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>,
	Mike Looijmans <mike.looijmans@topic.nl>,
	Lorenzo Bianconi <lorenzo.bianconi83@gmail.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH 4/6] iio: proximity: srf04: Use pm_ptr() to remove unused struct dev_pm_ops
Date: Mon, 8 Aug 2022 12:26:57 +0200	[thread overview]
Message-ID: <CAHp75VcRMvTQOQso1hwCAnGDGKm4W6Es4=TF-E5OT04-kFnpfg@mail.gmail.com> (raw)
In-Reply-To: <2LKAGR.8070IXEHVPDS3@crapouillou.net>

On Mon, Aug 8, 2022 at 12:17 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le lun., août 8 2022 at 12:09:35 +0200, Andy Shevchenko
> <andy.shevchenko@gmail.com> a écrit :
> > On Mon, Aug 8, 2022 at 11:49 AM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> >>  Le lun., août 8 2022 at 11:39:56 +0200, Andy Shevchenko
> >>  <andy.shevchenko@gmail.com> a écrit :
> >>  > On Mon, Aug 8, 2022 at 11:35 AM Paul Cercueil
> >> <paul@crapouillou.net>
> >>  > wrote:
> >>  >>  Le lun., août 8 2022 at 11:28:12 +0200, Andy Shevchenko
> >>  >>  <andy.shevchenko@gmail.com> a écrit :
> >>  >>  > On Sun, Aug 7, 2022 at 8:46 PM Jonathan Cameron
> >> <jic23@kernel.org>
> >>  >>  > 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.

This is a fair point.

-- 
With Best Regards,
Andy Shevchenko