[v3,3/5] dt-bindings: input: Add poll-interval property
diff mbox series

Message ID 1570083176-8231-4-git-send-email-michal.vokac@ysoft.com
State Under Review
Headers show
Series
  • Add polling mode to the MPR121 touchkey
Related show

Commit Message

Michal Vokáč Oct. 3, 2019, 6:12 a.m. UTC
Add an option to periodicaly poll the device to get state of the inputs
as the interrupt line may not be used on some platforms.

Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
---
Changes since v2:
 - None

Changes since v1:
 - Use poll-interval instead of linux,poll-interval.
 - Place the poll-interval binding into the common schema.
 - Properly describe that either interrupts or poll-interval property is
   required.
 - Fix the example to pass validation.

 .../bindings/input/fsl,mpr121-touchkey.yaml        | 25 +++++++++++++++++++++-
 Documentation/devicetree/bindings/input/input.yaml |  4 ++++
 2 files changed, 28 insertions(+), 1 deletion(-)

Comments

Rob Herring Oct. 10, 2019, 7:40 p.m. UTC | #1
On Thu, Oct 03, 2019 at 08:12:54AM +0200, Michal Vokáč wrote:
> Add an option to periodicaly poll the device to get state of the inputs
> as the interrupt line may not be used on some platforms.
> 
> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> ---
> Changes since v2:
>  - None
> 
> Changes since v1:
>  - Use poll-interval instead of linux,poll-interval.
>  - Place the poll-interval binding into the common schema.
>  - Properly describe that either interrupts or poll-interval property is
>    required.
>  - Fix the example to pass validation.
> 
>  .../bindings/input/fsl,mpr121-touchkey.yaml        | 25 +++++++++++++++++++++-
>  Documentation/devicetree/bindings/input/input.yaml |  4 ++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
> index c6fbcdf78556..035b2fee4491 100644
> --- a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
> +++ b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
> @@ -17,6 +17,10 @@ description: |
>  allOf:
>    - $ref: input.yaml#
>  
> +oneOf:

It should be valid to have both properties present, right? The h/w 
description can't know what the OS supports. In that case, we should use 
'anyOf' here instead.

With that,

Reviewed-by: Rob Herring <robh@kernel.org>

> +  - required: [ interrupts ]
> +  - required: [ poll-interval ]
> +
>  properties:
>    compatible:
>      const: fsl,mpr121-touchkey
> @@ -41,12 +45,12 @@ properties:
>  required:
>    - compatible
>    - reg
> -  - interrupts
>    - vdd-supply
>    - linux,keycodes
>  
>  examples:
>    - |
> +    // Example with interrupts
>      #include "dt-bindings/input/input.h"
>      i2c {
>          #address-cells = <1>;
> @@ -64,3 +68,22 @@ examples:
>                               <KEY_8>, <KEY_9>, <KEY_A>, <KEY_B>;
>          };
>      };
> +
> +  - |
> +    // Example with polling
> +    #include "dt-bindings/input/input.h"
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        mpr121@5a {
> +            compatible = "fsl,mpr121-touchkey";
> +            reg = <0x5a>;
> +            poll-interval = <20>;
> +            autorepeat;
> +            vdd-supply = <&ldo4_reg>;
> +            linux,keycodes = <KEY_0>, <KEY_1>, <KEY_2>, <KEY_3>,
> +                             <KEY_4>, <KEY_5>, <KEY_6>, <KEY_7>,
> +                             <KEY_8>, <KEY_9>, <KEY_A>, <KEY_B>;
> +        };
> +    };
> diff --git a/Documentation/devicetree/bindings/input/input.yaml b/Documentation/devicetree/bindings/input/input.yaml
> index ca8fe84a2e62..6d519046b3af 100644
> --- a/Documentation/devicetree/bindings/input/input.yaml
> +++ b/Documentation/devicetree/bindings/input/input.yaml
> @@ -24,6 +24,10 @@ properties:
>            minimum: 0
>            maximum: 0xff
>  
> +  poll-interval:
> +    description: Poll interval time in milliseconds.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
>    power-off-time-sec:
>      description:
>        Duration in seconds which the key should be kept pressed for device to
> -- 
> 2.1.4
>
Dmitry Torokhov Oct. 10, 2019, 8:01 p.m. UTC | #2
On Thu, Oct 10, 2019 at 02:40:36PM -0500, Rob Herring wrote:
> On Thu, Oct 03, 2019 at 08:12:54AM +0200, Michal Vokáč wrote:
> > Add an option to periodicaly poll the device to get state of the inputs
> > as the interrupt line may not be used on some platforms.
> > 
> > Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> > ---
> > Changes since v2:
> >  - None
> > 
> > Changes since v1:
> >  - Use poll-interval instead of linux,poll-interval.
> >  - Place the poll-interval binding into the common schema.
> >  - Properly describe that either interrupts or poll-interval property is
> >    required.
> >  - Fix the example to pass validation.
> > 
> >  .../bindings/input/fsl,mpr121-touchkey.yaml        | 25 +++++++++++++++++++++-
> >  Documentation/devicetree/bindings/input/input.yaml |  4 ++++
> >  2 files changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
> > index c6fbcdf78556..035b2fee4491 100644
> > --- a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
> > +++ b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
> > @@ -17,6 +17,10 @@ description: |
> >  allOf:
> >    - $ref: input.yaml#
> >  
> > +oneOf:
> 
> It should be valid to have both properties present, right?

