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

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: andy@kernel.org, geert@linux-m68k.org, robh+dt@kernel.org,
	 krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	andrew@lunn.ch,  gregory.clement@bootlin.com,
	sebastian.hesselbarth@gmail.com,  ojeda@kernel.org,
	tzimmermann@suse.de, javierm@redhat.com, robin@protonic.nl,
	 lee@kernel.org, pavel@ucw.cz, devicetree@vger.kernel.org,
	 linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
	 linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 1/4] auxdisplay: Add 7 segment LED display driver
Date: Wed, 28 Feb 2024 01:56:56 +0200	[thread overview]
Message-ID: <CAHp75Ve-zKDgBXhe8fzvW0GY2nB7=ZTfhsJX6OHBH8EQWaWG-Q@mail.gmail.com> (raw)
In-Reply-To: <20240227212244.262710-2-chris.packham@alliedtelesis.co.nz>

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.

-- 
With Best Regards,
Andy Shevchenko