On Tue, Feb 27, 2024 at 11:22 PM Chris Packham
<chris.packham@xxxxxxxxxxxxxxxxxxx> 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.
--
With Best Regards,
Andy Shevchenko
