This series adds a driver for a 7-segment LED display.
I think I've addressed all of Andy's feedback in this round. I haven't
heard from the ARM maintainers on any of the previous rounds. At Andy's
request I've dropped the USB LED change as it's not related. I can
submit the dts change separately if required, I've mostly been including
it so there is an in-tree user of the driver I'm adding.
Chris Packham (3):
auxdisplay: Add 7-segment LED display driver
dt-bindings: auxdisplay: Add bindings for generic 7-segment LED
ARM: dts: marvell: Add 7-segment LED display on x530
.../bindings/auxdisplay/gpio-7-segment.yaml | 54 +++++++++
.../boot/dts/marvell/armada-385-atl-x530.dts | 13 ++-
drivers/auxdisplay/Kconfig | 10 ++
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/seg-led-gpio.c | 109 ++++++++++++++++++
5 files changed, 186 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/auxdisplay/gpio-7-segment.yaml
create mode 100644 drivers/auxdisplay/seg-led-gpio.c
--
2.43.2
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 <[email protected]>
---
Notes:
Changes in v4:
- Fix one more usage of 7 segment
- Move ASCII art diagram to DT binding
- Include map_to_7segment.h for map_to_seg7()
- Use initialiser for values in seg_led_update
Changes in v3:
- "7 segment" -> "7-Segment" in Kconfig help text
- Update after feedback from Andy
- Use compatible = "gpio-7-segment" as suggested by Rob
Changes in v2:
- Rebased on top of auxdisplay/for-next dropping unnecessary code for 7
segment maps.
- Update for new linedisp_register() API
- Include headers based on actual usage
- Allow building as module
- Use compatible = "generic-gpio-7seg" to keep checkpatch.pl happy
drivers/auxdisplay/Kconfig | 10 +++
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/seg-led-gpio.c | 109 ++++++++++++++++++++++++++++++
3 files changed, 120 insertions(+)
create mode 100644 drivers/auxdisplay/seg-led-gpio.c
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index d4be0a3695ce..898ecfb34ed7 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -211,6 +211,16 @@ config ARM_CHARLCD
line and the Linux version on the second line, but that's
still useful.
+config SEG_LED_GPIO
+ tristate "Generic 7-segment LED display"
+ select LINEDISP
+ help
+ This driver supports a generic 7-segment LED display made up
+ of GPIO pins connected to the individual segments.
+
+ This driver can also be built as a module. If so, the module
+ will be called seg-led-gpio.
+
menuconfig PARPORT_PANEL
tristate "Parallel port LCD/Keypad Panel support"
depends on PARPORT
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index a725010ca651..4a8ea41b0550 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_PARPORT_PANEL) += panel.o
obj-$(CONFIG_LCD2S) += lcd2s.o
obj-$(CONFIG_LINEDISP) += line-display.o
obj-$(CONFIG_MAX6959) += max6959.o
+obj-$(CONFIG_SEG_LED_GPIO) += seg-led-gpio.o
diff --git a/drivers/auxdisplay/seg-led-gpio.c b/drivers/auxdisplay/seg-led-gpio.c
new file mode 100644
index 000000000000..6c97c068f4c1
--- /dev/null
+++ b/drivers/auxdisplay/seg-led-gpio.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for a 7-segment LED display
+ *
+ * The decimal point LED present on some devices is currently not
+ * supported.
+ *
+ * Copyright (C) Allied Telesis Labs
+ */
+
+#include <linux/bitmap.h>
+#include <linux/container_of.h>
+#include <linux/errno.h>
+#include <linux/gpio/consumer.h>
+#include <linux/map_to_7segment.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+#include "line-display.h"
+
+struct seg_led_priv {
+ struct linedisp linedisp;
+ struct delayed_work work;
+ struct gpio_descs *segment_gpios;
+};
+
+static void seg_led_update(struct work_struct *work)
+{
+ struct seg_led_priv *priv = container_of(work, struct seg_led_priv, work.work);
+ struct linedisp *linedisp = &priv->linedisp;
+ struct linedisp_map *map = linedisp->map;
+ DECLARE_BITMAP(values, 8) = { 0 };
+
+ bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0);
+
+ gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc,
+ priv->segment_gpios->info, values);
+}
+
+static int seg_led_linedisp_get_map_type(struct linedisp *linedisp)
+{
+ struct seg_led_priv *priv = container_of(linedisp, struct seg_led_priv, linedisp);
+
+ INIT_DELAYED_WORK(&priv->work, seg_led_update);
+ return LINEDISP_MAP_SEG7;
+}
+
+static void seg_led_linedisp_update(struct linedisp *linedisp)
+{
+ struct seg_led_priv *priv = container_of(linedisp, struct seg_led_priv, linedisp);
+
+ schedule_delayed_work(&priv->work, 0);
+}
+
+static const struct linedisp_ops seg_led_linedisp_ops = {
+ .get_map_type = seg_led_linedisp_get_map_type,
+ .update = seg_led_linedisp_update,
+};
+
+static int seg_led_probe(struct platform_device *pdev)
+{
+ struct seg_led_priv *priv;
+ struct device *dev = &pdev->dev;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, priv);
+
+ priv->segment_gpios = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
+ if (IS_ERR(priv->segment_gpios))
+ return PTR_ERR(priv->segment_gpios);
+
+ return linedisp_register(&priv->linedisp, dev, 1, &seg_led_linedisp_ops);
+}
+
+static int seg_led_remove(struct platform_device *pdev)
+{
+ struct seg_led_priv *priv = platform_get_drvdata(pdev);
+
+ cancel_delayed_work_sync(&priv->work);
+ linedisp_unregister(&priv->linedisp);
+
+ return 0;
+}
+
+static const struct of_device_id seg_led_of_match[] = {
+ { .compatible = "gpio-7-segment"},
+ {}
+};
+MODULE_DEVICE_TABLE(of, seg_led_of_match);
+
+static struct platform_driver seg_led_driver = {
+ .probe = seg_led_probe,
+ .remove = seg_led_remove,
+ .driver = {
+ .name = "seg-led-gpio",
+ .of_match_table = seg_led_of_match,
+ },
+};
+module_platform_driver(seg_led_driver);
+
+MODULE_AUTHOR("Chris Packham <[email protected]>");
+MODULE_DESCRIPTION("7 segment LED driver");
+MODULE_LICENSE("GPL");
--
2.43.2
Hi Chris,
On Tue, Mar 5, 2024 at 4:59 AM Chris Packham
<[email protected]> 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 <[email protected]>
> ---
>
> Notes:
> Changes in v4:
> - Fix one more usage of 7 segment
> - Move ASCII art diagram to DT binding
> - Include map_to_7segment.h for map_to_seg7()
> - Use initialiser for values in seg_led_update
Thanks for the update!
> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -211,6 +211,16 @@ config ARM_CHARLCD
> line and the Linux version on the second line, but that's
> still useful.
>
> +config SEG_LED_GPIO
> + tristate "Generic 7-segment LED display"
"depends on GPIOLIB || COMPILE_TEST"?
The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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
Hi Chris,
On Tue, Mar 5, 2024 at 4:59 AM Chris Packham
<[email protected]> 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 <[email protected]>
Sorry, I spoke too soon...
> --- /dev/null
> +++ b/drivers/auxdisplay/seg-led-gpio.c
> +static void seg_led_update(struct work_struct *work)
> +{
> + struct seg_led_priv *priv = container_of(work, struct seg_led_priv, work.work);
> + struct linedisp *linedisp = &priv->linedisp;
> + struct linedisp_map *map = linedisp->map;
> + DECLARE_BITMAP(values, 8) = { 0 };
> +
> + bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0);
> +
> + gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc,
> + priv->segment_gpios->info, values);
> +}
> +static int seg_led_probe(struct platform_device *pdev)
> +{
> + struct seg_led_priv *priv;
> + struct device *dev = &pdev->dev;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + priv->segment_gpios = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
> + if (IS_ERR(priv->segment_gpios))
> + return PTR_ERR(priv->segment_gpios);
This needs some validation of priv->segment_gpios->ndescs, else the
call to gpiod_set_array_value_cansleep() in seg_led_update() may
trigger an out-of-bounds access of the values bitmap.
> +
> + return linedisp_register(&priv->linedisp, dev, 1, &seg_led_linedisp_ops);
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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
On Tue, Mar 05, 2024 at 09:23:07AM +0100, Geert Uytterhoeven wrote:
> On Tue, Mar 5, 2024 at 4:59 AM Chris Packham
> <[email protected]> wrote:
..
> > + priv->segment_gpios = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
> > + if (IS_ERR(priv->segment_gpios))
> > + return PTR_ERR(priv->segment_gpios);
>
> This needs some validation of priv->segment_gpios->ndescs, else the
> call to gpiod_set_array_value_cansleep() in seg_led_update() may
> trigger an out-of-bounds access of the values bitmap.
Alternatively we can call gpiod_count() beforehand and check its result.
--
With Best Regards,
Andy Shevchenko
On 6/03/24 03:57, Andy Shevchenko wrote:
> On Tue, Mar 05, 2024 at 09:23:07AM +0100, Geert Uytterhoeven wrote:
>> On Tue, Mar 5, 2024 at 4:59 AM Chris Packham
>> <[email protected]> wrote:
> ...
>
>>> + priv->segment_gpios = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
>>> + if (IS_ERR(priv->segment_gpios))
>>> + return PTR_ERR(priv->segment_gpios);
>> This needs some validation of priv->segment_gpios->ndescs, else the
>> call to gpiod_set_array_value_cansleep() in seg_led_update() may
>> trigger an out-of-bounds access of the values bitmap.
> Alternatively we can call gpiod_count() beforehand and check its result.
Unless there are any objections I think I'll go with the ndescs check as
it'll be easier to update to the subnode style in the future. It does
mean there will be some extra allocations/frees (handled via the devm_
APIs) in the error case.
On Wed, Mar 6, 2024 at 12:34 AM Chris Packham
<[email protected]> wrote:
> On 6/03/24 03:57, Andy Shevchenko wrote:
> > On Tue, Mar 05, 2024 at 09:23:07AM +0100, Geert Uytterhoeven wrote:
> >> On Tue, Mar 5, 2024 at 4:59 AM Chris Packham
> >> <[email protected]> wrote:
..
> >>> + priv->segment_gpios = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW);
> >>> + if (IS_ERR(priv->segment_gpios))
> >>> + return PTR_ERR(priv->segment_gpios);
> >> This needs some validation of priv->segment_gpios->ndescs, else the
> >> call to gpiod_set_array_value_cansleep() in seg_led_update() may
> >> trigger an out-of-bounds access of the values bitmap.
> > Alternatively we can call gpiod_count() beforehand and check its result.
> Unless there are any objections I think I'll go with the ndescs check as
> it'll be easier to update to the subnode style in the future.
Either works for me.
> It does
> mean there will be some extra allocations/frees (handled via the devm_
> APIs) in the error case.
--
With Best Regards,
Andy Shevchenko