The poll does not really sense and does not have any effect when
interrupt is supplied.

> The h/w description can't know what the OS supports.

It also has no idea what OS does at all and whether it even pays
attention to any of these properties. We are just trying to say here "I
do not have an interrupt wired, so for this device's primary use case
(that is coupled with a certain $PRIMARY OS) we need to poll the
controller ever so often to handle our use case".

> In that case, we should use 'anyOf' here instead.
> 
> With that,
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> > +  - required: [ interrupts ]
> > +  - required: [ poll-interval ]
> > +
> >  properties:
> >    compatible:
> >      const: fsl,mpr121-touchkey
> > @@ -41,12 +45,12 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - interrupts
> >    - vdd-supply
> >    - linux,keycodes
> >  
> >  examples:
> >    - |
> > +    // Example with interrupts
> >      #include "dt-bindings/input/input.h"
> >      i2c {
> >          #address-cells = <1>;
> > @@ -64,3 +68,22 @@ examples:
> >                               <KEY_8>, <KEY_9>, <KEY_A>, <KEY_B>;
> >          };
> >      };
> > +
> > +  - |
> > +    // Example with polling
> > +    #include "dt-bindings/input/input.h"
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        mpr121@5a {
> > +            compatible = "fsl,mpr121-touchkey";
> > +            reg = <0x5a>;
> > +            poll-interval = <20>;
> > +            autorepeat;
> > +            vdd-supply = <&ldo4_reg>;
> > +            linux,keycodes = <KEY_0>, <KEY_1>, <KEY_2>, <KEY_3>,
> > +                             <KEY_4>, <KEY_5>, <KEY_6>, <KEY_7>,
> > +                             <KEY_8>, <KEY_9>, <KEY_A>, <KEY_B>;
> > +        };
> > +    };
> > diff --git a/Documentation/devicetree/bindings/input/input.yaml b/Documentation/devicetree/bindings/input/input.yaml
> > index ca8fe84a2e62..6d519046b3af 100644
> > --- a/Documentation/devicetree/bindings/input/input.yaml
> > +++ b/Documentation/devicetree/bindings/input/input.yaml
> > @@ -24,6 +24,10 @@ properties:
> >            minimum: 0
> >            maximum: 0xff
> >  
> > +  poll-interval:
> > +    description: Poll interval time in milliseconds.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> >    power-off-time-sec:
> >      description:
> >        Duration in seconds which the key should be kept pressed for device to
> > -- 
> > 2.1.4
> >
Michal Vokáč Oct. 11, 2019, 8:03 a.m. UTC | #3
On 10. 10. 19 22:01, Dmitry Torokhov wrote:
> On Thu, Oct 10, 2019 at 02:40:36PM -0500, Rob Herring wrote:
>> On Thu, Oct 03, 2019 at 08:12:54AM +0200, Michal Vokáč wrote:
>>> Add an option to periodicaly poll the device to get state of the inputs
>>> as the interrupt line may not be used on some platforms.
>>>
>>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
>>> ---
>>> Changes since v2:
>>>   - None
>>>
>>> Changes since v1:
>>>   - Use poll-interval instead of linux,poll-interval.
>>>   - Place the poll-interval binding into the common schema.
>>>   - Properly describe that either interrupts or poll-interval property is
>>>     required.
>>>   - Fix the example to pass validation.
>>>
>>>   .../bindings/input/fsl,mpr121-touchkey.yaml        | 25 +++++++++++++++++++++-
>>>   Documentation/devicetree/bindings/input/input.yaml |  4 ++++
>>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
>>> index c6fbcdf78556..035b2fee4491 100644
>>> --- a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
>>> +++ b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
>>> @@ -17,6 +17,10 @@ description: |
>>>   allOf:
>>>     - $ref: input.yaml#
>>>   
>>> +oneOf:
>>
>> It should be valid to have both properties present, right?
> 
> The poll does not really sense and does not have any effect when
> interrupt is supplied.

 From technical point of view, yes it is possible to have both
