[PATCH v4 0/3] auxdisplay: 7-segment LED display

[PATCH v4 0/3] auxdisplay: 7-segment LED display

[Chris Packham]

This series adds a driver for a 7-segment LED display.

I think I've addressed all of Andy's feedback in this round. I haven't
heard from the ARM maintainers on any of the previous rounds. At Andy's
request I've dropped the USB LED change as it's not related. I can
submit the dts change separately if required, I've mostly been including
it so there is an in-tree user of the driver I'm adding.

Chris Packham (3):
  auxdisplay: Add 7-segment LED display driver
  dt-bindings: auxdisplay: Add bindings for generic 7-segment LED
  ARM: dts: marvell: Add 7-segment LED display on x530

 .../bindings/auxdisplay/gpio-7-segment.yaml   |  54 +++++++++
 .../boot/dts/marvell/armada-385-atl-x530.dts  |  13 ++-
 drivers/auxdisplay/Kconfig                    |  10 ++
 drivers/auxdisplay/Makefile                   |   1 +
 drivers/auxdisplay/seg-led-gpio.c             | 109 ++++++++++++++++++
 5 files changed, 186 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/gpio-7-segment.yaml
 create mode 100644 drivers/auxdisplay/seg-led-gpio.c

-- 
2.43.2

[PATCH v4 1/3] auxdisplay: Add 7-segment LED display driver

[Chris Packham]

Add a driver for a 7-segment LED display. At the moment only one
character is supported but it should be possible to expand this to
support more characters and/or 14-segment displays in the future.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v4:
    - Fix one more usage of 7 segment
    - Move ASCII art diagram to DT binding
    - Include map_to_7segment.h for map_to_seg7()
    - Use initialiser for values in seg_led_update
    Changes in v3:
    - "7 segment" -> "7-Segment" in Kconfig help text
    - Update after feedback from Andy
    - Use compatible = "gpio-7-segment" as suggested by Rob
    Changes in v2:
    - Rebased on top of auxdisplay/for-next dropping unnecessary code for 7
      segment maps.
    - Update for new linedisp_register() API
    - Include headers based on actual usage
    - Allow building as module
    - Use compatible = "generic-gpio-7seg" to keep checkpatch.pl happy

 drivers/auxdisplay/Kconfig        |  10 +++
 drivers/auxdisplay/Makefile       |   1 +
 drivers/auxdisplay/seg-led-gpio.c | 109 ++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+)
 create mode 100644 drivers/auxdisplay/seg-led-gpio.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index d4be0a3695ce..898ecfb34ed7 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -211,6 +211,16 @@ config ARM_CHARLCD
 	  line and the Linux version on the second line, but that's
 	  still useful.
 
+config SEG_LED_GPIO
+	tristate "Generic 7-segment LED display"
+	select LINEDISP
+	help
+	  This driver supports a generic 7-segment LED display made up
+	  of GPIO pins connected to the individual segments.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called seg-led-gpio.
+
 menuconfig PARPORT_PANEL
 	tristate "Parallel port LCD/Keypad Panel support"
 	depends on PARPORT
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index a725010ca651..4a8ea41b0550 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
 obj-$(CONFIG_LCD2S)		+= lcd2s.o
 obj-$(CONFIG_LINEDISP)		+= line-display.o
 obj-$(CONFIG_MAX6959)		+= max6959.o
