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

[Charles Keepax]

On Mon, Apr 10, 2023 at 07:31:56PM -0500, Jeff LaBundy wrote:
> On Mon, Apr 10, 2023 at 08:56:34AM +0000, Charles Keepax wrote:
> > On Sat, Apr 08, 2023 at 10:44:39PM -0500, Jeff LaBundy wrote:
> > I would far rather not have every single attempt to communicate
> > with the device wrapped in a retry if the communication failed
> > incase the device is hibernating. It seems much cleaner, and less
> > likely to risk odd behaviour, to know we have brought the device
> > out of hibernation.

> A common way to deal with this is that of [1], where the bus calls
> are simply wrapped with all retry logic limited to two places (read
> and write). These functions could also print the register address
> in case of failure, solving the problem of having dozens of custom
> error messages thorughout the driver.

I suspect this really comes down to a matter of taste, but my
thoughts would be that the code is shorter that way, but not
necessarily simpler. This feels far more error prone and likely
to encounter issues where the device hibernates at a time someone
hadn't properly thought through. I am far more comfortable with
the device is blocked from hibernating whilst the driver is
actively engaged with it and it keeps any special handling for
exiting hibernate in one place.

> Does the current implementation at least allow the device to hibernate
> while the system is otherwise active, as opposed to _only_ during
> runtime suspend? If so, that's still a marked improvement from L25
> era where customers rightfully pointed out that the downstream driver
> was not making efficient use of hibernation. ;)

I am not entirely sure I follow this one, yes the device can only
hibernate whilst it is runtime suspended. But I don't understand
why that is a problem being runtime resumed implies this device
is active, not the system is otherwise active. I am not sure if
I am missing your point or there is some confusion here between
runtime and system suspend. The device can only hibernate during
runtime suspend, but the only thing that determines being runtime
resumed is activity on this device so in general it shouldn't be
hibernating at that point anyway.

> I don't feel particularly strongly about it, so if the current
> implementation will stay, perhaps consider a few comments in this
> area to describe how the device's state is managed.
> 

I certainly never object to adding some comments.

Thanks,
Charles