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

[Jeff LaBundy]

Hi Charles,

On Tue, Apr 11, 2023 at 09:27:08AM +0000, Charles Keepax wrote:
> 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.

Fair enough. I do concede that having this control in the driver as
opposed to DSP FW is more nimble and makes it easier to respond to
customer issues; I'm sure your battle scars will agree :)

> 
> > 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.

D'oh! I meant to say suspend suspend; I'm aligned.

> 
> > 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

Kind regards,
Jeff LaBundy