+obj-$(CONFIG_SEG_LED_GPIO)	+= seg-led-gpio.o
diff --git a/drivers/auxdisplay/seg-led-gpio.c b/drivers/auxdisplay/seg-led-gpio.c
new file mode 100644
index 000000000000..6c97c068f4c1
--- /dev/null
+++ b/drivers/auxdisplay/seg-led-gpio.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for a 7-segment LED display
+ *
+ * The decimal point LED present on some devices is currently not
+ * supported.
+ *
+ * Copyright (C) Allied Telesis Labs
+ */
+
+#include <linux/bitmap.h>
+#include <linux/container_of.h>
+#include <linux/errno.h>
+#include <linux/gpio/consumer.h>
+#include <linux/map_to_7segment.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+#include "line-display.h"
+
+struct seg_led_priv {
+	struct linedisp linedisp;
+	struct delayed_work work;
+	struct gpio_descs *segment_gpios;
+};
+
+static void seg_led_update(struct work_struct *work)
+{
+	struct seg_led_priv *priv = container_of(work, struct seg_led_priv, work.work);
+	struct linedisp *linedisp = &priv->linedisp;
+	struct linedisp_map *map = linedisp->map;
+	DECLARE_BITMAP(values, 8) = { 0 };
+
+	bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0);
+
+	gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc,
+				       priv->segment_gpios->info, values);
+}
+
+static int seg_led_linedisp_get_map_type(struct linedisp *linedisp)
+{
+	struct seg_led_priv *priv = container_of(linedisp, struct seg_led_priv, linedisp);
+
+	INIT_DELAYED_WORK(&priv->work, seg_led_update);
+	return LINEDISP_MAP_SEG7;
+}
+
+static void seg_led_linedisp_update(struct linedisp *linedisp)
+{
+	struct seg_led_priv *priv = container_of(linedisp, struct seg_led_priv, linedisp);
+
+	schedule_delayed_work(&priv->work, 0);
+}
+
+static const struct linedisp_ops seg_led_linedisp_ops = {
+	.get_map_type = seg_led_linedisp_get_map_type,
+	.update = seg_led_linedisp_update,
+};
+
+static int seg_led_probe(struct platform_device *pdev)
+{
+	struct seg_led_priv *priv;
+	struct device *dev = &pdev->dev;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->segment_gpios = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->segment_gpios))
+		return PTR_ERR(priv->segment_gpios);
+
+	return linedisp_register(&priv->linedisp, dev, 1, &seg_led_linedisp_ops);
+}
+
+static int seg_led_remove(struct platform_device *pdev)
+{
+	struct seg_led_priv *priv = platform_get_drvdata(pdev);
+
+	cancel_delayed_work_sync(&priv->work);
+	linedisp_unregister(&priv->linedisp);
+
+	return 0;
+}
+
+static const struct of_device_id seg_led_of_match[] = {
+	{ .compatible = "gpio-7-segment"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, seg_led_of_match);
+
+static struct platform_driver seg_led_driver = {
+	.probe = seg_led_probe,
+	.remove = seg_led_remove,
+	.driver = {
+		.name = "seg-led-gpio",
+		.of_match_table = seg_led_of_match,
+	},
+};
+module_platform_driver(seg_led_driver);
+
+MODULE_AUTHOR("Chris Packham <chris.packham@alliedtelesis.co.nz>");
+MODULE_DESCRIPTION("7 segment LED driver");
+MODULE_LICENSE("GPL");
-- 
2.43.2

Re: [PATCH v4 1/3] auxdisplay: Add 7-segment LED display driver

[Geert Uytterhoeven]

Hi Chris,

On Tue, Mar 5, 2024 at 4:59 AM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
> Add a driver for a 7-segment LED display. At the moment only one
> character is supported but it should be possible to expand this to
> support more characters and/or 14-segment displays in the future.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>
> Notes:
>     Changes in v4:
>     - Fix one more usage of 7 segment
>     - Move ASCII art diagram to DT binding
>     - Include map_to_7segment.h for map_to_seg7()
>     - Use initialiser for values in seg_led_update

Thanks for the update!

> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -211,6 +211,16 @@ config ARM_CHARLCD
>           line and the Linux version on the second line, but that's
>           still useful.
>
> +config SEG_LED_GPIO
> +       tristate "Generic 7-segment LED display"

"depends on GPIOLIB || COMPILE_TEST"?

The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>

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. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 1/3] auxdisplay: Add 7-segment LED display driver

[Geert Uytterhoeven]

Hi Chris,

On Tue, Mar 5, 2024 at 4:59 AM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
> Add a driver for a 7-segment LED display. At the moment only one
> character is supported but it should be possible to expand this to
> support more characters and/or 14-segment displays in the future.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Sorry, I spoke too soon...

