[v2,1/4] auxdisplay: Add 7 segment LED display driver
Commit Message
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 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.c | 119 +++++++++++++++++++++++++++++++++++
3 files changed, 130 insertions(+)
create mode 100644 drivers/auxdisplay/seg-led.c
Comments
Hi--
On 2/27/24 13:22, Chris Packham wrote:
> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
> index d4be0a3695ce..52a245ca0c8d 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
> + tristate "Generic 7 segment LED display"
7-segment
> + select LINEDISP
> + help
> + This driver supports a generic 7 segment LED display made up
7-segment
> + 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.
On Tue, Feb 27, 2024 at 11:22 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.
As Randy already pointed out
7-segment (everywhere)
14-segment (also everywhere)
..
> drivers/auxdisplay/seg-led.c | 119 +++++++++++++++++++++++++++++++++++
I believe we want to have a 'gpio' part in the file name and in the Kconfig.
> +config SEG_LED
> + 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.
..
> +#include <linux/container_of.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
Please, avoid proxy headers. I do not believe kernel.h is anyhow used here.
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
..
> +struct seg_led_priv {
> + struct gpio_descs *segment_gpios;
> + struct delayed_work work;
> + struct linedisp linedisp;
Make it the first member, so container_of() will be noop for this.
> +};
..
> +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_map *map = priv->linedisp.map;
> + DECLARE_BITMAP(values, 8);
> + values[0] = map_to_seg7(&map->map.seg7, priv->linedisp.buf[0]);
While it works in this case, it's bad code. You need to use
bitmap_set_value8(). And basically that's the API in a for-loop to be
used in case we have more than one digit. This will require another
header to be included.
> + 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);
Move this part closer to its user, i.e. struct platform_driver below.
..
> + INIT_DELAYED_WORK(&priv->work, seg_led_update);
Move this to ->get_map_type() as other drivers do. Yes, I agree that
->get_map_type() is not the best name currently. You can rename it to
->init_and_get_map_type() if you wish (patches are welcome!) or any
other better name.
@@ -211,6 +211,16 @@ config ARM_CHARLCD
line and the Linux version on the second line, but that's
still useful.
+config SEG_LED
+ 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.
+
menuconfig PARPORT_PANEL
tristate "Parallel port LCD/Keypad Panel support"
depends on PARPORT
@@ -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) += seg-led.o
new file mode 100644
@@ -0,0 +1,119 @@
+// 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/container_of.h>
+#include <linux/errno.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.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 gpio_descs *segment_gpios;
+ struct delayed_work work;
+ struct linedisp linedisp;
+};
+
+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_map *map = priv->linedisp.map;
+ DECLARE_BITMAP(values, 8);
+
+ values[0] = map_to_seg7(&map->map.seg7, priv->linedisp.buf[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)
+{
+ 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 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;
+ 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);
+
+ INIT_DELAYED_WORK(&priv->work, seg_led_update);
+
+ 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 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");