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
