Re: [PATCH v4 2/2] input: rotary encoder: Add wake up support

[Sylvain Rochet <sylvain.rochet@finsecur.com>]

Hi Johan,


On Tue, Oct 13, 2015 at 02:39:48PM +0200, Johan Hovold wrote:
> On Mon, Oct 12, 2015 at 09:13:32PM +0200, Sylvain Rochet wrote:
> > This patch adds wake up support to GPIO rotary encoders.
> > 
> > Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> > Reviewed-by: Johan Hovold <johan@kernel.org>
> 
> Hmm. I have not yet reviewed the changes you did in v4.

Woops, sorry, I forgot to remove it while rebasing.


> > ---
> >  drivers/input/misc/rotary_encoder.c | 39 +++++++++++++++++++++++++++++++++++++
> >  include/linux/rotary_encoder.h      |  1 +
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
> > index f27f81e..0d86dc4 100644
> > --- a/drivers/input/misc/rotary_encoder.c
> > +++ b/drivers/input/misc/rotary_encoder.c
> 
> > @@ -280,6 +283,10 @@ static int rotary_encoder_probe(struct platform_device *pdev)
> >  		goto exit_free_irq_b;
> >  	}
> >  
> > +	device_set_wakeup_capable(&pdev->dev, true);
> 
> You should continue to use the platform data to determine whether the
> device is capable of wakeup or not.
> 
> > +	if (pdata->wakeup_source)
> > +		device_wakeup_enable(&pdev->dev);
> > +
> 
> Just stick to
> 
> 	device_init_wakeup(&pdev->dev, pdata->wakeup_source);

There is unfortunately no or poor documentation on how "wakeup-source" 
DT property should behave. We have no clue if wake up should be enabled 
by default, which is what device_init_wakeup() is doing. If 
"wakeup-source" is just a flag to determine whether the device is 
capable of wakeup or not then it should probably not change the behavior 
and wake up should probably be off by default, thus the right way would 
be to call device_set_wakeup_capable(&pdev->dev, pdata->wakeup_source);

device_init_wakeup() is a bit confusing, its introductory comment says 
that "By default, most devices should leave wakeup disabled." but it 
actually enables it by default. In this case we are the exception "The 
exceptions are devices that everyone expects to be wakeup sources: 
keyboards, …" but it only adds more confusion, we are the exception, but 
by default we are not.

Anyway, I don't really care and I will resend with device_init_wakeup() 
because wakeup support enabled by default is what I need.

Thank you very much :-)

Cheers,
Sylvain
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html