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

[Jeff LaBundy <jeff@labundy.com>]

Hi Fred,

On Thu, May 04, 2023 at 09:51:37PM +0000, Fred Treven wrote:
> Hi Jeff,
> 
> I had one last question for you before I send the next version of this submission.
> 
> >> +
> >> + return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(cs40l26_remove);
> >> +
> >> +static int cs40l26_suspend(struct device *dev)
> >> +{
> >> + struct cs40l26_private *cs40l26 = dev_get_drvdata(dev);
> >> + int ret;
> >> +
> >> + mutex_lock(&cs40l26->lock);
> >> + dev_dbg(dev, "%s: Enabling hibernation\n", __func__);
> >> +
> >> + ret = cs40l26_allow_hiber(cs40l26);
> >> +
> >> + mutex_unlock(&cs40l26->lock);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int cs40l26_sys_suspend(struct device *dev)
> >> +{
> >> + struct i2c_client *i2c_client = to_i2c_client(dev);
> >> +
> >> + dev_dbg(dev, "System suspend, disabling IRQ\n");
> >> +
> >> + disable_irq(i2c_client->irq);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int cs40l26_sys_suspend_noirq(struct device *dev)
> >> +{
> >> + struct i2c_client *i2c_client = to_i2c_client(dev);
> >> +
> >> + dev_dbg(dev, "Late system suspend, re-enabling IRQ\n");
> >> + enable_irq(i2c_client->irq);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int cs40l26_resume(struct device *dev)
> >> +{
> >> + dev_dbg(dev, "%s: Disabling hibernation\n", __func__);
> >> +
> >> + return cs40l26_prevent_hiber(dev_get_drvdata(dev));
> >> +}
> >> +
> >> +static int cs40l26_sys_resume(struct device *dev)
> >> +{
> >> + struct i2c_client *i2c_client = to_i2c_client(dev);
> >> +
> >> + dev_dbg(dev, "System resume, re-enabling IRQ\n");
> >> +
> >> + enable_irq(i2c_client->irq);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int cs40l26_sys_resume_noirq(struct device *dev)
> >> +{
> >> + struct i2c_client *i2c_client = to_i2c_client(dev);
> >> +
> >> + dev_dbg(dev, "Early system resume, disabling IRQ\n");
> >> +
> >> + disable_irq(i2c_client->irq);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +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.

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.

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.

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.

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.

> 
> Appreciate any guidance you can provide.
> 
> Best regards,
> Fred

Kind regards,
Jeff LaBundy