[PATCH v3 0/4] auxdisplay: 7 segment LED display

[PATCH v3 0/4] auxdisplay: 7 segment LED display

[Chris Packham]

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

I haven't had a chance to look at the gpio changes that'd be required to
have multiple characters as subnodes. I wanted to get the code that
addressed Andy and Rob's comments out before my weekend.
--
[1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/

Chris Packham (4):
  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
  ARM: dts: marvell: Indicate USB activity on x530

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

-- 
2.43.2

[PATCH v3 1/4] auxdisplay: Add 7-segment LED display driver

[Chris Packham]

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

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

Notes:
    Changes in v3:
    - "7 segment" -> "7-Segment" in Kconfig help text
    - Update after feedback from Andy
    - Use compatible = "gpio-7-segment" as suggested by Rob
    Changes in v2:
    - Rebased on top of auxdisplay/for-next dropping unnecessary code for 7
      segment maps.
    - Update for new linedisp_register() API
    - Include headers based on actual usage
    - Allow building as module
    - Use compatible = "generic-gpio-7seg" to keep checkpatch.pl happy

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

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

Re: [PATCH v3 1/4] auxdisplay: Add 7-segment LED display driver

[Andy Shevchenko]

On Fri, Mar 01, 2024 at 02:42:00PM +1300, 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.

...

> + * Driver for a 7 segment LED display

7-segment

...

> + * The GPIOs are wired to the 7 segments in a clockwise fashion starting from
> + * the top.

Not exactly. They can wire them as they wish, we just need to agree on the
sequence of the segments in DT to be mapped to the 7-segment diagram.

...

> + *      -a-
> + *     |   |
> + *     f   b
> + *     |   |
> + *      -g-
> + *     |   |
> + *     e   c
> + *     |   |
> + *      -d-

I would drop this as it's available in UAPI header...

...

> +#include <linux/bitmap.h>
> +#include <linux/container_of.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>

...which you forgot to include here.

...

> +static void seg_led_update(struct work_struct *work)
> +{
> +	struct seg_led_priv *priv = container_of(work, struct seg_led_priv, work.work);
> +	struct linedisp *linedisp = &priv->linedisp;
> +	struct linedisp_map *map = linedisp->map;
> +	DECLARE_BITMAP(values, 8);

> +	bitmap_zero(values, 8);

Why do you need this zeroing?

> +	bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0);
> +
> +	gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc,
> +				       priv->segment_gpios->info, values);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/4] auxdisplay: Add 7-segment LED display driver

[Chris Packham]

On 2/03/24 07:18, Andy Shevchenko wrote:
>> +static void seg_led_update(struct work_struct *work)
>> +{
>> +	struct seg_led_priv *priv = container_of(work, struct seg_led_priv,http://scanmail.trustwave.com/?c=20988&d=iZzi5b3S-TQCft9iEXDE69U9UtY0-7GANk9t1WkCxg&u=http%3a%2f%2fwork%2ework%29%3b
>> +	struct linedisp *linedisp = &priv->linedisp;
>> +	struct linedisp_map *map = linedisp->map;
>> +	DECLARE_BITMAP(values, 8);
>> +	bitmap_zero(values, 8);
> Why do you need this zeroing?
>
>> +	bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0);
>> +
Without the zeroing above GCC complains about use  of a potentially 
uninitialized variable here. I think because bitmap_set_value8() does &= 
and |=.
>> +	gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc,
>> +				       priv->segment_gpios->info, values);
>> +}

Re: [PATCH v3 1/4] auxdisplay: Add 7-segment LED display driver

[Andy Shevchenko]

+Cc: Rasmus, Yury

On Sun, Mar 3, 2024 at 9:58 PM Chris Packham
<Chris.Packham@alliedtelesis.co.nz> wrote:
> On 2/03/24 07:18, Andy Shevchenko wrote:

...

> >> +    DECLARE_BITMAP(values, 8);
> >> +    bitmap_zero(values, 8);
> > Why do you need this zeroing?
> >
> >> +    bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0);

> Without the zeroing above GCC complains about use  of a potentially
> uninitialized variable here. I think because bitmap_set_value8() does &=
> and |=.

Hmm... Rasmus, Yury, do we have any ideas how to get rid of this redundancy?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/4] auxdisplay: Add 7-segment LED display driver

[Yury Norov]

On Sun, Mar 03, 2024 at 10:35:03PM +0200, Andy Shevchenko wrote:
> +Cc: Rasmus, Yury
> 
> On Sun, Mar 3, 2024 at 9:58 PM Chris Packham
> <Chris.Packham@alliedtelesis.co.nz> wrote:
> > On 2/03/24 07:18, Andy Shevchenko wrote:
> 
> ...
> 
> > >> +    DECLARE_BITMAP(values, 8);
> > >> +    bitmap_zero(values, 8);
> > > Why do you need this zeroing?
> > >
> > >> +    bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0);
> 
> > Without the zeroing above GCC complains about use  of a potentially
> > uninitialized variable here. I think because bitmap_set_value8() does &=
> > and |=.
> 
> Hmm... Rasmus, Yury, do we have any ideas how to get rid of this redundancy?