> --- /dev/null
> +++ b/drivers/auxdisplay/seg-led-gpio.c

> +static void seg_led_update(struct work_struct *work)
> +{
> +       struct seg_led_priv *priv = container_of(work, struct seg_led_priv, work.work);
> +       struct linedisp *linedisp = &priv->linedisp;
> +       struct linedisp_map *map = linedisp->map;
> +       DECLARE_BITMAP(values, 8) = { 0 };
> +
> +       bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0);
> +
> +       gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc,
> +                                      priv->segment_gpios->info, values);
> +}

> +static int seg_led_probe(struct platform_device *pdev)
> +{
> +       struct seg_led_priv *priv;
> +       struct device *dev = &pdev->dev;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, priv);
> +
> +       priv->segment_gpios = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
> +       if (IS_ERR(priv->segment_gpios))
> +               return PTR_ERR(priv->segment_gpios);

This needs some validation of priv->segment_gpios->ndescs, else the
call to gpiod_set_array_value_cansleep() in seg_led_update() may
trigger an out-of-bounds access of the values bitmap.

> +
> +       return linedisp_register(&priv->linedisp, dev, 1, &seg_led_linedisp_ops);
> +}

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. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v4 1/3] auxdisplay: Add 7-segment LED display driver

[Andy Shevchenko]

On Tue, Mar 05, 2024 at 09:23:07AM +0100, Geert Uytterhoeven wrote:
> On Tue, Mar 5, 2024 at 4:59 AM Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:

...

> > +       priv->segment_gpios = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
> > +       if (IS_ERR(priv->segment_gpios))
> > +               return PTR_ERR(priv->segment_gpios);
> 
> This needs some validation of priv->segment_gpios->ndescs, else the
> call to gpiod_set_array_value_cansleep() in seg_led_update() may
> trigger an out-of-bounds access of the values bitmap.

Alternatively we can call gpiod_count() beforehand and check its result.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/3] auxdisplay: Add 7-segment LED display driver

[Chris Packham]

On 6/03/24 03:57, Andy Shevchenko wrote:
> On Tue, Mar 05, 2024 at 09:23:07AM +0100, Geert Uytterhoeven wrote:
>> On Tue, Mar 5, 2024 at 4:59 AM Chris Packham
>> <chris.packham@alliedtelesis.co.nz> wrote:
> ...
>
>>> +       priv->segment_gpios = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
>>> +       if (IS_ERR(priv->segment_gpios))
>>> +               return PTR_ERR(priv->segment_gpios);
>> This needs some validation of priv->segment_gpios->ndescs, else the
>> call to gpiod_set_array_value_cansleep() in seg_led_update() may
>> trigger an out-of-bounds access of the values bitmap.
> Alternatively we can call gpiod_count() beforehand and check its result.
Unless there are any objections I think I'll go with the ndescs check as 
it'll be easier to update to the subnode style in the future. It does 
mean there will be some extra allocations/frees (handled via the devm_ 
APIs) in the error case.

Re: [PATCH v4 1/3] auxdisplay: Add 7-segment LED display driver

[Andy Shevchenko]

On Wed, Mar 6, 2024 at 12:34 AM Chris Packham
<Chris.Packham@alliedtelesis.co.nz> wrote:
> On 6/03/24 03:57, Andy Shevchenko wrote:
> > On Tue, Mar 05, 2024 at 09:23:07AM +0100, Geert Uytterhoeven wrote:
> >> On Tue, Mar 5, 2024 at 4:59 AM Chris Packham
> >> <chris.packham@alliedtelesis.co.nz> wrote:

...

> >>> +       priv->segment_gpios = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
> >>> +       if (IS_ERR(priv->segment_gpios))
> >>> +               return PTR_ERR(priv->segment_gpios);
> >> This needs some validation of priv->segment_gpios->ndescs, else the
> >> call to gpiod_set_array_value_cansleep() in seg_led_update() may
> >> trigger an out-of-bounds access of the values bitmap.
> > Alternatively we can call gpiod_count() beforehand and check its result.
> Unless there are any objections I think I'll go with the ndescs check as
> it'll be easier to update to the subnode style in the future.

