[1/2] dt-bindings: iio: proximity: Add bindings for Murata IRS-D200

  Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
for communication with interrupt support. Add devicetree bindings
requiring the compatible string, I2C slave address (reg) and interrupts.

Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
---
 .../iio/proximity/murata,irsd200.yaml         | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
  

On 16/06/2023 17:10, Waqar Hameed wrote:
> Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
> for communication with interrupt support. Add devicetree bindings
> requiring the compatible string, I2C slave address (reg) and interrupts.

Thank you for your patch. There is something to discuss/improve. I have
actually only remark about DTS example, but since I expect resend two
more nits as well.


A nit, subject: drop second/last, redundant "bindings for". The
"dt-bindings" prefix is already stating that these are bindings.

> 
> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
> ---
>  .../iio/proximity/murata,irsd200.yaml         | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
> new file mode 100644
> index 000000000000..d317fbe7bd50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/murata,irsd200.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Murata IRS-D200 PIR sensor
> +
> +maintainers:
> +  - Waqar Hameed <waqar.hameed@axis.com>
> +
> +description: |

Nit, do not need '|' unless you need to preserve formatting.

> +  PIR sensor for human detection.
> +
> +properties:
> +  compatible:
> +    const: murata,irsd200
> +
> +  reg:
> +    items:
> +      - enum:
> +          - 0x48
> +          - 0x49
> +        description: |
> +          When the AD pin is connected to GND, the slave address is 0x48.
> +          When the AD pin is connected to VDD, the slave address is 0x49.
> +
> +  interrupts:
> +    maxItems: 1
> +    description:
> +      Type should be IRQ_TYPE_EDGE_RISING.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        pir@48 {

Node names should be generic. See also explanation and list of examples
in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

so I guess: proximity@?


Best regards,
Krzysztof
  

On Fri, 16 Jun 2023 17:10:42 +0200
Waqar Hameed <waqar.hameed@axis.com> wrote:

> Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
> for communication with interrupt support. Add devicetree bindings
> requiring the compatible string, I2C slave address (reg) and interrupts.
> 
> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>

This device will have some power supplies, so I'd expect those to be both
listed and marked as required (maybe some are optional?)

Other than that and the points in the other review one thing inline about interrupts.

Jonathan

> ---
>  .../iio/proximity/murata,irsd200.yaml         | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
> new file mode 100644
> index 000000000000..d317fbe7bd50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/murata,irsd200.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/murata,irsd200.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Murata IRS-D200 PIR sensor
> +
> +maintainers:
> +  - Waqar Hameed <waqar.hameed@axis.com>
> +
> +description: |
> +  PIR sensor for human detection.
> +
> +properties:
> +  compatible:
> +    const: murata,irsd200
> +
> +  reg:
> +    items:
> +      - enum:
> +          - 0x48
> +          - 0x49
> +        description: |
> +          When the AD pin is connected to GND, the slave address is 0x48.
> +          When the AD pin is connected to VDD, the slave address is 0x49.
> +
> +  interrupts:
> +    maxItems: 1
> +    description:
> +      Type should be IRQ_TYPE_EDGE_RISING.
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts

If it is possible to remove interrupts from requires  - and hence have
at least a partly functional driver doing basic reading of the sensor
then that is usually a good idea.   Far too many board designers seem
to decide that they don't need to wire up interrupt lines.

If it's really hard then don't worry too much.

Jonathan

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        pir@48 {
> +            compatible = "murata,irsd200";
> +            reg = <0x48>;
> +            interrupts = <24 IRQ_TYPE_EDGE_RISING>;
> +        };
> +    };
> +...
  

On Sat, Jun 17, 2023 at 10:55 +0200 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 16/06/2023 17:10, Waqar Hameed wrote:
>> Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
>> for communication with interrupt support. Add devicetree bindings
>> requiring the compatible string, I2C slave address (reg) and interrupts.
>
> Thank you for your patch. There is something to discuss/improve. I have
> actually only remark about DTS example, but since I expect resend two
> more nits as well.
>
>
> A nit, subject: drop second/last, redundant "bindings for". The
> "dt-bindings" prefix is already stating that these are bindings.

Alright, will change to "dt-bindings: iio: proximity: Add Murata
IRS-D200".

[...]

>> +maintainers:
>> +  - Waqar Hameed <waqar.hameed@axis.com>
>> +
>> +description: |
>
> Nit, do not need '|' unless you need to preserve formatting.

Yes, will remove (a remnant from previous version that had multiple
lines... Sorry!).

[...]

>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        pir@48 {
>
> Node names should be generic. See also explanation and list of examples
> in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
> so I guess: proximity@?

On that list there is "temperature-sensor". Would it make sense then to
call it "proximity-sensor"?
  

On Sat, Jun 17, 2023 at 13:55 +0100 Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 16 Jun 2023 17:10:42 +0200
> Waqar Hameed <waqar.hameed@axis.com> wrote:
>
>> Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
>> for communication with interrupt support. Add devicetree bindings
>> requiring the compatible string, I2C slave address (reg) and interrupts.
>> 
>> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>
>
> This device will have some power supplies, so I'd expect those to be both
> listed and marked as required (maybe some are optional?)

Right, will add that here (and call `devm_regulator_get_enable()` in
driver's probe).

[...]

>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>
> If it is possible to remove interrupts from requires  - and hence have
> at least a partly functional driver doing basic reading of the sensor
> then that is usually a good idea.   Far too many board designers seem
> to decide that they don't need to wire up interrupt lines.
>
> If it's really hard then don't worry too much.

I see. It would be possible, but would also require some work. Let's
leave it for now then?
  

On 19/06/2023 12:40, Waqar Hameed wrote:
>> Node names should be generic. See also explanation and list of examples
>> in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>
>> so I guess: proximity@?
> 
> On that list there is "temperature-sensor". Would it make sense then to
> call it "proximity-sensor"?

Choose whatever is the most popular in existing Linux sources.

Best regards,
Krzysztof
  

On Mon, 19 Jun 2023 12:41:06 +0200
Waqar Hameed <waqar.hameed@axis.com> wrote:

> On Sat, Jun 17, 2023 at 13:55 +0100 Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri, 16 Jun 2023 17:10:42 +0200
> > Waqar Hameed <waqar.hameed@axis.com> wrote:
> >  
> >> Murata IRS-D200 is a PIR sensor for human detection. It uses the I2C bus
> >> for communication with interrupt support. Add devicetree bindings
> >> requiring the compatible string, I2C slave address (reg) and interrupts.
> >> 
> >> Signed-off-by: Waqar Hameed <waqar.hameed@axis.com>  
> >
> > This device will have some power supplies, so I'd expect those to be both
> > listed and marked as required (maybe some are optional?)  
> 
> Right, will add that here (and call `devm_regulator_get_enable()` in
> driver's probe).
> 
> [...]
> 
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - interrupts  
> >
> > If it is possible to remove interrupts from requires  - and hence have
> > at least a partly functional driver doing basic reading of the sensor
> > then that is usually a good idea.   Far too many board designers seem
> > to decide that they don't need to wire up interrupt liness
> > If it's really hard then don't worry too much.  
> 
> I see. It would be possible, but would also require some work. Let's
> leave it for now then?

Sure - as long as you review the patches when they come in :)

Jonathan