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

On 11/20/2012 01:31 PM, Andrew Lunn wrote:

On Tue, Nov 20, 2012 at 10:11:18AM -0700, Stephen Warren wrote:

quoted

On 11/20/2012 01:37 AM, Andrew Lunn wrote:

quoted

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?

quoted

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.