Either works for me.

> It does
> mean there will be some extra allocations/frees (handled via the devm_
> APIs) in the error case.


-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 2/3] dt-bindings: auxdisplay: Add bindings for generic 7-segment LED

[Chris Packham]

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
    Changes in v3:
    - Set maxItems: 7
    - Expand description of segment-gpios property.
    - Use compatible = "gpio-7-segment" as suggested by Rob
    Changes in v2:
    - Use compatible = "generic-gpio-7seg" to keep checkpatch.pl happy

 .../bindings/auxdisplay/gpio-7-segment.yaml   | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/gpio-7-segment.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/gpio-7-segment.yaml b/Documentation/devicetree/bindings/auxdisplay/gpio-7-segment.yaml
new file mode 100644
index 000000000000..172548dfb142
--- /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:
+      An array of GPIOs one per segment. The first GPIO corresponds to the A
+      segment the last GPIO corresponds to the G segment. Some LED blocks also
+      have a decimal point (currently ignored).
+
+               -a-
+              |   |
+              f   b
+              |   |
+               -g-
+              |   |
+              e   c
+              |   |
+               -d-  dp
+
+    minItems: 7
+    maxItems: 7
+
+required:
+  - segment-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/gpio/gpio.h>
+
+    led-7seg {
+        compatible = "gpio-7-segment";
+        segment-gpios = <&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>;
+    };
-- 
2.43.2

Re: [PATCH v4 2/3] dt-bindings: auxdisplay: Add bindings for generic 7-segment LED

[Geert Uytterhoeven]

Hi Chris,

On Tue, Mar 5, 2024 at 4:59 AM 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 the A
> +      segment the last GPIO corresponds to the G segment. Some LED blocks 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 = "gpio-7-segment";
> +        segment-gpios = <&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 = <&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. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v4 3/3] ARM: dts: marvell: Add 7-segment LED display on x530

[Chris Packham]

The Allied Telesis x530 products have a 7-segment LED display which is
used for node identification when the devices are stacked. Represent
this as a gpio-7-segment device.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    Changes in v4:
    - Use correct compatible name in commit message
    Changes in v3:
    - Use compatible = "gpio-7-segment" as suggested by Rob
    Changes in v2:
    - Use compatible = "generic-gpio-7seg" to keep checkpatch.pl happy

 arch/arm/boot/dts/marvell/armada-385-atl-x530.dts | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/marvell/armada-385-atl-x530.dts b/arch/arm/boot/dts/marvell/armada-385-atl-x530.dts
index 5a9ab8410b7b..60c2abf572b6 100644
--- a/arch/arm/boot/dts/marvell/armada-385-atl-x530.dts
+++ b/arch/arm/boot/dts/marvell/armada-385-atl-x530.dts
@@ -43,6 +43,17 @@ uart0: serial@12000 {
 			};
 		};
 	};
+
+	led-7seg {
+		compatible = "gpio-7-segment";
+		segment-gpios = <&led_7seg_gpio 0 GPIO_ACTIVE_LOW
+				 &led_7seg_gpio 1 GPIO_ACTIVE_LOW
+				 &led_7seg_gpio 2 GPIO_ACTIVE_LOW
+				 &led_7seg_gpio 3 GPIO_ACTIVE_LOW
+				 &led_7seg_gpio 4 GPIO_ACTIVE_LOW
+				 &led_7seg_gpio 5 GPIO_ACTIVE_LOW
+				 &led_7seg_gpio 6 GPIO_ACTIVE_LOW>;
+	};
 };
 
 &pciec {
@@ -149,7 +160,7 @@ i2c@3 {
 			#size-cells = <0>;
 			reg = <3>;
 
-			gpio@20 {
+			led_7seg_gpio: gpio@20 {
 				compatible = "nxp,pca9554";
 				gpio-controller;
 				#gpio-cells = <2>;
-- 
2.43.2