Messages in this thread | | | Date | Wed, 13 Apr 2022 15:38:51 +0200 | Subject | Re: [PATCH v8 4/6] dt-bindings: mfd: sensehat: Add Raspberry Pi Sense HAT schema | From | Krzysztof Kozlowski <> |
| |
On 12/04/2022 22:13, Charles Mirabile wrote: > This patch adds the device tree bindings for the Sense HAT > and each of its children devices in yaml form. >
Thank you for your patch. There is something to discuss/improve.
> +description: > + This device is part of the sensehat multi function device. > + For more information see ../mfd/raspberrypi,sensehat.yaml. > + > + This device features a programmable 8x8 RGB LED matrix. > + > +properties: > + compatible: > + const: raspberrypi,sensehat-display
This binding is practically empty, so I wonder what's is purpose? Do you plan to grow it? If this was already explained, sorry for bringing it up again... :)
> + > +required: > + - compatible > + > +additionalProperties: false > diff --git a/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml b/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml > new file mode 100644 > index 000000000000..c97cd1d8eac6 > --- /dev/null > +++ b/Documentation/devicetree/bindings/input/raspberrypi,sensehat-joystick.yaml > @@ -0,0 +1,33 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/input/raspberrypi,sensehat-joystick.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Raspberry Pi Sensehat Joystick > + > +maintainers: > + - Charles Mirabile <cmirabil@redhat.com> > + - Mwesigwa Guma <mguma@redhat.com> > + - Joel Savitz <jsavitz@redhat.com> > + > +description: > + This device is part of the sensehat multi function device. > + For more information see ../mfd/raspberrypi,sensehat.yaml. > + > + This device features a five button joystick (up, down,left, > + right, click) > + > +properties: > + compatible: > + const: raspberrypi,sensehat-joystick > + > + interrupts: > + items: > + - description: pin number for joystick interrupt
Description is obvious, so just "maxItems: 1"
> + > +required: > + - compatible > + - interrupts > + > +additionalProperties: false > diff --git a/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml b/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml > new file mode 100644 > index 000000000000..2484ec91b430 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/raspberrypi,sensehat.yaml > @@ -0,0 +1,69 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/raspberrypi,sensehat.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Raspberry Pi Sensehat > + > +maintainers: > + - Charles Mirabile <cmirabil@redhat.com> > + - Mwesigwa Guma <mguma@redhat.com> > + - Joel Savitz <jsavitz@redhat.com> > + > +description: > + The Raspberry Pi Sensehat is an addon board originally developed > + for the Raspberry Pi that has a joystick and an 8x8 RGB LED display > + as well as several environmental sensors. It connects via i2c and > + a gpio for irq. > + > +properties: > + compatible: > + const: raspberrypi,sensehat > + > + reg: > + items: > + - description: i2c device address
Description is obvious, so just "maxItems: 1"
> + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0
Why do you need these? You are not allowing any children with unit addresses.
> + > + "joystick": > + $ref: ../input/raspberrypi,sensehat-joystick.yaml
Full path, so "/schemas/input/raspberrypi,sensehat-joystick.yaml#"
> + > + "display": > + $ref: ../auxdisplay/raspberrypi,sensehat-display.yaml
The same.
> + > +required: > + - compatible > + - reg > + - "#address-cells" > + - "#size-cells" > + - joystick > + - display > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + sensehat@46 {
Generic node names please, so "hat".
> + #address-cells = <1>; > + #size-cells = <0>; > + compatible = "raspberrypi,sensehat"; > + reg = <0x46>;
Could you put compatible and then reg at beginning of properties (before address/size)? It is more common convention and these are more important properties.
> + display { > + compatible = "raspberrypi,sensehat-display"; > + }; > + joystick { > + compatible = "raspberrypi,sensehat-joystick"; > + interrupts = <23 GPIO_ACTIVE_HIGH>;
This does not look like proper interrupt flag. Wrong constant.
> + }; > + }; > + };
Best regards, Krzysztof
| |