| Message ID | 20240227212244.262710-2-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | auxdisplay: 7 segment LED display | expand |
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.
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" + 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 diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile index a725010ca651..744e354318ae 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) += seg-led.o diff --git a/drivers/auxdisplay/seg-led.c b/drivers/auxdisplay/seg-led.c new file mode 100644 index 000000000000..7bb304fcaa6e --- /dev/null +++ b/drivers/auxdisplay/seg-led.c @@ -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");
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