by Charles Keepax

[permalink] [raw]

Subject: Re: [PATCH 1/2] Input: cs40l26: Support for CS40L26 Boosted Haptic Amplifier

On Sat, May 06, 2023 at 03:30:17PM -0500, Jeff LaBundy wrote:
> On Thu, May 04, 2023 at 09:51:37PM +0000, Fred Treven wrote:
> > >> +const struct dev_pm_ops cs40l26_pm_ops = {
> > >> + SET_RUNTIME_PM_OPS(cs40l26_suspend, cs40l26_resume, NULL)
> > >> + SET_SYSTEM_SLEEP_PM_OPS(cs40l26_sys_suspend, cs40l26_sys_resume)
> > >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cs40l26_sys_suspend_noirq, cs40l26_sys_resume_noirq)
> > >> +};
> > >> +EXPORT_SYMBOL_GPL(cs40l26_pm_ops);
> > >
> > > Please use latest macros (e.g. DEFINE_SIMPLE_DEV_PM_OPS).
> >
> > When looking at these *_PM_OPS* macros that replace the deprecated versions,
> > it is unclear to me how to maintain support for *_sys_* and
> > *_sys_*_noirq* functions. Would these all need to be separately defined
> > via DEFINE_SIMPLE_DEV_PM_OPS?
> > Would the *_sys_* definitions still be defined through a struct i.e.
> > const struct dev_pm_ops cs40l26_sys_pm_ops which is then exported as it
> > is in my initial submission?
> > I’m unsure how to handle these cases with the latest macros.
>
> I don't happen to see macros for suspend_noirq and resume_noirq, so maybe you
> cannot use macros here after all and will instead have to fall back to tacking
> on __maybe_unused to these callbacks to accommodate the !CONFIG_PM case.
>

Correct this device can not presently use the simple macros.

> That being said, what are you ultimately trying to accomplish here with these
> noirq variants? For example the print statement says "early resume" when in
> fact a different callback exists for that (resume_early).
>
> On that note, why to disable interrupts during system suspend? I can imagine a
> use-case where a customer ties the output of a force sensor to a CS40L26 GPIO
> for low-latency haptic trigger, and then the CS40L26 interrupt output to the
> SoC as a wake-up trigger. Does the part not support this use-case? I vaguely
> seem to remember an issue with this on L25.
>
> Also, why is the logic inverted for the noirq variants? These are simply meant
> to accommodate additional tasks that need a guarantee the device's interrupt
> handler is not running (for example, clear or acknowledge a pending interrupt).
> In case I have misunderstood the intent, please let me know.
>

This is a generic issue with devices that use PM runtime, but
also have IRQs. The system suspend process re-enables IRQs before it
re-enables the PM runtime. This means if your IRQ handler uses PM
runtime and you get an IRQ in that window things don't work. The
simplest solution is to disable IRQs across the window. Ideally
one day this would probably get fixed in the PM core, but that is
likely a massively non-trivial amount of work.

To be clear the code allows IRQs whilst in system suspend (aka wakes)
and whilst resumed. As the IRQ output of the chip is level based, the
temporary disable only causes a slight delay in handling the IRQ.

> One last gripe, then I promise to stop bringing it up :) But the mental gymnastics
> required to explain the no-fewer-than-six PM callbacks used here, as well as how
> to support the !CONFIG_PM case, are in some ways additional nudges toward getting
> rid of this massive amount of PM overhead and relying on the device's internal
> power management as so many modern input devices now do. As a rule of thumb, if
> you're having to jump through a lot of hoops to do simple things that others seem
> to be doing with less work, something is wrong.
>

I am not sure there are significant issues supporting the
!CONFIG_PM case, you need a couple __maybe_unused's. What issues
are you expecting here? Yeah ok you get worse power consumption
in that case, but you did turn off power management, presumably
you were not that concerned about power consumption.

> In your defense, however, you are unlikely to come across many devices that do
> not enable CONFIG_PM given this device's target application. That being said, it
> is not unheard of for OEMs building wall-powered devices to enable CONFIG_PM but
> inhibit system suspend using a wake_lock because of some HW bug.
>

Again remember the system vs runtime suspend here. Holding a
system wake lock will have no effect on the runtime PM.

> Therefore, it seems a bit unfortunate that those use-cases wouldn't get to enjoy
> the power savings this devices offers. That's just my $.02; I also understand
> the reasons behind the current implementation and won't push you to change it.

The power savings from not blocking suspend are tiny, at least
outwith the !CONFIG_PM case. The driver is only blocking hibernate
when it is actively talking to the device, during which time the
device will very likely not be hibernating anyway.

I think really it is up to Fred and Ben who are supporting the
driver. If they feel the device will work reliably that way,
I certainly won't stand in the way. But I would be keen to avoid
a situation where all the downstream implementations (ie. most
of the testing) use PM runtime and the upstream code is full of
corner cases that haven't been ironed out, so I would like to
know they are going to be moving our customers over to this new
mode of operation if they decide to switch to it. Certainly you
are not wrong that it would save a fair amount of code from the
driver and make it look a lot cleaner.

Thanks,
Charles

2023-05-10 14:16:47

by Jeff LaBundy

[permalink] [raw]

Subject: Re: [PATCH 1/2] Input: cs40l26: Support for CS40L26 Boosted Haptic Amplifier

Hi Charles,