properties. But I agree that it does not really make sense to
use both at the same time.

>> The h/w description can't know what the OS supports.
> 
> It also has no idea what OS does at all and whether it even pays
> attention to any of these properties. We are just trying to say here "I
> do not have an interrupt wired, so for this device's primary use case
> (that is coupled with a certain $PRIMARY OS) we need to poll the
> controller ever so often to handle our use case".

If I understand correctly the relationship between Linux and DT
binding, in Linux we are free to implement just part of all the
possible configuration options described by the binding.

In this case if somebody would enable both interrupt and polling,
we will happily use the interrupt mode only. Maybe it would be nice
to at least print a message that the poll-intervall is ignored?

>> In that case, we should use 'anyOf' here instead.

What I am afraid of is that some DT writers may really use both
properties and expect that Linux will actually do something useful
in this case. Anyway, I am OK with that.

Michal

>> With that,
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>>
>>> +  - required: [ interrupts ]
>>> +  - required: [ poll-interval ]
>>> +
>>>   properties:
>>>     compatible:
>>>       const: fsl,mpr121-touchkey
>>> @@ -41,12 +45,12 @@ properties:
>>>   required:
>>>     - compatible
>>>     - reg
>>> -  - interrupts
>>>     - vdd-supply
>>>     - linux,keycodes
>>>   
>>>   examples:
>>>     - |
>>> +    // Example with interrupts
>>>       #include "dt-bindings/input/input.h"
>>>       i2c {
>>>           #address-cells = <1>;
>>> @@ -64,3 +68,22 @@ examples:
>>>                                <KEY_8>, <KEY_9>, <KEY_A>, <KEY_B>;
>>>           };
>>>       };
>>> +
>>> +  - |
>>> +    // Example with polling
>>> +    #include "dt-bindings/input/input.h"
>>> +    i2c {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        mpr121@5a {
>>> +            compatible = "fsl,mpr121-touchkey";
>>> +            reg = <0x5a>;
>>> +            poll-interval = <20>;
>>> +            autorepeat;
>>> +            vdd-supply = <&ldo4_reg>;
>>> +            linux,keycodes = <KEY_0>, <KEY_1>, <KEY_2>, <KEY_3>,
>>> +                             <KEY_4>, <KEY_5>, <KEY_6>, <KEY_7>,
>>> +                             <KEY_8>, <KEY_9>, <KEY_A>, <KEY_B>;
>>> +        };
>>> +    };
>>> diff --git a/Documentation/devicetree/bindings/input/input.yaml b/Documentation/devicetree/bindings/input/input.yaml
>>> index ca8fe84a2e62..6d519046b3af 100644
>>> --- a/Documentation/devicetree/bindings/input/input.yaml
>>> +++ b/Documentation/devicetree/bindings/input/input.yaml
>>> @@ -24,6 +24,10 @@ properties:
>>>             minimum: 0
>>>             maximum: 0xff
>>>   
>>> +  poll-interval:
>>> +    description: Poll interval time in milliseconds.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +
>>>     power-off-time-sec:
>>>       description:
>>>         Duration in seconds which the key should be kept pressed for device to
>>> -- 
>>> 2.1.4
>>>
>
Dmitry Torokhov Oct. 16, 2019, 12:23 a.m. UTC | #4
On Fri, Oct 11, 2019 at 10:03:25AM +0200, Michal Vokáč wrote:
> On 10. 10. 19 22:01, Dmitry Torokhov wrote:
> > On Thu, Oct 10, 2019 at 02:40:36PM -0500, Rob Herring wrote:
> > > On Thu, Oct 03, 2019 at 08:12:54AM +0200, Michal Vokáč wrote:
> > > > Add an option to periodicaly poll the device to get state of the inputs
> > > > as the interrupt line may not be used on some platforms.
> > > > 
> > > > Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
> > > > ---
> > > > Changes since v2:
> > > >   - None
> > > > 
> > > > Changes since v1:
> > > >   - Use poll-interval instead of linux,poll-interval.
> > > >   - Place the poll-interval binding into the common schema.
> > > >   - Properly describe that either interrupts or poll-interval property is
> > > >     required.
> > > >   - Fix the example to pass validation.
> > > > 
> > > >   .../bindings/input/fsl,mpr121-touchkey.yaml        | 25 +++++++++++++++++++++-
> > > >   Documentation/devicetree/bindings/input/input.yaml |  4 ++++
> > > >   2 files changed, 28 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
> > > > index c6fbcdf78556..035b2fee4491 100644
> > > > --- a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
> > > > +++ b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
> > > > @@ -17,6 +17,10 @@ description: |
> > > >   allOf:
> > > >     - $ref: input.yaml#
> > > > +oneOf:
> > > 
> > > It should be valid to have both properties present, right?
> > 
> > The poll does not really sense and does not have any effect when
> > interrupt is supplied.
> 
> From technical point of view, yes it is possible to have both
> properties. But I agree that it does not really make sense to
> use both at the same time.
> 
> > > The h/w description can't know what the OS supports.
> > 
> > It also has no idea what OS does at all and whether it even pays
> > attention to any of these properties. We are just trying to say here "I
> > do not have an interrupt wired, so for this device's primary use case
> > (that is coupled with a certain $PRIMARY OS) we need to poll the
> > controller ever so often to handle our use case".
> 
> If I understand correctly the relationship between Linux and DT
> binding, in Linux we are free to implement just part of all the
> possible configuration options described by the binding.
> 
> In this case if somebody would enable both interrupt and polling,
> we will happily use the interrupt mode only. Maybe it would be nice
> to at least print a message that the poll-intervall is ignored?
> 
> > > In that case, we should use 'anyOf' here instead.
> 
> What I am afraid of is that some DT writers may really use both
> properties and expect that Linux will actually do something useful
> in this case. Anyway, I am OK with that.