DECLARE_BITMAP(values, 8) = { 0 };

Re: [PATCH v3 1/4] auxdisplay: Add 7-segment LED display driver

[Chris Packham]

On 2/03/24 07:18, Andy Shevchenko wrote:
> I would drop this as it's available in UAPI header...
>
> ...
>
>> +#include <linux/bitmap.h>
>> +#include <linux/container_of.h>
>> +#include <linux/errno.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/types.h>
>> +#include <linux/workqueue.h>
> ...which you forgot to include here.
>
> ...
Actually do I need to? I'm not using anything in map_to_7segment.h, 
that's all taken care of in the line-display code.

Re: [PATCH v3 1/4] auxdisplay: Add 7-segment LED display driver

[Chris Packham]

On 4/03/24 13:45, Chris Packham wrote:
>
> On 2/03/24 07:18, Andy Shevchenko wrote:
>> I would drop this as it's available in UAPI header...
>>
>> ...
>>
>>> +#include <linux/bitmap.h>
>>> +#include <linux/container_of.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/gpio/consumer.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/types.h>
>>> +#include <linux/workqueue.h>
>> ...which you forgot to include here.
>>
>> ...
> Actually do I need to? I'm not using anything in map_to_7segment.h, 
> that's all taken care of in the line-display code.

Oops no I still need it for map_to_seg7().

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

[Chris Packham]

Add bindings for a generic 7-segment LED display using GPIOs.

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

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

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

diff --git a/Documentation/devicetree/bindings/auxdisplay/gpio-7-segment.yaml b/Documentation/devicetree/bindings/auxdisplay/gpio-7-segment.yaml
new file mode 100644
index 000000000000..76af4bf4e9a6
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/gpio-7-segment.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/gpio-7-segment.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO based LED segment display
+
+maintainers:
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+properties:
+  compatible:
+    const: gpio-7-segment
+
+  segment-gpios:
+    description:
+      An array of GPIOs one per segment. The first GPIO corresponds to the A
+      segment the last GPIO corresponds to the G segment.
+    minItems: 7
+    maxItems: 7
+
+required:
+  - segment-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/gpio/gpio.h>
+
+    led-7seg {
+        compatible = "gpio-7-segment";
+        segment-gpios = <&gpio 0 GPIO_ACTIVE_LOW
+                         &gpio 1 GPIO_ACTIVE_LOW
+                         &gpio 2 GPIO_ACTIVE_LOW
+                         &gpio 3 GPIO_ACTIVE_LOW
+                         &gpio 4 GPIO_ACTIVE_LOW
+                         &gpio 5 GPIO_ACTIVE_LOW
+                         &gpio 6 GPIO_ACTIVE_LOW>;
+    };
-- 
2.43.2

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

[Andy Shevchenko]

On Fri, Mar 01, 2024 at 02:42:01PM +1300, Chris Packham wrote:
> Add bindings for a generic 7-segment LED display using GPIOs.

...

> +  segment-gpios:
> +    description:
> +      An array of GPIOs one per segment. The first GPIO corresponds to the A
> +      segment the last GPIO corresponds to the G segment.
> +    minItems: 7
> +    maxItems: 7

As we discussed, put the ASCII art here to be sure everybody on the same page
what 'a' - 'g' are and I would put 'dp' as well for the better understanding of
the possible configurations (it doesn't mean you have to support it).

...

Either way this will require an Ack by DT maintainers.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 3/4] 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 generic-gpio-7seg device.

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

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

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

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

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

[Andy Shevchenko]

On Fri, Mar 01, 2024 at 02:42:02PM +1300, Chris Packham wrote:
> 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 generic-gpio-7seg device.

Isn't it called differently now?

...

Will need an Ack from the respective maintainers.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 4/4] ARM: dts: marvell: Indicate USB activity on x530

[Chris Packham]

Use the dot on the 7-segment LED block to indicate USB access on the
x530.

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

Notes:
    Changes in v3:
    - None
    Changes in v2:
    - New

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

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 60c2abf572b6..a53dd17d11b4 100644
--- a/arch/arm/boot/dts/marvell/armada-385-atl-x530.dts
+++ b/arch/arm/boot/dts/marvell/armada-385-atl-x530.dts
@@ -54,6 +54,15 @@ &led_7seg_gpio 4 GPIO_ACTIVE_LOW
 				 &led_7seg_gpio 5 GPIO_ACTIVE_LOW
 				 &led_7seg_gpio 6 GPIO_ACTIVE_LOW>;
 	};
