Re: [PATCH v2 1/4] auxdisplay: Add 7 segment LED display driver

[Andy Shevchenko]

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