Re: [PATCH v2 1/2] dt-bindings: leds: gpio: Add optional retain-state-shutdown property

[Andrew Jeffery <andrew@aj.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2891 bytes --]

Hi Rob,

On Thu, 2017-08-31 at 16:28 -0500, Rob Herring wrote:
> On Mon, Aug 28, 2017 at 09:47:10AM +0930, Andrew Jeffery wrote:
> > On Baseboard Management Controller (BMC) systems it's sometimes
> > necessary for a LED to retain its state across a BMC reset (which is
> > independent of the host system state). Add a devicetree property to
> > describe this behaviour. The property would typically be used in
> > conjunction with 'default-state = "keep"'.
> 
> A bit quick on the applying of this...
> 
> I don't understand how this works. The BMC usecase is interesting but 
> that's fairly irrelevant to the binding. How do you know the GPIO state 
> thru a reset?

The answer to that is a property of the system design in my opinion.

> You're doing a core reset, but not reseting the GPIO 
> controller?

Well, in my specific case, the GPIO controller in question isn't on the SoC,
it's off-chip behind an I2C bus. Powering off a BMC doesn't necessarily mean
removing power from its peripherals, so in this case the their state is
maintained.

Alternatively, and again in my specific (Aspeed) case, the on-SoC GPIO
controller can be selectively reset with respect to the rest of the SoC, or
individual GPIO lines can be marked as reset-tolerant. This gives the same
capability and is what you've suggested above.

> 
> When you use 'default-state = "keep"' but not this property? Seems like  the
> same thing to me.

Is that not also true of retain-state-suspend?

The existing behaviour of leds-gpio was to turn the managed LEDs off in the
remove() path. To me this was unexpected behaviour, but it is what it is. My
first pass at the patch (before I sent it out) was to do as you suggest and
only turn it off in the absence of 'default-state = "keep"'.  However there
were quite a number of users of that configuration, and this could be an
unexpected change of behaviour. Given retain-state-suspend was already defined,
it seemed that the non-breaking way to get the behaviour I desired was to
introduce retain-state-shutdown.

Sure, this is dealing with problems in the driver and not describing hardware,
but at this point the behaviour is already in place and people might be relying
on it.

> Presumably the bootloader would just distinguish  between a
> warm and cold reset to decide whether to re-init the state.
> 
> Finally, if we do have this property, why is it GPIO specific. You could 
> just as easily have a LED controller IC and want to do the same thing.

I don't think we're precluded from making it more general even if it's
submitted as specific to leds-gpio? I don't have anything against making it
more generic. I was tossing up about doing so, but went for the specific case
first because we could make it more generic without problems. Going the other
way is harder.

Cheers,

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]