+
+	leds {
+		compatible = "gpio-leds";
+		led-0 {
+			label = "usb";
+			gpios =  <&led_7seg_gpio 7 GPIO_ACTIVE_LOW>;
+			linux,default-trigger = "usb-host";
+		};
+	};
 };
 
 &pciec {
-- 
2.43.2

Re: [PATCH v3 4/4] ARM: dts: marvell: Indicate USB activity on x530

[Andy Shevchenko]

On Fri, Mar 01, 2024 at 02:42:03PM +1300, Chris Packham wrote:
> Use the dot on the 7-segment LED block to indicate USB access on the
> x530.

As I said, I'm not going to apply this even with Acks.

The problem here as I see it is the future decision on how DP should
behave like. If you put this into DT, we will to support this to the end
of the platform.

So, drop this from the next version. You may try afterwards to apply it via
different routes (will be not my problem :-).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] ARM: dts: marvell: Indicate USB activity on x530

[Geert Uytterhoeven]

Hi Andy,

On Fri, Mar 1, 2024 at 7:24 PM Andy Shevchenko <andy@kernel.org> wrote:
> On Fri, Mar 01, 2024 at 02:42:03PM +1300, Chris Packham wrote:
> > Use the dot on the 7-segment LED block to indicate USB access on the
> > x530.
>
> As I said, I'm not going to apply this even with Acks.

I guess you should not apply any of the dts patches to the
auxdisplay tree anyway?

> The problem here as I see it is the future decision on how DP should
> behave like.  If you put this into DT, we will to support this to the end
> of the platform.

As there exist 7-seg displays (and wirings) with and without DP,
the 7-seg driver and DT bindings should handle both cases.  How to
wire/use the DP LED is up to the hardware designer / DTS writer.

I agree it's a thin boundary between hardware description and software
policy, though.  Is that your main concern?

> So, drop this from the next version. You may try afterwards to apply it via
> different routes (will be not my problem :-).

Exactly ;-)

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v3 4/4] ARM: dts: marvell: Indicate USB activity on x530

[Chris Packham]

On 3/03/24 22:48, Geert Uytterhoeven wrote:
> Hi Andy,
>
> On Fri, Mar 1, 2024 at 7:24 PM Andy Shevchenko <andy@kernel.org> wrote:
>> On Fri, Mar 01, 2024 at 02:42:03PM +1300, Chris Packham wrote:
>>> Use the dot on the 7-segment LED block to indicate USB access on the
>>> x530.
>> As I said, I'm not going to apply this even with Acks.

I'll drop this one for the next round.

> I guess you should not apply any of the dts patches to the
> auxdisplay tree anyway?

That's OK by me. I've just been including them so there is an in-tree 
user of the driver. It also shows how I've been testing things.

I can send them via the ARM maintainers once the dust settles on the 
first two patches.

>
>> The problem here as I see it is the future decision on how DP should
>> behave like.  If you put this into DT, we will to support this to the end
>> of the platform.
> As there exist 7-seg displays (and wirings) with and without DP,
> the 7-seg driver and DT bindings should handle both cases.  How to
> wire/use the DP LED is up to the hardware designer / DTS writer.
>
> I agree it's a thin boundary between hardware description and software
> policy, though.  Is that your main concern?

In this specific case I'd justify the (ab)use of the DP LED on this 
product as an optimization so we don't have to find board space for a 
separate LED to indicate USB activity.

I do have an idea for handling the DP for the more general case. 
Basically if 8 segment GPIOs are supplied we can use a slightly 
different segment map (map7plus1?) so that characters that are better 
represented as dots use that instead.

Re: [PATCH v3 4/4] ARM: dts: marvell: Indicate USB activity on x530

[Andy Shevchenko]

On Sun, Mar 3, 2024 at 11:48 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Mar 1, 2024 at 7:24 PM Andy Shevchenko <andy@kernel.org> wrote:
> > On Fri, Mar 01, 2024 at 02:42:03PM +1300, Chris Packham wrote:
> > > Use the dot on the 7-segment LED block to indicate USB access on the
> > > x530.
> >
> > As I said, I'm not going to apply this even with Acks.
>
> I guess you should not apply any of the dts patches to the
> auxdisplay tree anyway?

I think it depends. If we got maintainers' Acks, etc, why not? If DT
maintainers think otherwise, then no, we shouldn't.

> > The problem here as I see it is the future decision on how DP should
> > behave like.  If you put this into DT, we will to support this to the end
> > of the platform.
>
> As there exist 7-seg displays (and wirings) with and without DP,
> the 7-seg driver and DT bindings should handle both cases.  How to
> wire/use the DP LED is up to the hardware designer / DTS writer.