OK, I changed it to "anyOf", folded into driver change and applied.
Michal Vokáč Oct. 16, 2019, 5:52 a.m. UTC | #5
On 16. 10. 19 2:23, Dmitry Torokhov wrote:
> On Fri, Oct 11, 2019 at 10:03:25AM +0200, Michal Vokáč wrote:
>> On 10. 10. 19 22:01, Dmitry Torokhov wrote:
>>> On Thu, Oct 10, 2019 at 02:40:36PM -0500, Rob Herring wrote:
>>>> On Thu, Oct 03, 2019 at 08:12:54AM +0200, Michal Vokáč wrote:
>>>>> Add an option to periodicaly poll the device to get state of the inputs
>>>>> as the interrupt line may not be used on some platforms.
>>>>>
>>>>> Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com>
>>>>> ---
>>>>> Changes since v2:
>>>>>    - None
>>>>>
>>>>> Changes since v1:
>>>>>    - Use poll-interval instead of linux,poll-interval.
>>>>>    - Place the poll-interval binding into the common schema.
>>>>>    - Properly describe that either interrupts or poll-interval property is
>>>>>      required.
>>>>>    - Fix the example to pass validation.
>>>>>
>>>>>    .../bindings/input/fsl,mpr121-touchkey.yaml        | 25 +++++++++++++++++++++-
>>>>>    Documentation/devicetree/bindings/input/input.yaml |  4 ++++
>>>>>    2 files changed, 28 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
>>>>> index c6fbcdf78556..035b2fee4491 100644
>>>>> --- a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
>>>>> +++ b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
>>>>> @@ -17,6 +17,10 @@ description: |
>>>>>    allOf:
>>>>>      - $ref: input.yaml#
>>>>> +oneOf:
>>>>
>>>> It should be valid to have both properties present, right?
>>>
>>> The poll does not really sense and does not have any effect when
>>> interrupt is supplied.
>>
>>  From technical point of view, yes it is possible to have both
>> properties. But I agree that it does not really make sense to
>> use both at the same time.
>>
>>>> The h/w description can't know what the OS supports.
>>>
>>> It also has no idea what OS does at all and whether it even pays
>>> attention to any of these properties. We are just trying to say here "I
>>> do not have an interrupt wired, so for this device's primary use case
>>> (that is coupled with a certain $PRIMARY OS) we need to poll the
>>> controller ever so often to handle our use case".
>>
>> If I understand correctly the relationship between Linux and DT
>> binding, in Linux we are free to implement just part of all the
>> possible configuration options described by the binding.
>>
>> In this case if somebody would enable both interrupt and polling,
>> we will happily use the interrupt mode only. Maybe it would be nice
>> to at least print a message that the poll-intervall is ignored?
>>
>>>> In that case, we should use 'anyOf' here instead.
>>
>> What I am afraid of is that some DT writers may really use both
>> properties and expect that Linux will actually do something useful
>> in this case. Anyway, I am OK with that.
> 
> OK, I changed it to "anyOf", folded into driver change and applied.

