Re: [PATCH] rotary encoder: Add wake up support

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

Hello Johan,

On Thu, Jan 29, 2015 at 11:49:53AM +0100, Johan Hovold wrote:
> On Tue, Jan 13, 2015 at 04:51:02PM +0100, Sylvain Rochet wrote:
> > This patch add wake up support for rotary encoders.
> > New DT property: rotary-encoder,wakeup
> > 
> > Signed-off-by: Sylvain Rochet <sylvain.rochet@finsecur.com>
> > ---
> >  .../devicetree/bindings/input/rotary-encoder.txt   |  1 +
> >  Documentation/input/rotary-encoder.txt             |  1 +
> >  drivers/input/misc/rotary_encoder.c                | 36 ++++++++++++++++++++++
> >  include/linux/rotary_encoder.h                     |  1 +
> >  4 files changed, 39 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> > index 3315495..468d545 100644
> > --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
> > +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> > @@ -15,6 +15,7 @@ Optional properties:
> >  - rotary-encoder,rollover: Automatic rollove when the rotary value becomes
> >    greater than the specified steps or smaller than 0. For absolute axis only.
> >  - rotary-encoder,half-period: Makes the driver work on half-period mode.
> > +- rotary-encoder,wakeup: Boolean, rotary encoder can wake-up the system.
> 
> As Dmitry already mentioned this should probably just be
> "wakeup-source".

Yeah, I think so.


> > +static SIMPLE_DEV_PM_OPS(rotary_encoder_pm_ops, rotary_encoder_suspend, rotary_encoder_resume);
> 
> Please break this line to stay within 80 cols.

I saw that afterward, I waited the answer about DT property to propose a 
v2. Fixed.


> > +	int wakeup;			/* configure the rotary-encoder as a wake-up source */
> 
> This should be bool.

Ditto. Fixed.


> I also suggest you rename the variable wakeup_source and drop the
> verbose comment.

I agree, done !


> Looks good otherwise.

Thank for the review, v2 sent :)


Sylvain