Re: [PATCH v2 2/2] input: rotary-encoder: Add 'on-each-step' tobinding documentation

[Ezequiel García]

On 4 October 2013 11:09, Ezequiel Garcia
<ezequiel.garcia@xxxxxxxxxxxxxxxxxx> wrote:
> On Fri, Oct 04, 2013 at 02:19:56PM +0100, Mark Rutland wrote:
>> On Fri, Oct 04, 2013 at 01:53:23PM +0100, Ezequiel Garcia wrote:
>> > The driver now supports a new mode to handle the interruptions generated
>> > by the device: on this new mode an input event is generated on each step
>> > (i.e. on each IRQ). Therefore, add a new DT property, to select the
>> > mode: 'rotary-encoder,on-each-step'.
>> >
>> > Cc: Daniel Mack <zonque@xxxxxxxxx>
>> > Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
>> > Cc: Rob Herring <rob.herring@xxxxxxxxxxx>
>> > Cc: devicetree@xxxxxxxxxxxxxxx
>> > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>
>> > ---
>> > I'm not at all happy with this DT binding as it's way to customized
>> > for the current driver. For instance, if we want to support mapping
>> > key events (or better arbitrary linux-input event types) it seems
>> > there's no easy way to fix the binding.
>> >
>> > Maybe a better way of handling the different 'modes' is through
>> > compatible strings?
>>
>> I'd prefer not to have more pseudo-devices in DT, and would prefer not
>> to have compatible strings that boil down to driver options. We end up
>> just embedding a tonne of Linux-specific driver configuration in the DT
>> rather than describing hardware.
>>
>> That said, I'm not sure what the best solution is here.
>>
>> >
>> > I'm not really sure, so I hope the DT guys have some comment on this.
>> >
>> >  Documentation/devicetree/bindings/input/rotary-encoder.txt | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
>> > index 3315495..b89e38d 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,on-each-step: Makes the driver send an event on each step.
>>
>> Could this not be something requested at runtime?
>>
>
> Sure. The different modes:
>
> * default (no option)
> * rotary-encoder,half-period
> * rotary-encoder,on-each-step
>
> Just map to different interruption handlers. I don't have any other
> rotary-encoder device, so I'm not at all sure what's the use of the
> other two cases.
> My particular device is detented, and produces a 'stable' event on each
> step (i.e on each IRQ).
>
> Regarding the runtime specification: you mean as a module parameter?
> That should be trivial to add, no?
>
>> Could you explain what you want to achieve with this? -- what events do
>> you want to occur when, to be handled in what way?
>>
>
> Hm.. maybe I should have added the binding to the 1/2 patch and CCed
> everybody involved for better context.
>
> Anyway, I hope the above is clearer, I'm not really sure how to specify
> the details in the DT binding, since it's a specific interruption handler
> for this class of encoder devices (stable on each step).
>
> That said, I really hope I'm crafting a generic solution and not some
> tailor-made implementation that just happens to match my use case.
>
> The input maintainer's opinion on this would be valuable.

Any insights on this binding?

If nobody objects, then I'd like to get this change accepted,
together with the driver change.

Thanks!
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/