diff mbox series

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

Message ID 20240301014203.2033844-2-chris.packham@alliedtelesis.co.nz (mailing list archive)
State New, archived
Headers show
Series auxdisplay: 7 segment LED display | expand

Commit Message

Chris Packham March 1, 2024, 1:42 a.m. UTC
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

Comments

Andy Shevchenko March 1, 2024, 6:18 p.m. UTC | #1
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);
> +}
Chris Packham March 3, 2024, 7:58 p.m. UTC | #2
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);
>> +}
Andy Shevchenko March 3, 2024, 8:35 p.m. UTC | #3
+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?
Yury Norov March 3, 2024, 8:56 p.m. UTC | #4
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 };
Chris Packham March 4, 2024, 12:45 a.m. UTC | #5
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.
Chris Packham March 4, 2024, 12:46 a.m. UTC | #6
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().
diff mbox series

Patch

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");