2024-02-25 21:35:20

by Chris Packham

[permalink] [raw]
Subject: [PATCH 1/3] auxdisplay: Add 7 segment LED display driver

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]>
---
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 <[email protected]>");
+MODULE_DESCRIPTION("7 segment LED driver");
+MODULE_LICENSE("GPL");
+
--
2.43.2



2024-02-26 02:32:55

by Andy Shevchenko

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

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

(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 <[email protected]>");
> +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

2024-02-26 02:50:06

by Randy Dunlap

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

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 <[email protected]>
> ---
> 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