Right. But my personal statistics for now is: 100% has DP (out of
about a dozen of different chip + LED combinations). What's yours?

> I agree it's a thin boundary between hardware description and software
> policy, though.  Is that your main concern?

I believe so. Because if we mark DP for use for something else, it
makes it much harder to re-use it as dot/comma later on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] ARM: dts: marvell: Indicate USB activity on x530

[Geert Uytterhoeven]

Hi Andy,

On Sun, Mar 3, 2024 at 9:43 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, Mar 3, 2024 at 11:48 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Mar 1, 2024 at 7:24 PM Andy Shevchenko <andy@kernel.org> wrote:
> > > The problem here as I see it is the future decision on how DP should
> > > behave like.  If you put this into DT, we will to support this to the end
> > > of the platform.
> >
> > As there exist 7-seg displays (and wirings) with and without DP,
> > the 7-seg driver and DT bindings should handle both cases.  How to
> > wire/use the DP LED is up to the hardware designer / DTS writer.
>
> Right. But my personal statistics for now is: 100% has DP (out of
> about a dozen of different chip + LED combinations). What's yours?

It's indeed hard to find contemporary 7-segment LED assemblies that
lack the DP.  But they do exist[1].  There's also no guarantee that the
DP is wired.
And don't forget custom or home-built assemblies using discrete LEDs,
especially for huge displays (e.g. using one LED-strip per segment).
So IMHO it would be a bad idea to make the DP mandatory.

[1] https://www.alibaba.com/product-detail/CC-CA-188-led-display-0_60626228913.html

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v3 4/4] ARM: dts: marvell: Indicate USB activity on x530

[Andy Shevchenko]

On Mon, Mar 4, 2024 at 11:57 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Sun, Mar 3, 2024 at 9:43 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

...

> So IMHO it would be a bad idea to make the DP mandatory.

But I'm not talking about making it mandatory, I'm talking about the
DP to be used as DP when it _is_ present and wired. If current
platform wants to use DP for something else, I'm pretty much worried
that this is the right thing to do.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/4] ARM: dts: marvell: Indicate USB activity on x530

[Geert Uytterhoeven]

Hi Andy,

On Mon, Mar 4, 2024 at 7:17 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Mar 4, 2024 at 11:57 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Sun, Mar 3, 2024 at 9:43 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
>
> ...
>
> > So IMHO it would be a bad idea to make the DP mandatory.
>
> But I'm not talking about making it mandatory, I'm talking about the

OK.

> DP to be used as DP when it _is_ present and wired. If current
> platform wants to use DP for something else, I'm pretty much worried
> that this is the right thing to do.

There is not much we can do about that. People can already model
such displays as individual LEDs, too.
And in some sense, the auxdisplay/linedisp driver for
"generic-gpio-7seg" imposes a policy, too.
What if people want to e.g. use 4 7-seg displays to show a continuously
running snake?

Gr{oetje,eeting}s,

                        Geert

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

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

Re: [PATCH v3 4/4] ARM: dts: marvell: Indicate USB activity on x530

[Andy Shevchenko]

On Mon, Mar 04, 2024 at 08:01:58PM +0100, Geert Uytterhoeven wrote:
> On Mon, Mar 4, 2024 at 7:17 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, Mar 4, 2024 at 11:57 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Sun, Mar 3, 2024 at 9:43 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:

...

> > > So IMHO it would be a bad idea to make the DP mandatory.
> >
> > But I'm not talking about making it mandatory, I'm talking about the
> 
> OK.
> 
> > DP to be used as DP when it _is_ present and wired. If current
> > platform wants to use DP for something else, I'm pretty much worried
> > that this is the right thing to do.
> 
> There is not much we can do about that. People can already model
> such displays as individual LEDs, too.
> And in some sense, the auxdisplay/linedisp driver for
> "generic-gpio-7seg" imposes a policy, too.

Does it? It's exactly targeting very specific HW configuration. The only
question here is DP.

> What if people want to e.g. use 4 7-seg displays to show a continuously
> running snake?

We have an ABI to update a "character" mapping, so it's possible to do, but
it is not a main purpose of line display library.

Free running 7-segment display does probably belong to LED framework in that
sense (as just represents a 7 LEDs that user configured in a specific way in
the physical world). In such case it's just the 7 LEDs on a single PCB.

If you consider these limits as "policy", okay, but it's _hardware driven_
one, and not software.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Mar 01, 2024 at 02:41:59PM +1300, Chris Packham wrote:
> This series adds a driver for a 7 segment LED display.
> 
> I haven't had a chance to look at the gpio changes that'd be required to
> have multiple characters as subnodes. I wanted to get the code that
> addressed Andy and Rob's comments out before my weekend.

Thank you for the update!
Almost fine, one more iteration needed for some minor fixes.

-- 
With Best Regards,
Andy Shevchenko