Received-SPF: pass (google.com: domain of linux-kernel+bounces-91899-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240305035853.916430-1-chris.packham@alliedtelesis.co.nz> <20240305035853.916430-3-chris.packham@alliedtelesis.co.nz>
In-Reply-To: <20240305035853.916430-3-chris.packham@alliedtelesis.co.nz>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Tue, 5 Mar 2024 09:19:42 +0100
Message-ID: <CAMuHMdVjx6rMbqKiG-9pt+y4c4nVZ-MQk8Miv3HB7jgTd=Pdcg@mail.gmail.com>
Subject: Re: [PATCH v4 2/3] dt-bindings: auxdisplay: Add bindings for generic
 7-segment LED
To: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: andy@kernel.org, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, 
	conor+dt@kernel.org, andrew@lunn.ch, gregory.clement@bootlin.com, 
	sebastian.hesselbarth@gmail.com, lee@kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, 
	linux-leds@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Chris,

On Tue, Mar 5, 2024 at 4:59=E2=80=AFAM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
> Add bindings for a generic 7-segment LED display using GPIOs.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Notes:
>     Changes in v4:
>     - Add ASCII art diagram showing arrangement of segments

Thanks for the update!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/gpio-7-segment.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/auxdisplay/gpio-7-segment.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO based LED segment display
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +properties:
> +  compatible:
> +    const: gpio-7-segment
> +
> +  segment-gpios:
> +    description:

Please add "|", as you want to preserve the formatting of the diagram.

> +      An array of GPIOs one per segment. The first GPIO corresponds to t=
he A
> +      segment the last GPIO corresponds to the G segment. Some LED block=
s also

.. segment, the ...


> +      have a decimal point (currently ignored).
> +
> +               -a-
> +              |   |
> +              f   b
> +              |   |
> +               -g-
> +              |   |
> +              e   c
> +              |   |
> +               -d-  dp
> +
> +    minItems: 7
> +    maxItems: 7

I guess it doesn't hurt to have "maxItems: 8", so people don't have
to update their DTS later when support for the DP is added?
That would need an update to the description above, as the last segment
would then be either G or DP.

> +
> +required:
> +  - segment-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    led-7seg {
> +        compatible =3D "gpio-7-segment";
> +        segment-gpios =3D <&gpio 0 GPIO_ACTIVE_LOW
> +                         &gpio 1 GPIO_ACTIVE_LOW
> +                         &gpio 2 GPIO_ACTIVE_LOW
> +                         &gpio 3 GPIO_ACTIVE_LOW
> +                         &gpio 4 GPIO_ACTIVE_LOW
> +                         &gpio 5 GPIO_ACTIVE_LOW
> +                         &gpio 6 GPIO_ACTIVE_LOW>;

Please group GPIO specifiers using angular brackets:

    segment-gpios =3D <&gpio 0 GPIO_ACTIVE_LOW>,
                     <&gpio 1 GPIO_ACTIVE_LOW>,
                     ...

> +    };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k=
org

In personal conversations with technical people, I call myself a hacker. Bu=
t
when I'm talking to journalists I just say "programmer" or something like t=
hat.
                                -- Linus Torvalds