Re: [PATCH 1/2] gpio: gpiolib: Expand sleep tolerance to include controller reset

[Andrew Jeffery]

On Thu, 2017-10-26 at 10:03 +0100, Charles Keepax wrote:
> On Thu, Oct 26, 2017 at 11:10:53AM +1030, Andrew Jeffery wrote:
> > On Wed, 2017-10-25 at 09:32 +0100, Charles Keepax wrote:
> > > On Wed, Oct 25, 2017 at 03:34:16PM +1030, Andrew Jeffery wrote:
> > > This means that if we have a set_config we are directly
> > > equating PERSISTENT to RESET_TOLERANT, which seems wrong to
> > > me. I might have a GPIO on a controller with pinconf that
> > > doesn't have anything to do with RESET_TOLERANT. Should the
> > > PIN_CONFIG_RESET_TOLERANT, really just be PIN_CONFIG_PERSISTENT?
> > > And then its upto the driver what persistence means for that
> > > chip?
> > 
> > Right, maybe I'm tying the design in a bit too closely with the Aspeed hardware
> > again. I'm coming out in favour of changing it based on my thoughts below.
> > 
> > However, I want to understand whether there are alternatives to reset tolerance
> > as a property. The obvious one is sleep persistence as implemented in the
> > Arizona driver, but it didn't take the set_config() route with its initial
> > implementation.
> > 
> > The set_config() approach was largely driven by the sysfs patch in
> > the RFC series, because I wanted a way to propagate the desired property
> > to hardware without affecting (or again calling through the handlers for) the
> > export state or direction. It seemed to come out neat enough in general so I
> > kept it.
> > 
> > Were you thinking of using set_config() to control the Arizona's behaviour and
> > do something other than the calls to gpiochip_line_is_persistent() in
> > arizona_gpio_direction_{in,out}() (as in, keep the state inside the device
> > driver rather than querying gpiolib where the state is currently stored)? That
> > would seem neat in that it gives us one method of setting both persistence
> > types (and in this case I should rename the pinconf parameter). With that we
> > could probably get rid of gpiochip_line_is_persistent(). A small downside is
> > some duplicated state (we probably still want to keep FLAG_TRANSITORY in
> > gpiolib).
> > 
> > Would it be reasonable for other drivers to implement sleep persistence in the
> > same manner as the Arizona driver currently does? If that's the case, I don't
> > think there is a third tolerance option in addition to sleep and reset, and so
> > we might not need to rename the pinconf parameter.
> > 
> > Finally, we could implement reset persistence in the same manner as the
> > Arizona's sleep persistence (with gpiochip_line_is_persistent()) given the
> > sysfs patch has been thrown away.
> > 
> > So to summarise, having rambled a bit, I see the situation as:
> > 
> > 1. Rename the pinconf parameter, and patch the Arizona driver to use
> > ÂÂÂset_config() and hold sleep persistence state internally.
> > 2. Drop the pinconf parameter and do something similar to the Arizona's sleep
> > ÂÂÂpersistence for reset persistence in the Aspeed driver.
> > 3. Keep things as proposed are and live with two approaches to persistence
> > 
> > Point 3 seems like the least desirable. I'm leaning towards 1. I could probably
> > live with 2. after experimenting with it and confirming it's workable for me.
> > 
> > If you think 1. is the way to go are you happy for me to send a patch to the
> > Arizona driver implementing the change?
> > 
> 
> I guess the design was more playing to the fact that in the
> Arizona case we arn't really setting any register bit for the
> persisence, we are just not letting the CODEC power down whilst
> the GPIO is in use. The config approach does make more sense in
> the case where you are actually setting register bits.
> 
> I don't have any great objection to keeping the persistence state
> in the Arizona GPIO driver, but it does seem to be duplicating
> the state a little. Also an early version of the patch chain did
> that and we decided it would better in the core.
> 
> I am not opposed to option 3 either. For example open-drain has
> a similar setup where there is a pin config option for it and
> also a GPIO function call to check for its presence on a specific
> GPIO. Certainly my objection was just that we were equating the
> very generic property to a very specific pinconf.
> 
> I think I would probably be keen to see what Linus's thoughts
> are, but my feelings are really your patch is probably good if we
> just rename the RESET_TOLERANT pinconf to something a little more
> generic.

Okay, well in that case I'll do the rename and send out the patches, it
is simple enough. Thanks again for your feedback.

Andrew

Attachment: signature.asc
Description: This is a digitally signed message part