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

Stephen Warren swarren at wwwdotorg.org
Tue Nov 20 16:17:24 EST 2012


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.



More information about the linux-arm-kernel mailing list