Re: [PATCH v3 1/3] power: Add simple poweroff-gpio driver

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 11/20/2012 01:31 PM, Andrew Lunn wrote:
> On Tue, Nov 20, 2012 at 10:11:18AM -0700, Stephen Warren wrote:
>> On 11/20/2012 01:37 AM, Andrew Lunn wrote:
>>> Hi Jason
>>>
>>> These are good comments from Stephan that i want to address. However,
>>> i also don't want to delay the pull-requests direction arm-soc, the
>>> merge window is getting close. Both Linus and Anton have Acked the
>>> current version, so please go with what you have and i will produce a
>>> patch over the top. If its available before Arnd pulls, you can squash
>>> it, otherwise send it upstream as a standalone patch.
>>
>> I'm not sure I agree here; the comments I made re: the delays and
>> pulse-vs-level may affect the definition of the DT binding, and that's
>> something that should be correct from the start.
> 
> Hi Stephen
> 
> There should be no need to change the binding is a none-backwards
> compatible way. The current delays work for all current users.

Well, the binding isn't written for current users, the description is
just a generic "GPIO to turn off the system".

> If some
> other user comes along which needs longer delays, they can add an
> optional property which specified a longer delay.

The DT maintainers have in the past expressed a wish to define DT
bindings completely, rather than incrementally adding stuff if at all
possible. I guess they weren't CC'd on this patch; I've added them here
for comment. I'll defer to whatever they think here.

> The pulse-vs-level is needed for the potential PXA use case, which is
> where i took this code from. When we first proposed this driver,
> Russel King pointed to the PXA reset.c code and requested we create
> something which PXA could use. The configuring the GPIO as an input
> also come from PXA, which the current two uses user don't need.
> 
> I agree it need better documentation, but the current code covers all
> bases. It will turn off a level based system, it will also turn off an
> edge based system. There is no need to specify in the DT which is
> needed, it will just work.

I'm not convinced the pulse logic can be correct for arbitrary users who
expect a level-sensitive GPIO; what if the MPU (or whatever the GPIO is
connected to) is confused by the pulse and fails to power down when it
sees a pulse (e.g. as a safety measure), but would have worked fine with
just a regular level?

>> The implementation of gpio_poweroff_do_poweroff() really doesn't seem to
>> make sense; related to the above.
>>
>> Also, probe deferral doesn't work, which will likely make this code
>> completely ineffective.
> 
> This code has been tested by the two current users. It works. However,
> i agree about your comment here, and i will fix it in the follow up
> patch.

If this were a specific driver just for certain platforms, I think it
might be OK. Yet the driver purports to be generic. As such, I think we
need to make it explicitly work for the general case, not just happen to
work for the platforms that have been tested so far.