On Tue, May 09, 2023 at 10:22:57AM +0000, Charles Keepax wrote:
> On Sat, May 06, 2023 at 03:30:17PM -0500, Jeff LaBundy wrote:
> > On Thu, May 04, 2023 at 09:51:37PM +0000, Fred Treven wrote:
> > > >> +const struct dev_pm_ops cs40l26_pm_ops = {
> > > >> + SET_RUNTIME_PM_OPS(cs40l26_suspend, cs40l26_resume, NULL)
> > > >> + SET_SYSTEM_SLEEP_PM_OPS(cs40l26_sys_suspend, cs40l26_sys_resume)
> > > >> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(cs40l26_sys_suspend_noirq, cs40l26_sys_resume_noirq)
> > > >> +};
> > > >> +EXPORT_SYMBOL_GPL(cs40l26_pm_ops);
> > > >
> > > > Please use latest macros (e.g. DEFINE_SIMPLE_DEV_PM_OPS).
> > >
> > > When looking at these *_PM_OPS* macros that replace the deprecated versions,
> > > it is unclear to me how to maintain support for *_sys_* and
> > > *_sys_*_noirq* functions. Would these all need to be separately defined
> > > via DEFINE_SIMPLE_DEV_PM_OPS?
> > > Would the *_sys_* definitions still be defined through a struct i.e.
> > > const struct dev_pm_ops cs40l26_sys_pm_ops which is then exported as it
> > > is in my initial submission?
> > > I’m unsure how to handle these cases with the latest macros.
> >
> > I don't happen to see macros for suspend_noirq and resume_noirq, so maybe you
> > cannot use macros here after all and will instead have to fall back to tacking
> > on __maybe_unused to these callbacks to accommodate the !CONFIG_PM case.
> >
>
> Correct this device can not presently use the simple macros.
>
> > That being said, what are you ultimately trying to accomplish here with these
> > noirq variants? For example the print statement says "early resume" when in
> > fact a different callback exists for that (resume_early).
> >
> > On that note, why to disable interrupts during system suspend? I can imagine a
> > use-case where a customer ties the output of a force sensor to a CS40L26 GPIO
> > for low-latency haptic trigger, and then the CS40L26 interrupt output to the
> > SoC as a wake-up trigger. Does the part not support this use-case? I vaguely
> > seem to remember an issue with this on L25.
> >
> > Also, why is the logic inverted for the noirq variants? These are simply meant
> > to accommodate additional tasks that need a guarantee the device's interrupt
> > handler is not running (for example, clear or acknowledge a pending interrupt).
> > In case I have misunderstood the intent, please let me know.
> >
>
> This is a generic issue with devices that use PM runtime, but
> also have IRQs. The system suspend process re-enables IRQs before it
> re-enables the PM runtime. This means if your IRQ handler uses PM
> runtime and you get an IRQ in that window things don't work. The
> simplest solution is to disable IRQs across the window. Ideally
> one day this would probably get fixed in the PM core, but that is
> likely a massively non-trivial amount of work.
>
> To be clear the code allows IRQs whilst in system suspend (aka wakes)
> and whilst resumed. As the IRQ output of the chip is level based, the
> temporary disable only causes a slight delay in handling the IRQ.

ACK, thanks for the clarification. A couple suggestions for you Fred:

1. Update the print statements in the noirq variants; currently they say
late/early when in fact these are separate callbacks that occur directly
before/after the noirq callbacks (unless you meant to use late/early)?

2. Consider some comments here to highlight that this driver relies upon
PM callbacks from its interrupt handler and hence disables them briefly
throughout suspend/resume to accommodate the order of operations mentioned
above.

>
> > One last gripe, then I promise to stop bringing it up :) But the mental gymnastics
> > required to explain the no-fewer-than-six PM callbacks used here, as well as how
> > to support the !CONFIG_PM case, are in some ways additional nudges toward getting
> > rid of this massive amount of PM overhead and relying on the device's internal
> > power management as so many modern input devices now do. As a rule of thumb, if
> > you're having to jump through a lot of hoops to do simple things that others seem
> > to be doing with less work, something is wrong.
> >
>
> I am not sure there are significant issues supporting the
> !CONFIG_PM case, you need a couple __maybe_unused's. What issues
> are you expecting here? Yeah ok you get worse power consumption
> in that case, but you did turn off power management, presumably
> you were not that concerned about power consumption.

No concerns really, all I am saying is that in the decision whether to simplify
the driver and allow the DSP FW to naturally manage the device's power state, or
duplicate the logic in the driver and gain explicit control, the option for the
device to behave the same and save a few extra mW regardless of platform config
is one argument for the former (DSP FW).

>
> > In your defense, however, you are unlikely to come across many devices that do
> > not enable CONFIG_PM given this device's target application. That being said, it
> > is not unheard of for OEMs building wall-powered devices to enable CONFIG_PM but
> > inhibit system suspend using a wake_lock because of some HW bug.
> >
>
> Again remember the system vs runtime suspend here. Holding a
> system wake lock will have no effect on the runtime PM.

Good call.

>
> > Therefore, it seems a bit unfortunate that those use-cases wouldn't get to enjoy
> > the power savings this devices offers. That's just my $.02; I also understand
> > the reasons behind the current implementation and won't push you to change it.
>
> The power savings from not blocking suspend are tiny, at least
> outwith the !CONFIG_PM case. The driver is only blocking hibernate
> when it is actively talking to the device, during which time the
> device will very likely not be hibernating anyway.
>
> I think really it is up to Fred and Ben who are supporting the
> driver. If they feel the device will work reliably that way,
> I certainly won't stand in the way. But I would be keen to avoid
> a situation where all the downstream implementations (ie. most
> of the testing) use PM runtime and the upstream code is full of
> corner cases that haven't been ironed out, so I would like to
> know they are going to be moving our customers over to this new
> mode of operation if they decide to switch to it. Certainly you
> are not wrong that it would save a fair amount of code from the
> driver and make it look a lot cleaner.

Agreed; I would maintain parity with the battle-tested implementation that is out
in the wild.

>
> Thanks,
> Charles

Kind regards,
Jeff LaBundy