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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 28 Feb 2024 01:56:56 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Chris Packham <chris.packham@...iedtelesis.co.nz>
Cc: andy@...nel.org, geert@...ux-m68k.org, robh+dt@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, andrew@...n.ch, 
	gregory.clement@...tlin.com, sebastian.hesselbarth@...il.com, 
	ojeda@...nel.org, tzimmermann@...e.de, javierm@...hat.com, robin@...tonic.nl, 
	lee@...nel.org, pavel@....cz, devicetree@...r.kernel.org, 
	linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 1/4] auxdisplay: Add 7 segment LED display driver

On Tue, Feb 27, 2024 at 11:22 PM Chris Packham
<chris.packham@...iedtelesis.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.

-- 
With Best Regards,
Andy Shevchenko