AFAIK this is discouraged and DT binding changes and driver changes
should ideally be kept as separate patches. Separating the binding
from driver changes makes is easier for other projects to reuse
the binding.

The other thing is, Rob reviewed just the binding part, not the driver
change, but now his tag covers both changes.

Michal

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
index c6fbcdf78556..035b2fee4491 100644
--- a/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
+++ b/Documentation/devicetree/bindings/input/fsl,mpr121-touchkey.yaml
@@ -17,6 +17,10 @@  description: |
 allOf:
   - $ref: input.yaml#
 
+oneOf:
+  - required: [ interrupts ]
+  - required: [ poll-interval ]
+
 properties:
   compatible:
     const: fsl,mpr121-touchkey
@@ -41,12 +45,12 @@  properties:
 required:
   - compatible
   - reg
-  - interrupts
   - vdd-supply
   - linux,keycodes
 
 examples:
   - |
+    // Example with interrupts
     #include "dt-bindings/input/input.h"
     i2c {
         #address-cells = <1>;
@@ -64,3 +68,22 @@  examples:
                              <KEY_8>, <KEY_9>, <KEY_A>, <KEY_B>;
         };
     };
+
+  - |
+    // Example with polling
+    #include "dt-bindings/input/input.h"
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        mpr121@5a {
+            compatible = "fsl,mpr121-touchkey";
+            reg = <0x5a>;
+            poll-interval = <20>;
+            autorepeat;
+            vdd-supply = <&ldo4_reg>;
+            linux,keycodes = <KEY_0>, <KEY_1>, <KEY_2>, <KEY_3>,
+                             <KEY_4>, <KEY_5>, <KEY_6>, <KEY_7>,
+                             <KEY_8>, <KEY_9>, <KEY_A>, <KEY_B>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/input/input.yaml b/Documentation/devicetree/bindings/input/input.yaml
index ca8fe84a2e62..6d519046b3af 100644
--- a/Documentation/devicetree/bindings/input/input.yaml
+++ b/Documentation/devicetree/bindings/input/input.yaml
@@ -24,6 +24,10 @@  properties:
           minimum: 0
           maximum: 0xff
 
+  poll-interval:
+    description: Poll interval time in milliseconds.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
   power-off-time-sec:
     description:
       Duration in seconds which the key should be kept pressed for device to