Received-SPF: pass (google.com: domain of linux-kernel+bounces-84240-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240227212244.262710-1-chris.packham@alliedtelesis.co.nz> <20240227212244.262710-2-chris.packham@alliedtelesis.co.nz>
In-Reply-To: <20240227212244.262710-2-chris.packham@alliedtelesis.co.nz>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Wed, 28 Feb 2024 01:56:56 +0200
Message-ID: <CAHp75Ve-zKDgBXhe8fzvW0GY2nB7=ZTfhsJX6OHBH8EQWaWG-Q@mail.gmail.com>
Subject: Re: [PATCH v2 1/4] auxdisplay: Add 7 segment LED display driver
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 27, 2024 at 11:22=E2=80=AFPM 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 =3D container_of(work, struct seg_led_p=
riv, work.work);
> +       struct linedisp_map *map =3D priv->linedisp.map;
> +       DECLARE_BITMAP(values, 8);

> +       values[0] =3D 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[] =3D {
> +       { .compatible =3D "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.

--=20
With Best Regards,
Andy Shevchenko