| 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 |
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); > +}
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); >> +}
+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?
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 };
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.
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 --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");
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