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

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

[Chris Packham]

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

I'd like to get some feedback on how this could be extended to support >1
character. The driver as presented is sufficient for my hardware which only has
a single character display but I can see that for this to be generically useful
supporting more characters would be desireable.

Earlier I posted an idea that the characters could be represended by
sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
convenience of using devm_gpiod_get_array() (unless I've missed something).

[1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/

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

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

-- 
2.43.2

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

[Chris Packham]

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

I'd like to get some feedback on how this could be extended to support >1
character. The driver as presented is sufficient for my hardware which only has
a single character display but I can see that for this to be generically useful
supporting more characters would be desireable.

Earlier I posted an idea that the characters could be represended by
sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
convenience of using devm_gpiod_get_array() (unless I've missed something).

[1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/

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

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

-- 
2.43.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 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>
---
 drivers/auxdisplay/Kconfig   |   7 ++
 drivers/auxdisplay/Makefile  |   1 +
 drivers/auxdisplay/seg-led.c | 152 +++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/auxdisplay/seg-led.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index d944d5298eca..e826b5b15881 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -197,6 +197,13 @@ config ARM_CHARLCD
 	  line and the Linux version on the second line, but that's
 	  still useful.
 
+config SEG_LED
+	bool "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.
+
 menuconfig PARPORT_PANEL
 	tristate "Parallel port LCD/Keypad Panel support"
 	depends on PARPORT
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 6968ed4d3f0a..808fdf156bd5 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_HT16K33)		+= ht16k33.o
 obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
 obj-$(CONFIG_LCD2S)		+= lcd2s.o
 obj-$(CONFIG_LINEDISP)		+= line-display.o
+obj-$(CONFIG_SEG_LED)		+= seg-led.o
diff --git a/drivers/auxdisplay/seg-led.c b/drivers/auxdisplay/seg-led.c
new file mode 100644
index 000000000000..c0b302a09cbb
--- /dev/null
+++ b/drivers/auxdisplay/seg-led.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for a 7 segment LED display
+ *
+ * The GPIOs are wired to the 7 segments in a clock wise fashion starting from
+ * the top.
+ *
+ *      -a-
+ *     |   |
+ *     f   b
+ *     |   |
+ *      -g-
+ *     |   |
+ *     e   c
+ *     |   |
+ *      -d-
+ *
+ * The decimal point LED presnet on some devices is currently not
+ * supported.
+ *
+ * Copyright (C) Allied Telesis Labs
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/map_to_7segment.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include "line-display.h"
+
+struct seg_led_priv {
+	struct gpio_descs *segment_gpios;
+	struct delayed_work work;
+	struct linedisp linedisp;
+	struct seg7_conversion_map seg7;
+	unsigned int map_size;
+	size_t num_char;
+	char curr[];
+};
+
+static const SEG7_DEFAULT_MAP(initial_map_seg7);
+
+static ssize_t map_seg7_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct seg_led_priv *priv = dev_get_drvdata(dev);
+
+	memcpy(buf, &priv->seg7, priv->map_size);
+	return priv->map_size;
+}
+
+static ssize_t map_seg7_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t cnt)
+{
+	struct seg_led_priv *priv = dev_get_drvdata(dev);
+
+	if (cnt != priv->map_size)
+		return -EINVAL;
+
+	memcpy(&priv->seg7, buf, cnt);
+	return cnt;
+}
+
+static DEVICE_ATTR_RW(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 void seg_led_update(struct work_struct *work)
+{
+	struct seg_led_priv *priv = container_of(work, struct seg_led_priv, work.work);
+	DECLARE_BITMAP(values, 8);
+
+	values[0] = map_to_seg7(&priv->seg7, priv->curr[0]);
+
+	gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc,
+				       priv->segment_gpios->info, values);
+}
+
+static const struct of_device_id seg_led_of_match[] = {
+	{ .compatible = "generic,gpio-7seg"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, seg_led_of_match);
+
+static int seg_led_probe(struct platform_device *pdev)
+{
+	struct seg_led_priv *priv;
+	int err;
+
+	priv = devm_kzalloc(&pdev->dev, struct_size(priv, curr, 1), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	priv->num_char = 1;
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->segment_gpios = devm_gpiod_get_array(&pdev->dev, "segment", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->segment_gpios))
+		return PTR_ERR(priv->segment_gpios);
+
+	priv->seg7 = initial_map_seg7;
+	priv->map_size = sizeof(priv->seg7);
+
+	err = device_create_file(&pdev->dev, &dev_attr_map_seg7);
+	if (err)
+		return err;
+
+	INIT_DELAYED_WORK(&priv->work, seg_led_update);
+
+	err = linedisp_register(&priv->linedisp, &pdev->dev, 1, priv->curr,
+				seg_led_linedisp_update);
+	if (err) {
+		device_remove_file(&pdev->dev, &dev_attr_map_seg7);
+		return err;
+	}
+
+	return 0;
+}
+
+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);
+	device_remove_file(&pdev->dev, &dev_attr_map_seg7);
+
+	return 0;
+}
+
+static struct platform_driver seg_led_driver = {
+	.probe = seg_led_probe,
+	.remove = seg_led_remove,
+	.driver = {
+		.name = "seg-led",
+		.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

[PATCH 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>
---
 drivers/auxdisplay/Kconfig   |   7 ++
 drivers/auxdisplay/Makefile  |   1 +
 drivers/auxdisplay/seg-led.c | 152 +++++++++++++++++++++++++++++++++++
 3 files changed, 160 insertions(+)
 create mode 100644 drivers/auxdisplay/seg-led.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index d944d5298eca..e826b5b15881 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -197,6 +197,13 @@ config ARM_CHARLCD
 	  line and the Linux version on the second line, but that's
 	  still useful.
 
+config SEG_LED
+	bool "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.
+
 menuconfig PARPORT_PANEL
 	tristate "Parallel port LCD/Keypad Panel support"
 	depends on PARPORT
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 6968ed4d3f0a..808fdf156bd5 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_HT16K33)		+= ht16k33.o
 obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
 obj-$(CONFIG_LCD2S)		+= lcd2s.o
 obj-$(CONFIG_LINEDISP)		+= line-display.o
+obj-$(CONFIG_SEG_LED)		+= seg-led.o
diff --git a/drivers/auxdisplay/seg-led.c b/drivers/auxdisplay/seg-led.c
new file mode 100644
index 000000000000..c0b302a09cbb
--- /dev/null
+++ b/drivers/auxdisplay/seg-led.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for a 7 segment LED display
+ *
+ * The GPIOs are wired to the 7 segments in a clock wise fashion starting from
+ * the top.
+ *
+ *      -a-
+ *     |   |
+ *     f   b
+ *     |   |
+ *      -g-
+ *     |   |
+ *     e   c
+ *     |   |
+ *      -d-
+ *
+ * The decimal point LED presnet on some devices is currently not
+ * supported.
+ *
+ * Copyright (C) Allied Telesis Labs
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/map_to_7segment.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include "line-display.h"
+
+struct seg_led_priv {
+	struct gpio_descs *segment_gpios;
+	struct delayed_work work;
+	struct linedisp linedisp;
+	struct seg7_conversion_map seg7;
+	unsigned int map_size;
+	size_t num_char;
+	char curr[];
+};
+
+static const SEG7_DEFAULT_MAP(initial_map_seg7);
+
+static ssize_t map_seg7_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct seg_led_priv *priv = dev_get_drvdata(dev);
+
+	memcpy(buf, &priv->seg7, priv->map_size);
+	return priv->map_size;
+}
+
+static ssize_t map_seg7_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t cnt)
+{
+	struct seg_led_priv *priv = dev_get_drvdata(dev);
+
+	if (cnt != priv->map_size)
+		return -EINVAL;
+
+	memcpy(&priv->seg7, buf, cnt);
+	return cnt;
+}
+
+static DEVICE_ATTR_RW(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 void seg_led_update(struct work_struct *work)
+{
+	struct seg_led_priv *priv = container_of(work, struct seg_led_priv, work.work);
+	DECLARE_BITMAP(values, 8);
+
+	values[0] = map_to_seg7(&priv->seg7, priv->curr[0]);
+
+	gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc,
+				       priv->segment_gpios->info, values);
+}
+
+static const struct of_device_id seg_led_of_match[] = {
+	{ .compatible = "generic,gpio-7seg"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, seg_led_of_match);
+
+static int seg_led_probe(struct platform_device *pdev)
+{
+	struct seg_led_priv *priv;
+	int err;
+
+	priv = devm_kzalloc(&pdev->dev, struct_size(priv, curr, 1), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	priv->num_char = 1;
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->segment_gpios = devm_gpiod_get_array(&pdev->dev, "segment", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->segment_gpios))
+		return PTR_ERR(priv->segment_gpios);
+
+	priv->seg7 = initial_map_seg7;
+	priv->map_size = sizeof(priv->seg7);
+
+	err = device_create_file(&pdev->dev, &dev_attr_map_seg7);
+	if (err)
+		return err;
+
+	INIT_DELAYED_WORK(&priv->work, seg_led_update);
+
+	err = linedisp_register(&priv->linedisp, &pdev->dev, 1, priv->curr,
+				seg_led_linedisp_update);
+	if (err) {
+		device_remove_file(&pdev->dev, &dev_attr_map_seg7);
+		return err;
+	}
+
+	return 0;
+}
+
+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);
+	device_remove_file(&pdev->dev, &dev_attr_map_seg7);
+
+	return 0;
+}
+
+static struct platform_driver seg_led_driver = {
+	.probe = seg_led_probe,
+	.remove = seg_led_remove,
+	.driver = {
+		.name = "seg-led",
+		.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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 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>
---
 .../auxdisplay/generic,gpio-7seg.yaml         | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml b/Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
new file mode 100644
index 000000000000..238da2ede740
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/generic,gpio-7seg.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: generic,gpio-7seg
+
+  segment-gpios:
+    description:
+      An array of GPIOs one per segment.
+    minItems: 7
+
+required:
+  - segment-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/gpio/gpio.h>
+
+    led-7seg {
+        compatible = "generic,gpio-7seg";
+        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

[PATCH 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>
---
 .../auxdisplay/generic,gpio-7seg.yaml         | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml b/Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
new file mode 100644
index 000000000000..238da2ede740
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/generic,gpio-7seg.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: generic,gpio-7seg
+
+  segment-gpios:
+    description:
+      An array of GPIOs one per segment.
+    minItems: 7
+
+required:
+  - segment-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/gpio/gpio.h>
+
+    led-7seg {
+        compatible = "generic,gpio-7seg";
+        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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 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-7seg device.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 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..088b77987081 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 = "generic,gpio-7seg";
+		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

[PATCH 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-7seg device.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 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..088b77987081 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 = "generic,gpio-7seg";
+		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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Sun, Feb 25, 2024 at 11:34 PM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
>
> This series adds a driver for a 7 segment LED display.
>
> I'd like to get some feedback on how this could be extended to support >1
> character. The driver as presented is sufficient for my hardware which only has
> a single character display but I can see that for this to be generically useful
> supporting more characters would be desireable.
>
> Earlier I posted an idea that the characters could be represended by
> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> convenience of using devm_gpiod_get_array() (unless I've missed something).

It seems you didn't know that the tree for auxdisplay has been changed.
Can you rebase your stuff on top of
https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-auxdisplay.git/log/?h=for-next?
It will reduce your code base by ~50%.

WRT subnodes, you can go with device_for_each_child_node() and
retrieve gpio array per digit. It means you will have an array of
arrays of GPIOs.

> [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/



-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Feb 25, 2024 at 11:34 PM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
>
> This series adds a driver for a 7 segment LED display.
>
> I'd like to get some feedback on how this could be extended to support >1
> character. The driver as presented is sufficient for my hardware which only has
> a single character display but I can see that for this to be generically useful
> supporting more characters would be desireable.
>
> Earlier I posted an idea that the characters could be represended by
> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> convenience of using devm_gpiod_get_array() (unless I've missed something).

It seems you didn't know that the tree for auxdisplay has been changed.
Can you rebase your stuff on top of
https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-auxdisplay.git/log/?h=for-next?
It will reduce your code base by ~50%.

WRT subnodes, you can go with device_for_each_child_node() and
retrieve gpio array per digit. It means you will have an array of
arrays of GPIOs.

> [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/



-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Sun, Feb 25, 2024 at 11:34 PM 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.

(I try to comment only on the things that will remain after rebasing
on top of auxdisplay:for-next)

...

> +config SEG_LED
> +       bool "Generic 7 segment LED display"

Why can't it be a module?

> +       select LINEDISP
> +       help
> +         This driver supports a generic 7 segment LED display made up
> +         of GPIO pins connected to the individual segments.

Checkpatch wants 3+ lines of help, I would add the module name (after
changing it to be tristate, etc).

...

> + * The GPIOs are wired to the 7 segments in a clock wise fashion starting from

clockwise

> + * the top.

...

> + * The decimal point LED presnet on some devices is currently not

present

> + * supported.

...

> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/map_to_7segment.h>
> +#include <linux/module.h>

> +#include <linux/of.h>

This is not used. And actually shouldn't be in a new code like this
with rare exceptions.

> +#include <linux/platform_device.h>

This is rather semirandom, please use IWYU (Include What You Use)
principle. We have, among others, container_of.h, types.h, err.h,
bitmap.h, mod_devicetable.h.

...

With

    sturct device *dev = &pdev->dev;

the below code will be neater.

> +       priv = devm_kzalloc(&pdev->dev, struct_size(priv, curr, 1), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +       priv->num_char = 1;
> +
> +       platform_set_drvdata(pdev, priv);
> +
> +       priv->segment_gpios = devm_gpiod_get_array(&pdev->dev, "segment", GPIOD_OUT_LOW);
> +       if (IS_ERR(priv->segment_gpios))
> +               return PTR_ERR(priv->segment_gpios);

...

> +static struct platform_driver seg_led_driver = {
> +       .probe = seg_led_probe,
> +       .remove = seg_led_remove,
> +       .driver = {
> +               .name = "seg-led",
> +               .of_match_table = seg_led_of_match,
> +       },
> +};

> +

Redundant blank line.

> +module_platform_driver(seg_led_driver);
> +
> +MODULE_AUTHOR("Chris Packham <chris.packham@alliedtelesis.co.nz>");
> +MODULE_DESCRIPTION("7 segment LED driver");
> +MODULE_LICENSE("GPL");

> +

Seems like a redundant blank line at the end of the file.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Feb 25, 2024 at 11:34 PM 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.

(I try to comment only on the things that will remain after rebasing
on top of auxdisplay:for-next)

...

> +config SEG_LED
> +       bool "Generic 7 segment LED display"

Why can't it be a module?

> +       select LINEDISP
> +       help
> +         This driver supports a generic 7 segment LED display made up
> +         of GPIO pins connected to the individual segments.

Checkpatch wants 3+ lines of help, I would add the module name (after
changing it to be tristate, etc).

...

> + * The GPIOs are wired to the 7 segments in a clock wise fashion starting from

clockwise

> + * the top.

...

> + * The decimal point LED presnet on some devices is currently not

present

> + * supported.

...

> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/map_to_7segment.h>
> +#include <linux/module.h>

> +#include <linux/of.h>

This is not used. And actually shouldn't be in a new code like this
with rare exceptions.

> +#include <linux/platform_device.h>

This is rather semirandom, please use IWYU (Include What You Use)
principle. We have, among others, container_of.h, types.h, err.h,
bitmap.h, mod_devicetable.h.

...

With

    sturct device *dev = &pdev->dev;

the below code will be neater.

> +       priv = devm_kzalloc(&pdev->dev, struct_size(priv, curr, 1), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +       priv->num_char = 1;
> +
> +       platform_set_drvdata(pdev, priv);
> +
> +       priv->segment_gpios = devm_gpiod_get_array(&pdev->dev, "segment", GPIOD_OUT_LOW);
> +       if (IS_ERR(priv->segment_gpios))
> +               return PTR_ERR(priv->segment_gpios);

...

> +static struct platform_driver seg_led_driver = {
> +       .probe = seg_led_probe,
> +       .remove = seg_led_remove,
> +       .driver = {
> +               .name = "seg-led",
> +               .of_match_table = seg_led_of_match,
> +       },
> +};

> +

Redundant blank line.

> +module_platform_driver(seg_led_driver);
> +
> +MODULE_AUTHOR("Chris Packham <chris.packham@alliedtelesis.co.nz>");
> +MODULE_DESCRIPTION("7 segment LED driver");
> +MODULE_LICENSE("GPL");

> +

Seems like a redundant blank line at the end of the file.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Randy Dunlap]

Hi--

On 2/25/24 13:34, Chris Packham 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>
> ---
>  drivers/auxdisplay/Kconfig   |   7 ++
>  drivers/auxdisplay/Makefile  |   1 +
>  drivers/auxdisplay/seg-led.c | 152 +++++++++++++++++++++++++++++++++++
>  3 files changed, 160 insertions(+)
>  create mode 100644 drivers/auxdisplay/seg-led.c
> 
> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
> index d944d5298eca..e826b5b15881 100644
> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -197,6 +197,13 @@ config ARM_CHARLCD
>  	  line and the Linux version on the second line, but that's
>  	  still useful.
>  
> +config SEG_LED
> +	bool "Generic 7 segment LED display"
> +	select LINEDISP
> +	help
> +	  This driver supports a generic 7 segment LED display made up

	                                 7-segment

> +	  of GPIO pins connected to the individual segments.
> +
>  menuconfig PARPORT_PANEL
>  	tristate "Parallel port LCD/Keypad Panel support"
>  	depends on PARPORT
> diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
> index 6968ed4d3f0a..808fdf156bd5 100644
> --- a/drivers/auxdisplay/Makefile
> +++ b/drivers/auxdisplay/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_HT16K33)		+= ht16k33.o
>  obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
>  obj-$(CONFIG_LCD2S)		+= lcd2s.o
>  obj-$(CONFIG_LINEDISP)		+= line-display.o
> +obj-$(CONFIG_SEG_LED)		+= seg-led.o
> diff --git a/drivers/auxdisplay/seg-led.c b/drivers/auxdisplay/seg-led.c
> new file mode 100644
> index 000000000000..c0b302a09cbb
> --- /dev/null
> +++ b/drivers/auxdisplay/seg-led.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for a 7 segment LED display
> + *
> + * The GPIOs are wired to the 7 segments in a clock wise fashion starting from
> + * the top.
> + *
> + *      -a-
> + *     |   |
> + *     f   b
> + *     |   |
> + *      -g-
> + *     |   |
> + *     e   c
> + *     |   |
> + *      -d-
> + *
> + * The decimal point LED presnet on some devices is currently not

                            present

> + * supported.
> + *
> + * Copyright (C) Allied Telesis Labs
> + */

-- 
#Randy

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Randy Dunlap]

Hi--

On 2/25/24 13:34, Chris Packham 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>
> ---
>  drivers/auxdisplay/Kconfig   |   7 ++
>  drivers/auxdisplay/Makefile  |   1 +
>  drivers/auxdisplay/seg-led.c | 152 +++++++++++++++++++++++++++++++++++
>  3 files changed, 160 insertions(+)
>  create mode 100644 drivers/auxdisplay/seg-led.c
> 
> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
> index d944d5298eca..e826b5b15881 100644
> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -197,6 +197,13 @@ config ARM_CHARLCD
>  	  line and the Linux version on the second line, but that's
>  	  still useful.
>  
> +config SEG_LED
> +	bool "Generic 7 segment LED display"
> +	select LINEDISP
> +	help
> +	  This driver supports a generic 7 segment LED display made up

	                                 7-segment

> +	  of GPIO pins connected to the individual segments.
> +
>  menuconfig PARPORT_PANEL
>  	tristate "Parallel port LCD/Keypad Panel support"
>  	depends on PARPORT
> diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
> index 6968ed4d3f0a..808fdf156bd5 100644
> --- a/drivers/auxdisplay/Makefile
> +++ b/drivers/auxdisplay/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_HT16K33)		+= ht16k33.o
>  obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
>  obj-$(CONFIG_LCD2S)		+= lcd2s.o
>  obj-$(CONFIG_LINEDISP)		+= line-display.o
> +obj-$(CONFIG_SEG_LED)		+= seg-led.o
> diff --git a/drivers/auxdisplay/seg-led.c b/drivers/auxdisplay/seg-led.c
> new file mode 100644
> index 000000000000..c0b302a09cbb
> --- /dev/null
> +++ b/drivers/auxdisplay/seg-led.c
> @@ -0,0 +1,152 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for a 7 segment LED display
> + *
> + * The GPIOs are wired to the 7 segments in a clock wise fashion starting from
> + * the top.
> + *
> + *      -a-
> + *     |   |
> + *     f   b
> + *     |   |
> + *      -g-
> + *     |   |
> + *     e   c
> + *     |   |
> + *      -d-
> + *
> + * The decimal point LED presnet on some devices is currently not

                            present

> + * supported.
> + *
> + * Copyright (C) Allied Telesis Labs
> + */

-- 
#Randy

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

[Alexander Dahl]

Hello Chris,

Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham:
> This series adds a driver for a 7 segment LED display.
> 
> I'd like to get some feedback on how this could be extended to support >1
> character. The driver as presented is sufficient for my hardware which only has
> a single character display but I can see that for this to be generically useful
> supporting more characters would be desireable.
> 
> Earlier I posted an idea that the characters could be represended by
> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> convenience of using devm_gpiod_get_array() (unless I've missed something).
> 
> [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/

Read that thread out of curiosity and I'm sorry if I'm late to the
party, but I wondered why this is limited to LEDs connected to GPIOs?

Would it be possible to somehow stack this on top of some existing
LEDs?  I mean you could wire a 7 segment device to almost any LED
driver IC with enough channels, couldn't you?

Greets
Alex

> 
> 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
> 
>  .../auxdisplay/generic,gpio-7seg.yaml         |  40 +++++
>  .../boot/dts/marvell/armada-385-atl-x530.dts  |  13 +-
>  drivers/auxdisplay/Kconfig                    |   7 +
>  drivers/auxdisplay/Makefile                   |   1 +
>  drivers/auxdisplay/seg-led.c                  | 152 ++++++++++++++++++
>  5 files changed, 212 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
>  create mode 100644 drivers/auxdisplay/seg-led.c
> 
> -- 
> 2.43.2
> 
> 

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

[Alexander Dahl]

Hello Chris,

Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham:
> This series adds a driver for a 7 segment LED display.
> 
> I'd like to get some feedback on how this could be extended to support >1
> character. The driver as presented is sufficient for my hardware which only has
> a single character display but I can see that for this to be generically useful
> supporting more characters would be desireable.
> 
> Earlier I posted an idea that the characters could be represended by
> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> convenience of using devm_gpiod_get_array() (unless I've missed something).
> 
> [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/

Read that thread out of curiosity and I'm sorry if I'm late to the
party, but I wondered why this is limited to LEDs connected to GPIOs?

Would it be possible to somehow stack this on top of some existing
LEDs?  I mean you could wire a 7 segment device to almost any LED
driver IC with enough channels, couldn't you?

Greets
Alex

> 
> 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
> 
>  .../auxdisplay/generic,gpio-7seg.yaml         |  40 +++++
>  .../boot/dts/marvell/armada-385-atl-x530.dts  |  13 +-
>  drivers/auxdisplay/Kconfig                    |   7 +
>  drivers/auxdisplay/Makefile                   |   1 +
>  drivers/auxdisplay/seg-led.c                  | 152 ++++++++++++++++++
>  5 files changed, 212 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
>  create mode 100644 drivers/auxdisplay/seg-led.c
> 
> -- 
> 2.43.2
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 04:23:15AM +0200, Andy Shevchenko wrote:
> On Sun, Feb 25, 2024 at 11:34 PM Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
> >
> > This series adds a driver for a 7 segment LED display.
> >
> > I'd like to get some feedback on how this could be extended to support >1
> > character. The driver as presented is sufficient for my hardware which only has
> > a single character display but I can see that for this to be generically useful
> > supporting more characters would be desireable.
> >
> > Earlier I posted an idea that the characters could be represended by
> > sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> > convenience of using devm_gpiod_get_array() (unless I've missed something).
> 
> It seems you didn't know that the tree for auxdisplay has been changed.
> Can you rebase your stuff on top of
> https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-auxdisplay.git/log/?h=for-next?
> It will reduce your code base by ~50%.

I have just updated the branch so it adds one patch that changes the prototype
of linedisp_register().

> WRT subnodes, you can go with device_for_each_child_node() and
> retrieve gpio array per digit. It means you will have an array of
> arrays of GPIOs.

Btw, as Geert proposed for another 7-segment driver, we might gain from the
display-width-chars property. But I think this property has to be parsed on
top of line display library, no need to have it in each affected driver.

> > [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Feb 26, 2024 at 04:23:15AM +0200, Andy Shevchenko wrote:
> On Sun, Feb 25, 2024 at 11:34 PM Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
> >
> > This series adds a driver for a 7 segment LED display.
> >
> > I'd like to get some feedback on how this could be extended to support >1
> > character. The driver as presented is sufficient for my hardware which only has
> > a single character display but I can see that for this to be generically useful
> > supporting more characters would be desireable.
> >
> > Earlier I posted an idea that the characters could be represended by
> > sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> > convenience of using devm_gpiod_get_array() (unless I've missed something).
> 
> It seems you didn't know that the tree for auxdisplay has been changed.
> Can you rebase your stuff on top of
> https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-auxdisplay.git/log/?h=for-next?
> It will reduce your code base by ~50%.

I have just updated the branch so it adds one patch that changes the prototype
of linedisp_register().

> WRT subnodes, you can go with device_for_each_child_node() and
> retrieve gpio array per digit. It means you will have an array of
> arrays of GPIOs.

Btw, as Geert proposed for another 7-segment driver, we might gain from the
display-width-chars property. But I think this property has to be parsed on
top of line display library, no need to have it in each affected driver.

> > [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/

-- 
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Mon, 26 Feb 2024 10:34:20 +1300, Chris Packham wrote:
> This series adds a driver for a 7 segment LED display.
> 
> I'd like to get some feedback on how this could be extended to support >1
> character. The driver as presented is sufficient for my hardware which only has
> a single character display but I can see that for this to be generically useful
> supporting more characters would be desireable.
> 
> Earlier I posted an idea that the characters could be represended by
> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> convenience of using devm_gpiod_get_array() (unless I've missed something).
> 
> [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/
> 
> 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
> 
>  .../auxdisplay/generic,gpio-7seg.yaml         |  40 +++++
>  .../boot/dts/marvell/armada-385-atl-x530.dts  |  13 +-
>  drivers/auxdisplay/Kconfig                    |   7 +
>  drivers/auxdisplay/Makefile                   |   1 +
>  drivers/auxdisplay/seg-led.c                  | 152 ++++++++++++++++++
>  5 files changed, 212 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
>  create mode 100644 drivers/auxdisplay/seg-led.c
> 
> --
> 2.43.2
> 
> 
> 


My bot found new DT warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y marvell/armada-385-atl-x530.dtb' for 20240225213423.690561-1-chris.packham@alliedtelesis.co.nz:

arch/arm/boot/dts/marvell/armada-385-atl-x530.dtb: soc: internal-regs: {'compatible': ['simple-bus'], '#address-cells': [[1]], '#size-cells': [[1]], 'ranges': [[0, 4026597376, 0, 1048576]], 'sdramc@1400': {'compatible': ['marvell,armada-xp-sdram-controller'], 'reg': [[5120, 1280]]}, 'cache-controller@8000': {'compatible': ['arm,pl310-cache'], 'reg': [[32768, 4096]], 'cache-unified': True, 'cache-level': [[2]], 'arm,double-linefill-incr': [[0]], 'arm,double-linefill-wrap': [[0]], 'arm,double-linefill': [[0]], 'prefetch-data': [[1]]}, 'scu@c000': {'compatible': ['arm,cortex-a9-scu'], 'reg': [[49152, 88]]}, 'timer@c200': {'compatible': ['arm,cortex-a9-global-timer'], 'reg': [[49664, 32]], 'interrupts': [[1, 11, 769]], 'clocks': [[4, 2]]}, 'timer@c600': {'compatible': ['arm,cortex-a9-twd-timer'], 'reg': [[50688, 32]], 'interrupts': [[1, 13, 769]], 'clocks': [[4, 2]]}, 'interrupt-controller@d000': {'compatible': ['arm,cortex-a9-gic'], '#interrupt-cells': [[3]], '#size-cells': [[0]], 'interrupt-controller': True, 'reg': [[53248, 4096], [49408, 256]], 'phandle': [[3]]}, 'i2c@11000': {'compatible': ['marvell,mv78230-a0-i2c', 'marvell,mv64xxx-i2c'], 'reg': [[69632, 32]], '#address-cells': [[1]], '#size-cells': [[0]], 'interrupts': [[0, 2, 4]], 'clocks': [[4, 0]], 'status': ['okay'], 'pinctrl-names': ['default', 'gpio'], 'pinctrl-0': [[5]], 'clock-frequency': [[100000]], 'pinctrl-1': [[6]], 'scl-gpio': [[7, 2, 6]], 'sda-gpio': [[7, 3, 6]], 'mux@71': {'#address-cells': [[1]], '#size-cells': [[0]], 'compatible': ['nxp,pca9544'], 'reg': [[113]], 'i2c-mux-idle-disconnect': True, 'i2c@0': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[0]]}, 'i2c@1': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[1]], 'hwmon@2e': {'compatible': ['adi,adt7476'], 'reg': [[46]]}, 'hwmon@2d': {'compatible': ['adi,adt7476'], 'reg': [[45]]}}, 'i2c@2': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[2]], 'rtc@68': {'compatible': ['dallas,ds1340'], 'reg': [[104]]}}, 'i2c@3': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[3]], 'gpio@20': {'compatible': ['nxp,pca9554'], 'gpio-controller': True, '#gpio-cells': [[2]], 'reg': [[32]], 'phandle': [[23]]}}}}, 'i2c@11100': {'compatible': ['marvell,mv78230-a0-i2c', 'marvell,mv64xxx-i2c'], 'reg': [[69888, 32]], '#address-cells': [[1]], '#size-cells': [[0]], 'interrupts': [[0, 3, 4]], 'clocks': [[4, 0]], 'status': ['disabled']}, 'serial@12000': {'compatible': ['marvell,armada-38x-uart', 'ns16550a'], 'reg': [[73728, 256]], 'reg-shift': [[2]], 'interrupts': [[0, 12, 4]], 'reg-io-width': [[1]], 'clocks': [[4, 0]], 'status': ['okay'], 'pinctrl-names': ['default'], 'pinctrl-0': [[8]]}, 'serial@12100': {'compatible': ['marvell,armada-38x-uart', 'ns16550a'], 'reg': [[73984, 256]], 'reg-shift': [[2]], 'interrupts': [[0, 13, 4]], 'reg-io-width': [[1]], 'clocks': [[4, 0]], 'status': ['disabled']}, 'pinctrl@18000': {'reg': [[98304, 32]], 'compatible': ['marvell,mv88f6820-pinctrl'], 'phandle': [[9]], 'ge-rgmii-pins-0': {'marvell,pins': ['mpp6', 'mpp7', 'mpp8', 'mpp9', 'mpp10', 'mpp11', 'mpp12', 'mpp13', 'mpp14', 'mpp15', 'mpp16', 'mpp17'], 'marvell,function': ['ge0']}, 'ge-rgmii-pins-1': {'marvell,pins': ['mpp21', 'mpp27', 'mpp28', 'mpp29', 'mpp30', 'mpp31', 'mpp32', 'mpp37', 'mpp38', 'mpp39', 'mpp40', 'mpp41'], 'marvell,function': ['ge1']}, 'i2c-pins-0': {'marvell,pins': ['mpp2', 'mpp3'], 'marvell,function': ['i2c0'], 'phandle': [[5]]}, 'mdio-pins': {'marvell,pins': ['mpp4', 'mpp5'], 'marvell,function': ['ge']}, 'ref-clk-pins-0': {'marvell,pins': ['mpp45'], 'marvell,function': ['ref']}, 'ref-clk-pins-1': {'marvell,pins': ['mpp46'], 'marvell,function': ['ref']}, 'spi-pins-0': {'marvell,pins': ['mpp22', 'mpp23', 'mpp24', 'mpp25'], 'marvell,function': ['spi0']}, 'spi-pins-1': {'marvell,pins': ['mpp56', 'mpp57', 'mpp58', 'mpp59'], 'marvell,function': ['spi1'], 'phandle': [[17]]}, 'nand-pins': {'marvell,pins': ['mpp22', 'mpp34', 'mpp23', 'mpp33', 'mpp38', 'mpp28', 'mpp40', 'mpp42', 'mpp35', 'mpp36', 'mpp25', 'mpp30', 'mpp32'], 'marvell,function': ['dev']}, 'nand-rb': {'marvell,pins': ['mpp41'], 'marvell,function': ['nand']}, 'uart-pins-0': {'marvell,pins': ['mpp0', 'mpp1'], 'marvell,function': ['ua0'], 'phandle': [[8]]}, 'uart-pins-1': {'marvell,pins': ['mpp19', 'mpp20'], 'marvell,function': ['ua1']}, 'sdhci-pins': {'marvell,pins': ['mpp48', 'mpp49', 'mpp50', 'mpp52', 'mpp53', 'mpp54', 'mpp55', 'mpp57', 'mpp58', 'mpp59'], 'marvell,function': ['sd0']}, 'sata-pins-0': {'marvell,pins': ['mpp20'], 'marvell,function': ['sata0']}, 'sata-pins-1': {'marvell,pins': ['mpp19'], 'marvell,function': ['sata1']}, 'sata-pins-2': {'marvell,pins': ['mpp47'], 'marvell,function': ['sata2']}, 'sata-pins-3': {'marvell,pins': ['mpp44'], 'marvell,function': ['sata3']}, 'i2s-pins': {'marvell,pins': ['mpp48', 'mpp49', 'mpp50', 'mpp51', 'mpp52', 'mpp53'], 'marvell,function': ['audio']}, 'spdif-pins': {'marvell,pins': ['mpp51'], 'marvell,function': ['audio']}, 'i2c-gpio-pins-0': {'marvell,pins': ['mpp2', 'mpp3'], 'marvell,function': ['gpio'], 'phandle': [[6]]}}, 'gpio@18100': {'compatible': ['marvell,armada-370-gpio', 'marvell,orion-gpio'], 'reg': [[98560, 64], [98752, 8]], 'reg-names': ['gpio', 'pwm'], 'ngpios': [[32]], 'gpio-controller': True, 'gpio-ranges': [[9, 0, 0, 32]], '#gpio-cells': [[2]], '#pwm-cells': [[2]], 'interrupt-controller': True, '#interrupt-cells': [[2]], 'interrupts': [[0, 53, 4], [0, 54, 4], [0, 55, 4], [0, 56, 4]], 'clocks': [[4, 0]], 'phandle': [[7]]}, 'gpio@18140': {'compatible': ['marvell,armada-370-gpio', 'marvell,orion-gpio'], 'reg': [[98624, 64], [98760, 8]], 'reg-names': ['gpio', 'pwm'], 'ngpios': [[28]], 'gpio-controller': True, 'gpio-ranges': [[9, 0, 32, 28]], '#gpio-cells': [[2]], '#pwm-cells': [[2]], 'interrupt-controller': True, '#interrupt-cells': [[2]], 'interrupts': [[0, 58, 4], [0, 59, 4], [0, 60, 4], [0, 61, 4]], 'clocks': [[4, 0]], 'phandle': [[19]]}, 'system-controller@18200': {'compatible': ['marvell,armada-380-system-controller', 'marvell,armada-370-xp-system-controller'], 'reg': [[98816, 256]]}, 'clock-gating-control@18220': {'compatible': ['marvell,armada-380-gating-clock'], 'reg': [[98848, 4]], 'clocks': [[4, 0]], '#clock-cells': [[1]], 'phandle': [[11]]}, 'phy@18300': {'compatible': ['marvell,armada-380-comphy'], 'reg-names': ['comphy', 'conf'], 'reg': [[99072, 256], [99424, 4]], '#address-cells': [[1]], '#size-cells': [[0]], 'phy@0': {'reg': [[0]], '#phy-cells': [[1]]}, 'phy@1': {'reg': [[1]], '#phy-cells': [[1]]}, 'phy@2': {'reg': [[2]], '#phy-cells': [[1]]}, 'phy@3': {'reg': [[3]], '#phy-cells': [[1]]}, 'phy@4': {'reg': [[4]], '#phy-cells': [[1]]}, 'phy@5': {'reg': [[5]], '#phy-cells': [[1]]}}, 'mvebu-sar@18600': {'compatible': ['marvell,armada-380-core-clock'], 'reg': [[99840, 4]], '#clock-cells': [[1]], 'phandle': [[4]]}, 'mbus-controller@20000': {'compatible': ['marvell,mbus-controller'], 'reg': [[131072, 256], [131456, 32], [131664, 8]], 'phandle': [[2]]}, 'interrupt-controller@20a00': {'compatible': ['marvell,mpic'], 'reg': [[133632, 720], [135280, 88]], '#interrupt-cells': [[1]], '#size-cells': [[1]], 'interrupt-controller': True, 'msi-controller': True, 'interrupts': [[1, 15, 4]], 'phandle': [[1]]}, 'timer@20300': {'compatible': ['marvell,armada-380-timer', 'marvell,armada-xp-timer'], 'reg': [[131840, 48], [135232, 48]], 'interrupts-extended': [[3, 0, 8, 4], [3, 0, 9, 4], [3, 0, 10, 4], [3, 0, 11, 4], [1, 5], [1, 6]], 'clocks': [[4, 2], [10]], 'clock-names': ['nbclk', 'fixed']}, 'watchdog@20300': {'compatible': ['marvell,armada-380-wdt'], 'reg': [[131840, 52], [132868, 4], [98912, 4]], 'clocks': [[4, 2], [10]], 'clock-names': ['nbclk', 'fixed'], 'interrupts-extended': [[3, 0, 64, 4], [3, 0, 9, 4]]}, 'cpurst@20800': {'compatible': ['marvell,armada-370-cpu-reset'], 'reg': [[133120, 16]]}, 'mpcore-soc-ctrl@20d20': {'compatible': ['marvell,armada-380-mpcore-soc-ctrl'], 'reg': [[134432, 108]]}, 'coherency-fabric@21010': {'compatible': ['marvell,armada-380-coherency-fabric'], 'reg': [[135184, 28]]}, 'pmsu@22000': {'compatible': ['marvell,armada-380-pmsu'], 'reg': [[139264, 4096]]}, 'ethernet@70000': {'compatible': ['marvell,armada-370-neta'], 'reg': [[458752, 16384]], 'interrupts-extended': [[1, 8]], 'clocks': [[11, 4]], 'tx-csum-limit': [[9800]], 'status': ['disabled']}, 'ethernet@30000': {'compatible': ['marvell,armada-370-neta'], 'reg': [[196608, 16384]], 'interrupts-extended': [[1, 10]], 'clocks': [[11, 3]], 'status': ['disabled']}, 'ethernet@34000': {'compatible': ['marvell,armada-370-neta'], 'reg': [[212992, 16384]], 'interrupts-extended': [[1, 12]], 'clocks': [[11, 2]], 'status': ['disabled']}, 'usb@58000': {'compatible': ['marvell,orion-ehci'], 'reg': [[360448, 1280]], 'interrupts': [[0, 18, 4]], 'clocks': [[11, 18]], 'status': ['okay']}, 'xor@60800': {'compatible': ['marvell,armada-380-xor', 'marvell,orion-xor'], 'reg': [[395264, 256], [395776, 256]], 'clocks': [[11, 22]], 'status': ['okay'], 'xor00': {'interrupts': [[0, 22, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True}, 'xor01': {'interrupts': [[0, 23, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True, 'dmacap,memset': True}}, 'xor@60900': {'compatible': ['marvell,armada-380-xor', 'marvell,orion-xor'], 'reg': [[395520, 256], [396032, 256]], 'clocks': [[11, 28]], 'status': ['okay'], 'xor10': {'interrupts': [[0, 65, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True}, 'xor11': {'interrupts': [[0, 66, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True, 'dmacap,memset': True}}, 'mdio@72004': {'#address-cells': [[1]], '#size-cells': [[0]], 'compatible': ['marvell,orion-mdio'], 'reg': [[466948, 4]], 'clocks': [[11, 4]]}, 'crypto@90000': {'compatible': ['marvell,armada-38x-crypto'], 'reg': [[589824, 65536]], 'reg-names': ['regs'], 'interrupts': [[0, 19, 4], [0, 20, 4]], 'clocks': [[11, 23], [11, 21], [11, 14], [11, 16]], 'clock-names': ['cesa0', 'cesa1', 'cesaz0', 'cesaz1'], 'marvell,crypto-srams': [[12, 13]], 'marvell,crypto-sram-size': [[2048]]}, 'rtc@a3800': {'compatible': ['marvell,armada-380-rtc'], 'reg': [[669696, 32], [99488, 12]], 'reg-names': ['rtc', 'rtc-soc'], 'interrupts': [[0, 21, 4]]}, 'sata@a8000': {'compatible': ['marvell,armada-380-ahci'], 'reg': [[688128, 8192]], 'interrupts': [[0, 26, 4]], 'clocks': [[11, 15]], 'status': ['disabled']}, 'bm@c8000': {'compatible': ['marvell,armada-380-neta-bm'], 'reg': [[819200, 172]], 'clocks': [[11, 13]], 'internal-mem': [[14]], 'status': ['disabled']}, 'sata@e0000': {'compatible': ['marvell,armada-380-ahci'], 'reg': [[917504, 8192]], 'interrupts': [[0, 28, 4]], 'clocks': [[11, 30]], 'status': ['disabled']}, 'clock@e4250': {'compatible': ['marvell,armada-380-corediv-clock'], 'reg': [[934480, 12]], '#clock-cells': [[1]], 'clocks': [[15]], 'clock-output-names': ['nand'], 'phandle': [[16]]}, 'thermal@e8078': {'compatible': ['marvell,armada380-thermal'], 'reg': [[934008, 4], [934000, 8]], 'status': ['okay']}, 'nand-controller@d0000': {'compatible': ['marvell,armada370-nand-controller'], 'reg': [[851968, 84]], '#address-cells': [[1]], '#size-cells': [[0]], 'interrupts': [[0, 84, 4]], 'clocks': [[16, 0]], 'status': ['okay'], 'nand@0': {'reg': [[0]], 'label': ['pxa3xx_nand-0'], 'nand-rb': [[0]], 'nand-on-flash-bbt': True, 'nand-ecc-strength': [[4]], 'nand-ecc-step-size': [[512]], 'marvell,nand-enable-arbiter': True, 'partitions': {'compatible': ['fixed-partitions'], '#address-cells': [[1]], '#size-cells': [[1]], 'partition@0': {'reg': [[0, 251658240]], 'label': ['user']}, 'partition@f000000': {'reg': [[251658240, 8388608]], 'label': ['errlog']}, 'partition@f800000': {'reg': [[260046848, 8388608]], 'label': ['nand-bbt']}}}}, 'sdhci@d8000': {'compatible': ['marvell,armada-380-sdhci'], 'reg-names': ['sdhci', 'mbus', 'conf-sdio3'], 'reg': [[884736, 4096], [901120, 256], [99412, 4]], 'interrupts': [[0, 25, 4]], 'clocks': [[11, 17]], 'mrvl,clk-delay-cycles': [[31]], 'status': ['disabled']}, 'audio-controller@e8000': {'#sound-dai-cells': [[1]], 'compatible': ['marvell,armada-380-audio'], 'reg': [[950272, 16384], [99344, 12], [98820, 4]], 'reg-names': ['i2s_regs', 'pll_regs', 'soc_ctrl'], 'interrupts': [[0, 75, 4]], 'clocks': [[11, 0]], 'clock-names': ['internal'], 'status': ['disabled']}, 'usb3@f0000': {'compatible': ['marvell,armada-380-xhci'], 'reg': [[983040, 16384], [999424, 16384]], 'interrupts': [[0, 16, 4]], 'clocks': [[11, 9]], 'status': ['disabled']}, 'usb3@f8000': {'compatible': ['marvell,armada-380-xhci'], 'reg': [[1015808, 16384], [1032192, 16384]], 'interrupts': [[0, 17, 4]], 'clocks': [[11, 10]], 'status': ['disabled']}} should not be valid under {'type': 'object'}
	from schema $id: http://devicetree.org/schemas/simple-bus.yaml#

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

[Rob Herring]

On Mon, 26 Feb 2024 10:34:20 +1300, Chris Packham wrote:
> This series adds a driver for a 7 segment LED display.
> 
> I'd like to get some feedback on how this could be extended to support >1
> character. The driver as presented is sufficient for my hardware which only has
> a single character display but I can see that for this to be generically useful
> supporting more characters would be desireable.
> 
> Earlier I posted an idea that the characters could be represended by
> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> convenience of using devm_gpiod_get_array() (unless I've missed something).
> 
> [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/
> 
> 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
> 
>  .../auxdisplay/generic,gpio-7seg.yaml         |  40 +++++
>  .../boot/dts/marvell/armada-385-atl-x530.dts  |  13 +-
>  drivers/auxdisplay/Kconfig                    |   7 +
>  drivers/auxdisplay/Makefile                   |   1 +
>  drivers/auxdisplay/seg-led.c                  | 152 ++++++++++++++++++
>  5 files changed, 212 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
>  create mode 100644 drivers/auxdisplay/seg-led.c
> 
> --
> 2.43.2
> 
> 
> 


My bot found new DT warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y marvell/armada-385-atl-x530.dtb' for 20240225213423.690561-1-chris.packham@alliedtelesis.co.nz:

arch/arm/boot/dts/marvell/armada-385-atl-x530.dtb: soc: internal-regs: {'compatible': ['simple-bus'], '#address-cells': [[1]], '#size-cells': [[1]], 'ranges': [[0, 4026597376, 0, 1048576]], 'sdramc@1400': {'compatible': ['marvell,armada-xp-sdram-controller'], 'reg': [[5120, 1280]]}, 'cache-controller@8000': {'compatible': ['arm,pl310-cache'], 'reg': [[32768, 4096]], 'cache-unified': True, 'cache-level': [[2]], 'arm,double-linefill-incr': [[0]], 'arm,double-linefill-wrap': [[0]], 'arm,double-linefill': [[0]], 'prefetch-data': [[1]]}, 'scu@c000': {'compatible': ['arm,cortex-a9-scu'], 'reg': [[49152, 88]]}, 'timer@c200': {'compatible': ['arm,cortex-a9-global-timer'], 'reg': [[49664, 32]], 'interrupts': [[1, 11, 769]], 'clocks': [[4, 2]]}, 'timer@c600': {'compatible': ['arm,cortex-a9-twd-timer'], 'reg': [[50688, 32]], 'interrupts': [[1, 13, 769]], 'clocks': [[4, 2]]}, 'interrupt-controller@d000': {'compatible': ['arm,cortex-a9-gic'], '#interrupt-cells': [[3]], '#size-cells': [[0]], 'inte
 rrupt-controller': True, 'reg': [[53248, 4096], [49408, 256]], 'phandle': [[3]]}, 'i2c@11000': {'compatible': ['marvell,mv78230-a0-i2c', 'marvell,mv64xxx-i2c'], 'reg': [[69632, 32]], '#address-cells': [[1]], '#size-cells': [[0]], 'interrupts': [[0, 2, 4]], 'clocks': [[4, 0]], 'status': ['okay'], 'pinctrl-names': ['default', 'gpio'], 'pinctrl-0': [[5]], 'clock-frequency': [[100000]], 'pinctrl-1': [[6]], 'scl-gpio': [[7, 2, 6]], 'sda-gpio': [[7, 3, 6]], 'mux@71': {'#address-cells': [[1]], '#size-cells': [[0]], 'compatible': ['nxp,pca9544'], 'reg': [[113]], 'i2c-mux-idle-disconnect': True, 'i2c@0': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[0]]}, 'i2c@1': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[1]], 'hwmon@2e': {'compatible': ['adi,adt7476'], 'reg': [[46]]}, 'hwmon@2d': {'compatible': ['adi,adt7476'], 'reg': [[45]]}}, 'i2c@2': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[2]], 'rtc@68': {'compatible': ['dallas,ds1340'], 'reg': [[104]]}}, 'i2c@3
 ': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[3]], 'gpio@20': {'compatible': ['nxp,pca9554'], 'gpio-controller': True, '#gpio-cells': [[2]], 'reg': [[32]], 'phandle': [[23]]}}}}, 'i2c@11100': {'compatible': ['marvell,mv78230-a0-i2c', 'marvell,mv64xxx-i2c'], 'reg': [[69888, 32]], '#address-cells': [[1]], '#size-cells': [[0]], 'interrupts': [[0, 3, 4]], 'clocks': [[4, 0]], 'status': ['disabled']}, 'serial@12000': {'compatible': ['marvell,armada-38x-uart', 'ns16550a'], 'reg': [[73728, 256]], 'reg-shift': [[2]], 'interrupts': [[0, 12, 4]], 'reg-io-width': [[1]], 'clocks': [[4, 0]], 'status': ['okay'], 'pinctrl-names': ['default'], 'pinctrl-0': [[8]]}, 'serial@12100': {'compatible': ['marvell,armada-38x-uart', 'ns16550a'], 'reg': [[73984, 256]], 'reg-shift': [[2]], 'interrupts': [[0, 13, 4]], 'reg-io-width': [[1]], 'clocks': [[4, 0]], 'status': ['disabled']}, 'pinctrl@18000': {'reg': [[98304, 32]], 'compatible': ['marvell,mv88f6820-pinctrl'], 'phandle': [[9]], 'ge-rgmii-pin
 s-0': {'marvell,pins': ['mpp6', 'mpp7', 'mpp8', 'mpp9', 'mpp10', 'mpp11', 'mpp12', 'mpp13', 'mpp14', 'mpp15', 'mpp16', 'mpp17'], 'marvell,function': ['ge0']}, 'ge-rgmii-pins-1': {'marvell,pins': ['mpp21', 'mpp27', 'mpp28', 'mpp29', 'mpp30', 'mpp31', 'mpp32', 'mpp37', 'mpp38', 'mpp39', 'mpp40', 'mpp41'], 'marvell,function': ['ge1']}, 'i2c-pins-0': {'marvell,pins': ['mpp2', 'mpp3'], 'marvell,function': ['i2c0'], 'phandle': [[5]]}, 'mdio-pins': {'marvell,pins': ['mpp4', 'mpp5'], 'marvell,function': ['ge']}, 'ref-clk-pins-0': {'marvell,pins': ['mpp45'], 'marvell,function': ['ref']}, 'ref-clk-pins-1': {'marvell,pins': ['mpp46'], 'marvell,function': ['ref']}, 'spi-pins-0': {'marvell,pins': ['mpp22', 'mpp23', 'mpp24', 'mpp25'], 'marvell,function': ['spi0']}, 'spi-pins-1': {'marvell,pins': ['mpp56', 'mpp57', 'mpp58', 'mpp59'], 'marvell,function': ['spi1'], 'phandle': [[17]]}, 'nand-pins': {'marvell,pins': ['mpp22', 'mpp34', 'mpp23', 'mpp33', 'mpp38', 'mpp28', 'mpp40', 'mpp42', 'mpp35', 'mpp
 36', 'mpp25', 'mpp30', 'mpp32'], 'marvell,function': ['dev']}, 'nand-rb': {'marvell,pins': ['mpp41'], 'marvell,function': ['nand']}, 'uart-pins-0': {'marvell,pins': ['mpp0', 'mpp1'], 'marvell,function': ['ua0'], 'phandle': [[8]]}, 'uart-pins-1': {'marvell,pins': ['mpp19', 'mpp20'], 'marvell,function': ['ua1']}, 'sdhci-pins': {'marvell,pins': ['mpp48', 'mpp49', 'mpp50', 'mpp52', 'mpp53', 'mpp54', 'mpp55', 'mpp57', 'mpp58', 'mpp59'], 'marvell,function': ['sd0']}, 'sata-pins-0': {'marvell,pins': ['mpp20'], 'marvell,function': ['sata0']}, 'sata-pins-1': {'marvell,pins': ['mpp19'], 'marvell,function': ['sata1']}, 'sata-pins-2': {'marvell,pins': ['mpp47'], 'marvell,function': ['sata2']}, 'sata-pins-3': {'marvell,pins': ['mpp44'], 'marvell,function': ['sata3']}, 'i2s-pins': {'marvell,pins': ['mpp48', 'mpp49', 'mpp50', 'mpp51', 'mpp52', 'mpp53'], 'marvell,function': ['audio']}, 'spdif-pins': {'marvell,pins': ['mpp51'], 'marvell,function': ['audio']}, 'i2c-gpio-pins-0': {'marvell,pins': ['mp
 p2', 'mpp3'], 'marvell,function': ['gpio'], 'phandle': [[6]]}}, 'gpio@18100': {'compatible': ['marvell,armada-370-gpio', 'marvell,orion-gpio'], 'reg': [[98560, 64], [98752, 8]], 'reg-names': ['gpio', 'pwm'], 'ngpios': [[32]], 'gpio-controller': True, 'gpio-ranges': [[9, 0, 0, 32]], '#gpio-cells': [[2]], '#pwm-cells': [[2]], 'interrupt-controller': True, '#interrupt-cells': [[2]], 'interrupts': [[0, 53, 4], [0, 54, 4], [0, 55, 4], [0, 56, 4]], 'clocks': [[4, 0]], 'phandle': [[7]]}, 'gpio@18140': {'compatible': ['marvell,armada-370-gpio', 'marvell,orion-gpio'], 'reg': [[98624, 64], [98760, 8]], 'reg-names': ['gpio', 'pwm'], 'ngpios': [[28]], 'gpio-controller': True, 'gpio-ranges': [[9, 0, 32, 28]], '#gpio-cells': [[2]], '#pwm-cells': [[2]], 'interrupt-controller': True, '#interrupt-cells': [[2]], 'interrupts': [[0, 58, 4], [0, 59, 4], [0, 60, 4], [0, 61, 4]], 'clocks': [[4, 0]], 'phandle': [[19]]}, 'system-controller@18200': {'compatible': ['marvell,armada-380-system-controller', 'mar
 vell,armada-370-xp-system-controller'], 'reg': [[98816, 256]]}, 'clock-gating-control@18220': {'compatible': ['marvell,armada-380-gating-clock'], 'reg': [[98848, 4]], 'clocks': [[4, 0]], '#clock-cells': [[1]], 'phandle': [[11]]}, 'phy@18300': {'compatible': ['marvell,armada-380-comphy'], 'reg-names': ['comphy', 'conf'], 'reg': [[99072, 256], [99424, 4]], '#address-cells': [[1]], '#size-cells': [[0]], 'phy@0': {'reg': [[0]], '#phy-cells': [[1]]}, 'phy@1': {'reg': [[1]], '#phy-cells': [[1]]}, 'phy@2': {'reg': [[2]], '#phy-cells': [[1]]}, 'phy@3': {'reg': [[3]], '#phy-cells': [[1]]}, 'phy@4': {'reg': [[4]], '#phy-cells': [[1]]}, 'phy@5': {'reg': [[5]], '#phy-cells': [[1]]}}, 'mvebu-sar@18600': {'compatible': ['marvell,armada-380-core-clock'], 'reg': [[99840, 4]], '#clock-cells': [[1]], 'phandle': [[4]]}, 'mbus-controller@20000': {'compatible': ['marvell,mbus-controller'], 'reg': [[131072, 256], [131456, 32], [131664, 8]], 'phandle': [[2]]}, 'interrupt-controller@20a00': {'compatible': 
 ['marvell,mpic'], 'reg': [[133632, 720], [135280, 88]], '#interrupt-cells': [[1]], '#size-cells': [[1]], 'interrupt-controller': True, 'msi-controller': True, 'interrupts': [[1, 15, 4]], 'phandle': [[1]]}, 'timer@20300': {'compatible': ['marvell,armada-380-timer', 'marvell,armada-xp-timer'], 'reg': [[131840, 48], [135232, 48]], 'interrupts-extended': [[3, 0, 8, 4], [3, 0, 9, 4], [3, 0, 10, 4], [3, 0, 11, 4], [1, 5], [1, 6]], 'clocks': [[4, 2], [10]], 'clock-names': ['nbclk', 'fixed']}, 'watchdog@20300': {'compatible': ['marvell,armada-380-wdt'], 'reg': [[131840, 52], [132868, 4], [98912, 4]], 'clocks': [[4, 2], [10]], 'clock-names': ['nbclk', 'fixed'], 'interrupts-extended': [[3, 0, 64, 4], [3, 0, 9, 4]]}, 'cpurst@20800': {'compatible': ['marvell,armada-370-cpu-reset'], 'reg': [[133120, 16]]}, 'mpcore-soc-ctrl@20d20': {'compatible': ['marvell,armada-380-mpcore-soc-ctrl'], 'reg': [[134432, 108]]}, 'coherency-fabric@21010': {'compatible': ['marvell,armada-380-coherency-fabric'], 'reg'
 : [[135184, 28]]}, 'pmsu@22000': {'compatible': ['marvell,armada-380-pmsu'], 'reg': [[139264, 4096]]}, 'ethernet@70000': {'compatible': ['marvell,armada-370-neta'], 'reg': [[458752, 16384]], 'interrupts-extended': [[1, 8]], 'clocks': [[11, 4]], 'tx-csum-limit': [[9800]], 'status': ['disabled']}, 'ethernet@30000': {'compatible': ['marvell,armada-370-neta'], 'reg': [[196608, 16384]], 'interrupts-extended': [[1, 10]], 'clocks': [[11, 3]], 'status': ['disabled']}, 'ethernet@34000': {'compatible': ['marvell,armada-370-neta'], 'reg': [[212992, 16384]], 'interrupts-extended': [[1, 12]], 'clocks': [[11, 2]], 'status': ['disabled']}, 'usb@58000': {'compatible': ['marvell,orion-ehci'], 'reg': [[360448, 1280]], 'interrupts': [[0, 18, 4]], 'clocks': [[11, 18]], 'status': ['okay']}, 'xor@60800': {'compatible': ['marvell,armada-380-xor', 'marvell,orion-xor'], 'reg': [[395264, 256], [395776, 256]], 'clocks': [[11, 22]], 'status': ['okay'], 'xor00': {'interrupts': [[0, 22, 4]], 'dmacap,memcpy': Tru
 e, 'dmacap,xor': True}, 'xor01': {'interrupts': [[0, 23, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True, 'dmacap,memset': True}}, 'xor@60900': {'compatible': ['marvell,armada-380-xor', 'marvell,orion-xor'], 'reg': [[395520, 256], [396032, 256]], 'clocks': [[11, 28]], 'status': ['okay'], 'xor10': {'interrupts': [[0, 65, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True}, 'xor11': {'interrupts': [[0, 66, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True, 'dmacap,memset': True}}, 'mdio@72004': {'#address-cells': [[1]], '#size-cells': [[0]], 'compatible': ['marvell,orion-mdio'], 'reg': [[466948, 4]], 'clocks': [[11, 4]]}, 'crypto@90000': {'compatible': ['marvell,armada-38x-crypto'], 'reg': [[589824, 65536]], 'reg-names': ['regs'], 'interrupts': [[0, 19, 4], [0, 20, 4]], 'clocks': [[11, 23], [11, 21], [11, 14], [11, 16]], 'clock-names': ['cesa0', 'cesa1', 'cesaz0', 'cesaz1'], 'marvell,crypto-srams': [[12, 13]], 'marvell,crypto-sram-size': [[2048]]}, 'rtc@a3800': {'compatible': ['marvell,armada
 -380-rtc'], 'reg': [[669696, 32], [99488, 12]], 'reg-names': ['rtc', 'rtc-soc'], 'interrupts': [[0, 21, 4]]}, 'sata@a8000': {'compatible': ['marvell,armada-380-ahci'], 'reg': [[688128, 8192]], 'interrupts': [[0, 26, 4]], 'clocks': [[11, 15]], 'status': ['disabled']}, 'bm@c8000': {'compatible': ['marvell,armada-380-neta-bm'], 'reg': [[819200, 172]], 'clocks': [[11, 13]], 'internal-mem': [[14]], 'status': ['disabled']}, 'sata@e0000': {'compatible': ['marvell,armada-380-ahci'], 'reg': [[917504, 8192]], 'interrupts': [[0, 28, 4]], 'clocks': [[11, 30]], 'status': ['disabled']}, 'clock@e4250': {'compatible': ['marvell,armada-380-corediv-clock'], 'reg': [[934480, 12]], '#clock-cells': [[1]], 'clocks': [[15]], 'clock-output-names': ['nand'], 'phandle': [[16]]}, 'thermal@e8078': {'compatible': ['marvell,armada380-thermal'], 'reg': [[934008, 4], [934000, 8]], 'status': ['okay']}, 'nand-controller@d0000': {'compatible': ['marvell,armada370-nand-controller'], 'reg': [[851968, 84]], '#address-ce
 lls': [[1]], '#size-cells': [[0]], 'interrupts': [[0, 84, 4]], 'clocks': [[16, 0]], 'status': ['okay'], 'nand@0': {'reg': [[0]], 'label': ['pxa3xx_nand-0'], 'nand-rb': [[0]], 'nand-on-flash-bbt': True, 'nand-ecc-strength': [[4]], 'nand-ecc-step-size': [[512]], 'marvell,nand-enable-arbiter': True, 'partitions': {'compatible': ['fixed-partitions'], '#address-cells': [[1]], '#size-cells': [[1]], 'partition@0': {'reg': [[0, 251658240]], 'label': ['user']}, 'partition@f000000': {'reg': [[251658240, 8388608]], 'label': ['errlog']}, 'partition@f800000': {'reg': [[260046848, 8388608]], 'label': ['nand-bbt']}}}}, 'sdhci@d8000': {'compatible': ['marvell,armada-380-sdhci'], 'reg-names': ['sdhci', 'mbus', 'conf-sdio3'], 'reg': [[884736, 4096], [901120, 256], [99412, 4]], 'interrupts': [[0, 25, 4]], 'clocks': [[11, 17]], 'mrvl,clk-delay-cycles': [[31]], 'status': ['disabled']}, 'audio-controller@e8000': {'#sound-dai-cells': [[1]], 'compatible': ['marvell,armada-380-audio'], 'reg': [[950272, 1638
 4], [99344, 12], [98820, 4]], 'reg-names': ['i2s_regs', 'pll_regs', 'soc_ctrl'], 'interrupts': [[0, 75, 4]], 'clocks': [[11, 0]], 'clock-names': ['internal'], 'status': ['disabled']}, 'usb3@f0000': {'compatible': ['marvell,armada-380-xhci'], 'reg': [[983040, 16384], [999424, 16384]], 'interrupts': [[0, 16, 4]], 'clocks': [[11, 9]], 'status': ['disabled']}, 'usb3@f8000': {'compatible': ['marvell,armada-380-xhci'], 'reg': [[1015808, 16384], [1032192, 16384]], 'interrupts': [[0, 17, 4]], 'clocks': [[11, 10]], 'status': ['disabled']}} should not be valid under {'type': 'object'}
	from schema $id: http://devicetree.org/schemas/simple-bus.yaml#






_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Chris Packham]

Hi Alex,

On 27/02/24 05:04, Alexander Dahl wrote:
> Hello Chris,
>
> Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham:
>> This series adds a driver for a 7 segment LED display.
>>
>> I'd like to get some feedback on how this could be extended to support >1
>> character. The driver as presented is sufficient for my hardware which only has
>> a single character display but I can see that for this to be generically useful
>> supporting more characters would be desireable.
>>
>> Earlier I posted an idea that the characters could be represended by
>> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
>> convenience of using devm_gpiod_get_array() (unless I've missed something).
>>
>> [1] - https://scanmail.trustwave.com/?c=20988&d=trbc5eARVo-5gepRnwbAKbQmiGk1bOSpqZDQx9bx7w&u=https%3a%2f%2flore%2ekernel%2eorg%2flkml%2f2a8d19ee-b18b-4b7c-869f-7d601cea30b6%40alliedtelesis%2eco%2enz%2f
> Read that thread out of curiosity and I'm sorry if I'm late to the
> party, but I wondered why this is limited to LEDs connected to GPIOs?
>
> Would it be possible to somehow stack this on top of some existing
> LEDs?  I mean you could wire a 7 segment device to almost any LED
> driver IC with enough channels, couldn't you?

Mainly because the GPIO version is the hardware I have. I do wonder how 
this might work with something like the pca9551 which really is just a 
fancy version of the pca9554 on my board. A naive implementation would 
be to configure all the pca9551 pins as GPIOs and use what I have as-is. 
Making a line display out of LED triggers might be another way of doing 
it but not something I really want to pursue.

>
> Greets
> Alex
>
>> 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
>>
>>   .../auxdisplay/generic,gpio-7seg.yaml         |  40 +++++
>>   .../boot/dts/marvell/armada-385-atl-x530.dts  |  13 +-
>>   drivers/auxdisplay/Kconfig                    |   7 +
>>   drivers/auxdisplay/Makefile                   |   1 +
>>   drivers/auxdisplay/seg-led.c                  | 152 ++++++++++++++++++
>>   5 files changed, 212 insertions(+), 1 deletion(-)
>>   create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
>>   create mode 100644 drivers/auxdisplay/seg-led.c
>>
>> -- 
>> 2.43.2
>>
>>

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

[Chris Packham]

Hi Alex,

On 27/02/24 05:04, Alexander Dahl wrote:
> Hello Chris,
>
> Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham:
>> This series adds a driver for a 7 segment LED display.
>>
>> I'd like to get some feedback on how this could be extended to support >1
>> character. The driver as presented is sufficient for my hardware which only has
>> a single character display but I can see that for this to be generically useful
>> supporting more characters would be desireable.
>>
>> Earlier I posted an idea that the characters could be represended by
>> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
>> convenience of using devm_gpiod_get_array() (unless I've missed something).
>>
>> [1] - https://scanmail.trustwave.com/?c=20988&d=trbc5eARVo-5gepRnwbAKbQmiGk1bOSpqZDQx9bx7w&u=https%3a%2f%2flore%2ekernel%2eorg%2flkml%2f2a8d19ee-b18b-4b7c-869f-7d601cea30b6%40alliedtelesis%2eco%2enz%2f
> Read that thread out of curiosity and I'm sorry if I'm late to the
> party, but I wondered why this is limited to LEDs connected to GPIOs?
>
> Would it be possible to somehow stack this on top of some existing
> LEDs?  I mean you could wire a 7 segment device to almost any LED
> driver IC with enough channels, couldn't you?

Mainly because the GPIO version is the hardware I have. I do wonder how 
this might work with something like the pca9551 which really is just a 
fancy version of the pca9554 on my board. A naive implementation would 
be to configure all the pca9551 pins as GPIOs and use what I have as-is. 
Making a line display out of LED triggers might be another way of doing 
it but not something I really want to pursue.

>
> Greets
> Alex
>
>> 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
>>
>>   .../auxdisplay/generic,gpio-7seg.yaml         |  40 +++++
>>   .../boot/dts/marvell/armada-385-atl-x530.dts  |  13 +-
>>   drivers/auxdisplay/Kconfig                    |   7 +
>>   drivers/auxdisplay/Makefile                   |   1 +
>>   drivers/auxdisplay/seg-led.c                  | 152 ++++++++++++++++++
>>   5 files changed, 212 insertions(+), 1 deletion(-)
>>   create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
>>   create mode 100644 drivers/auxdisplay/seg-led.c
>>
>> -- 
>> 2.43.2
>>
>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Chris Packham]

On 26/02/24 15:23, Andy Shevchenko wrote:
> On Sun, Feb 25, 2024 at 11:34 PM Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
>> This series adds a driver for a 7 segment LED display.
>>
>> I'd like to get some feedback on how this could be extended to support >1
>> character. The driver as presented is sufficient for my hardware which only has
>> a single character display but I can see that for this to be generically useful
>> supporting more characters would be desireable.
>>
>> Earlier I posted an idea that the characters could be represended by
>> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
>> convenience of using devm_gpiod_get_array() (unless I've missed something).
> It seems you didn't know that the tree for auxdisplay has been changed.
> Can you rebase your stuff on top of
> https://scanmail.trustwave.com/?c=20988&d=vfbb5fnU59kvIREfdD-21Pab30bpMpuTM2Ipv28now&u=https%3a%2f%2fgit%2ekernel%2eorg%2fpub%2fscm%2flinux%2fkernel%2fgit%2fandy%2flinux-auxdisplay%2egit%2flog%2f%3fh%3dfor-next%3f
> It will reduce your code base by ~50%.
>
> WRT subnodes, you can go with device_for_each_child_node() and
> retrieve gpio array per digit. It means you will have an array of
> arrays of GPIOs.

So would the following work?

     count = device_get_child_node_count(dev);
     struct gpio_descs **chars  = devm_kzalloc(dev, sizeof(*chars) * 
count, GFP_KERNEL);

     i = 0;
     device_for_each_child_node(dev, child) {
         chars[i] = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
     }

I haven't used the child. The devm_gpiod_get_array() will be looking at 
the fwnode of the parent.

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

[Chris Packham]

On 26/02/24 15:23, Andy Shevchenko wrote:
> On Sun, Feb 25, 2024 at 11:34 PM Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
>> This series adds a driver for a 7 segment LED display.
>>
>> I'd like to get some feedback on how this could be extended to support >1
>> character. The driver as presented is sufficient for my hardware which only has
>> a single character display but I can see that for this to be generically useful
>> supporting more characters would be desireable.
>>
>> Earlier I posted an idea that the characters could be represended by
>> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
>> convenience of using devm_gpiod_get_array() (unless I've missed something).
> It seems you didn't know that the tree for auxdisplay has been changed.
> Can you rebase your stuff on top of
> https://scanmail.trustwave.com/?c=20988&d=vfbb5fnU59kvIREfdD-21Pab30bpMpuTM2Ipv28now&u=https%3a%2f%2fgit%2ekernel%2eorg%2fpub%2fscm%2flinux%2fkernel%2fgit%2fandy%2flinux-auxdisplay%2egit%2flog%2f%3fh%3dfor-next%3f
> It will reduce your code base by ~50%.
>
> WRT subnodes, you can go with device_for_each_child_node() and
> retrieve gpio array per digit. It means you will have an array of
> arrays of GPIOs.

So would the following work?

     count = device_get_child_node_count(dev);
     struct gpio_descs **chars  = devm_kzalloc(dev, sizeof(*chars) * 
count, GFP_KERNEL);

     i = 0;
     device_for_each_child_node(dev, child) {
         chars[i] = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
     }

I haven't used the child. The devm_gpiod_get_array() will be looking at 
the fwnode of the parent.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Tue, Feb 27, 2024 at 2:52 AM Chris Packham
<Chris.Packham@alliedtelesis.co.nz> wrote:
> On 26/02/24 15:23, Andy Shevchenko wrote:
> > On Sun, Feb 25, 2024 at 11:34 PM Chris Packham
> > <chris.packham@alliedtelesis.co.nz> wrote:
> >> This series adds a driver for a 7 segment LED display.
> >>
> >> I'd like to get some feedback on how this could be extended to support >1
> >> character. The driver as presented is sufficient for my hardware which only has
> >> a single character display but I can see that for this to be generically useful
> >> supporting more characters would be desireable.
> >>
> >> Earlier I posted an idea that the characters could be represended by
> >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> >> convenience of using devm_gpiod_get_array() (unless I've missed something).
> > It seems you didn't know that the tree for auxdisplay has been changed.
> > Can you rebase your stuff on top of
> > https://scanmail.trustwave.com/?c=20988&d=vfbb5fnU59kvIREfdD-21Pab30bpMpuTM2Ipv28now&u=https%3a%2f%2fgit%2ekernel%2eorg%2fpub%2fscm%2flinux%2fkernel%2fgit%2fandy%2flinux-auxdisplay%2egit%2flog%2f%3fh%3dfor-next%3f
> > It will reduce your code base by ~50%.
> >
> > WRT subnodes, you can go with device_for_each_child_node() and
> > retrieve gpio array per digit. It means you will have an array of
> > arrays of GPIOs.
>
> So would the following work?
>
>      count = device_get_child_node_count(dev);
>      struct gpio_descs **chars  = devm_kzalloc(dev, sizeof(*chars) *
> count, GFP_KERNEL);
>
>      i = 0;
>      device_for_each_child_node(dev, child) {
>          chars[i] = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);

I see what you meant earlier.
This should be devm_fwnode_gpiod_get_index(), but we lack the _array
variant of it...

Dunno what to do, maybe adding the _array variant is the good move,
maybe something else.

>      }
>
> I haven't used the child. The devm_gpiod_get_array() will be looking at
> the fwnode of the parent.


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Feb 27, 2024 at 2:52 AM Chris Packham
<Chris.Packham@alliedtelesis.co.nz> wrote:
> On 26/02/24 15:23, Andy Shevchenko wrote:
> > On Sun, Feb 25, 2024 at 11:34 PM Chris Packham
> > <chris.packham@alliedtelesis.co.nz> wrote:
> >> This series adds a driver for a 7 segment LED display.
> >>
> >> I'd like to get some feedback on how this could be extended to support >1
> >> character. The driver as presented is sufficient for my hardware which only has
> >> a single character display but I can see that for this to be generically useful
> >> supporting more characters would be desireable.
> >>
> >> Earlier I posted an idea that the characters could be represended by
> >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> >> convenience of using devm_gpiod_get_array() (unless I've missed something).
> > It seems you didn't know that the tree for auxdisplay has been changed.
> > Can you rebase your stuff on top of
> > https://scanmail.trustwave.com/?c=20988&d=vfbb5fnU59kvIREfdD-21Pab30bpMpuTM2Ipv28now&u=https%3a%2f%2fgit%2ekernel%2eorg%2fpub%2fscm%2flinux%2fkernel%2fgit%2fandy%2flinux-auxdisplay%2egit%2flog%2f%3fh%3dfor-next%3f
> > It will reduce your code base by ~50%.
> >
> > WRT subnodes, you can go with device_for_each_child_node() and
> > retrieve gpio array per digit. It means you will have an array of
> > arrays of GPIOs.
>
> So would the following work?
>
>      count = device_get_child_node_count(dev);
>      struct gpio_descs **chars  = devm_kzalloc(dev, sizeof(*chars) *
> count, GFP_KERNEL);
>
>      i = 0;
>      device_for_each_child_node(dev, child) {
>          chars[i] = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);

I see what you meant earlier.
This should be devm_fwnode_gpiod_get_index(), but we lack the _array
variant of it...

Dunno what to do, maybe adding the _array variant is the good move,
maybe something else.

>      }
>
> I haven't used the child. The devm_gpiod_get_array() will be looking at
> the fwnode of the parent.


-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Alexander Dahl]

Hello Chris,

Am Mon, Feb 26, 2024 at 08:10:07PM +0000 schrieb Chris Packham:
> Hi Alex,
> 
> On 27/02/24 05:04, Alexander Dahl wrote:
> > Hello Chris,
> >
> > Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham:
> >> This series adds a driver for a 7 segment LED display.
> >>
> >> I'd like to get some feedback on how this could be extended to support >1
> >> character. The driver as presented is sufficient for my hardware which only has
> >> a single character display but I can see that for this to be generically useful
> >> supporting more characters would be desireable.
> >>
> >> Earlier I posted an idea that the characters could be represended by
> >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> >> convenience of using devm_gpiod_get_array() (unless I've missed something).
> >>
> >> [1] - https://scanmail.trustwave.com/?c=20988&d=trbc5eARVo-5gepRnwbAKbQmiGk1bOSpqZDQx9bx7w&u=https%3a%2f%2flore%2ekernel%2eorg%2flkml%2f2a8d19ee-b18b-4b7c-869f-7d601cea30b6%40alliedtelesis%2eco%2enz%2f
> > Read that thread out of curiosity and I'm sorry if I'm late to the
> > party, but I wondered why this is limited to LEDs connected to GPIOs?
> >
> > Would it be possible to somehow stack this on top of some existing
> > LEDs?  I mean you could wire a 7 segment device to almost any LED
> > driver IC with enough channels, couldn't you?
> 
> Mainly because the GPIO version is the hardware I have. I do wonder how 
> this might work with something like the pca9551 which really is just a 
> fancy version of the pca9554 on my board. A naive implementation would 
> be to configure all the pca9551 pins as GPIOs and use what I have as-is. 

My idea was to do it on top of the LED abstraction, not on top of the
GPIO abstraction.  Currently you are using the GPIO consumer interface
and group 7 gpios which are supplied by that pca9554 in your case, but
could also be coming from a SoC or a 74hc595 etc.

Why not put it a level of abstraction higher, and let it consume LEDs
instead of GPIOs?  Your usecase would still be possible then.

As far as I could see the concept of a LED consumer can be done, the
leds-group-multicolor driver in
drivers/leds/rgb/leds-group-multicolor.c does that, introduced with
kernel v6.6 not so long ago.  It sets the sysfs entries of the LEDs
part of the group to readonly so they are not usable on their own
anymore and one would not disturb the grouped multicolor LED.

This would allow to use LEDs as a 7 segment group driven by any LED
driver including leds-gpio.

> Making a line display out of LED triggers might be another way of doing 
> it but not something I really want to pursue.

Fair enough.  I just wanted to share my idea.  Didn't want to pressure
you in any direction. :-)

Greets
Alex

> 
> >
> > Greets
> > Alex
> >
> >> 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
> >>
> >>   .../auxdisplay/generic,gpio-7seg.yaml         |  40 +++++
> >>   .../boot/dts/marvell/armada-385-atl-x530.dts  |  13 +-
> >>   drivers/auxdisplay/Kconfig                    |   7 +
> >>   drivers/auxdisplay/Makefile                   |   1 +
> >>   drivers/auxdisplay/seg-led.c                  | 152 ++++++++++++++++++
> >>   5 files changed, 212 insertions(+), 1 deletion(-)
> >>   create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
> >>   create mode 100644 drivers/auxdisplay/seg-led.c
> >>
> >> -- 
> >> 2.43.2
> >>
> >>

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

[Alexander Dahl]

Hello Chris,

Am Mon, Feb 26, 2024 at 08:10:07PM +0000 schrieb Chris Packham:
> Hi Alex,
> 
> On 27/02/24 05:04, Alexander Dahl wrote:
> > Hello Chris,
> >
> > Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham:
> >> This series adds a driver for a 7 segment LED display.
> >>
> >> I'd like to get some feedback on how this could be extended to support >1
> >> character. The driver as presented is sufficient for my hardware which only has
> >> a single character display but I can see that for this to be generically useful
> >> supporting more characters would be desireable.
> >>
> >> Earlier I posted an idea that the characters could be represended by
> >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> >> convenience of using devm_gpiod_get_array() (unless I've missed something).
> >>
> >> [1] - https://scanmail.trustwave.com/?c=20988&d=trbc5eARVo-5gepRnwbAKbQmiGk1bOSpqZDQx9bx7w&u=https%3a%2f%2flore%2ekernel%2eorg%2flkml%2f2a8d19ee-b18b-4b7c-869f-7d601cea30b6%40alliedtelesis%2eco%2enz%2f
> > Read that thread out of curiosity and I'm sorry if I'm late to the
> > party, but I wondered why this is limited to LEDs connected to GPIOs?
> >
> > Would it be possible to somehow stack this on top of some existing
> > LEDs?  I mean you could wire a 7 segment device to almost any LED
> > driver IC with enough channels, couldn't you?
> 
> Mainly because the GPIO version is the hardware I have. I do wonder how 
> this might work with something like the pca9551 which really is just a 
> fancy version of the pca9554 on my board. A naive implementation would 
> be to configure all the pca9551 pins as GPIOs and use what I have as-is. 

My idea was to do it on top of the LED abstraction, not on top of the
GPIO abstraction.  Currently you are using the GPIO consumer interface
and group 7 gpios which are supplied by that pca9554 in your case, but
could also be coming from a SoC or a 74hc595 etc.

Why not put it a level of abstraction higher, and let it consume LEDs
instead of GPIOs?  Your usecase would still be possible then.

As far as I could see the concept of a LED consumer can be done, the
leds-group-multicolor driver in
drivers/leds/rgb/leds-group-multicolor.c does that, introduced with
kernel v6.6 not so long ago.  It sets the sysfs entries of the LEDs
part of the group to readonly so they are not usable on their own
anymore and one would not disturb the grouped multicolor LED.

This would allow to use LEDs as a 7 segment group driven by any LED
driver including leds-gpio.

> Making a line display out of LED triggers might be another way of doing 
> it but not something I really want to pursue.

Fair enough.  I just wanted to share my idea.  Didn't want to pressure
you in any direction. :-)

Greets
Alex

> 
> >
> > Greets
> > Alex
> >
> >> 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
> >>
> >>   .../auxdisplay/generic,gpio-7seg.yaml         |  40 +++++
> >>   .../boot/dts/marvell/armada-385-atl-x530.dts  |  13 +-
> >>   drivers/auxdisplay/Kconfig                    |   7 +
> >>   drivers/auxdisplay/Makefile                   |   1 +
> >>   drivers/auxdisplay/seg-led.c                  | 152 ++++++++++++++++++
> >>   5 files changed, 212 insertions(+), 1 deletion(-)
> >>   create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml
> >>   create mode 100644 drivers/auxdisplay/seg-led.c
> >>
> >> -- 
> >> 2.43.2
> >>
> >>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Andy Shevchenko]

On Tue, Feb 27, 2024 at 10:43 AM Alexander Dahl <ada@thorsis.com> wrote:
> Am Mon, Feb 26, 2024 at 08:10:07PM +0000 schrieb Chris Packham:
> > On 27/02/24 05:04, Alexander Dahl wrote:
> > > Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham:
> > >> This series adds a driver for a 7 segment LED display.
> > >>
> > >> I'd like to get some feedback on how this could be extended to support >1
> > >> character. The driver as presented is sufficient for my hardware which only has
> > >> a single character display but I can see that for this to be generically useful
> > >> supporting more characters would be desireable.
> > >>
> > >> Earlier I posted an idea that the characters could be represended by
> > >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> > >> convenience of using devm_gpiod_get_array() (unless I've missed something).
> > >>
> > >> [1] - https://scanmail.trustwave.com/?c=20988&d=trbc5eARVo-5gepRnwbAKbQmiGk1bOSpqZDQx9bx7w&u=https%3a%2f%2flore%2ekernel%2eorg%2flkml%2f2a8d19ee-b18b-4b7c-869f-7d601cea30b6%40alliedtelesis%2eco%2enz%2f
> > > Read that thread out of curiosity and I'm sorry if I'm late to the
> > > party, but I wondered why this is limited to LEDs connected to GPIOs?
> > >
> > > Would it be possible to somehow stack this on top of some existing
> > > LEDs?  I mean you could wire a 7 segment device to almost any LED
> > > driver IC with enough channels, couldn't you?
> >
> > Mainly because the GPIO version is the hardware I have. I do wonder how
> > this might work with something like the pca9551 which really is just a
> > fancy version of the pca9554 on my board. A naive implementation would
> > be to configure all the pca9551 pins as GPIOs and use what I have as-is.
>
> My idea was to do it on top of the LED abstraction, not on top of the
> GPIO abstraction.  Currently you are using the GPIO consumer interface
> and group 7 gpios which are supplied by that pca9554 in your case, but
> could also be coming from a SoC or a 74hc595 etc.
>
> Why not put it a level of abstraction higher, and let it consume LEDs
> instead of GPIOs?  Your usecase would still be possible then.
>
> As far as I could see the concept of a LED consumer can be done, the
> leds-group-multicolor driver in
> drivers/leds/rgb/leds-group-multicolor.c does that, introduced with
> kernel v6.6 not so long ago.  It sets the sysfs entries of the LEDs
> part of the group to readonly so they are not usable on their own
> anymore and one would not disturb the grouped multicolor LED.
>
> This would allow to use LEDs as a 7 segment group driven by any LED
> driver including leds-gpio.

7-segment LEDs are not just a bunch of leds, they have explicit
meaning and using LED infrastructure is an obscuration of what's going
on (too many unrelated details are exposed, too hard to achieve what
user needs). In the auxdisplay we have already the concept of "line
display" with a 7-segment, or 14 (that are two main standards) with
respective character mapping in case somebody wants to print more than
hexadecimal digits on them. If I am mistaken, I would like to see the
concept in the example here on how user space will take care of
displaying (an arbitrary) data. Can you provide a draft of (user-side)
documentation before we even start going this direction?

> > Making a line display out of LED triggers might be another way of doing
> > it but not something I really want to pursue.
>
> Fair enough.  I just wanted to share my idea.  Didn't want to pressure
> you in any direction. :-)


-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Feb 27, 2024 at 10:43 AM Alexander Dahl <ada@thorsis.com> wrote:
> Am Mon, Feb 26, 2024 at 08:10:07PM +0000 schrieb Chris Packham:
> > On 27/02/24 05:04, Alexander Dahl wrote:
> > > Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham:
> > >> This series adds a driver for a 7 segment LED display.
> > >>
> > >> I'd like to get some feedback on how this could be extended to support >1
> > >> character. The driver as presented is sufficient for my hardware which only has
> > >> a single character display but I can see that for this to be generically useful
> > >> supporting more characters would be desireable.
> > >>
> > >> Earlier I posted an idea that the characters could be represended by
> > >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the
> > >> convenience of using devm_gpiod_get_array() (unless I've missed something).
> > >>
> > >> [1] - https://scanmail.trustwave.com/?c=20988&d=trbc5eARVo-5gepRnwbAKbQmiGk1bOSpqZDQx9bx7w&u=https%3a%2f%2flore%2ekernel%2eorg%2flkml%2f2a8d19ee-b18b-4b7c-869f-7d601cea30b6%40alliedtelesis%2eco%2enz%2f
> > > Read that thread out of curiosity and I'm sorry if I'm late to the
> > > party, but I wondered why this is limited to LEDs connected to GPIOs?
> > >
> > > Would it be possible to somehow stack this on top of some existing
> > > LEDs?  I mean you could wire a 7 segment device to almost any LED
> > > driver IC with enough channels, couldn't you?
> >
> > Mainly because the GPIO version is the hardware I have. I do wonder how
> > this might work with something like the pca9551 which really is just a
> > fancy version of the pca9554 on my board. A naive implementation would
> > be to configure all the pca9551 pins as GPIOs and use what I have as-is.
>
> My idea was to do it on top of the LED abstraction, not on top of the
> GPIO abstraction.  Currently you are using the GPIO consumer interface
> and group 7 gpios which are supplied by that pca9554 in your case, but
> could also be coming from a SoC or a 74hc595 etc.
>
> Why not put it a level of abstraction higher, and let it consume LEDs
> instead of GPIOs?  Your usecase would still be possible then.
>
> As far as I could see the concept of a LED consumer can be done, the
> leds-group-multicolor driver in
> drivers/leds/rgb/leds-group-multicolor.c does that, introduced with
> kernel v6.6 not so long ago.  It sets the sysfs entries of the LEDs
> part of the group to readonly so they are not usable on their own
> anymore and one would not disturb the grouped multicolor LED.
>
> This would allow to use LEDs as a 7 segment group driven by any LED
> driver including leds-gpio.

7-segment LEDs are not just a bunch of leds, they have explicit
meaning and using LED infrastructure is an obscuration of what's going
on (too many unrelated details are exposed, too hard to achieve what
user needs). In the auxdisplay we have already the concept of "line
display" with a 7-segment, or 14 (that are two main standards) with
respective character mapping in case somebody wants to print more than
hexadecimal digits on them. If I am mistaken, I would like to see the
concept in the example here on how user space will take care of
displaying (an arbitrary) data. Can you provide a draft of (user-side)
documentation before we even start going this direction?

> > Making a line display out of LED triggers might be another way of doing
> > it but not something I really want to pursue.
>
> Fair enough.  I just wanted to share my idea.  Didn't want to pressure
> you in any direction. :-)


-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

Hi!

> My idea was to do it on top of the LED abstraction, not on top of the
> GPIO abstraction.  Currently you are using the GPIO consumer
> interface

Let's not do that.
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

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

[Pavel Machek]

[-- Attachment #1.1: Type: text/plain, Size: 266 bytes --]

Hi!

> My idea was to do it on top of the LED abstraction, not on top of the
> GPIO abstraction.  Currently you are using the GPIO consumer
> interface

Let's not do that.
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel