Received-SPF: pass (google.com: domain of linux-kernel+bounces-88902-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 1 Mar 2024 20:18:37 +0200
From: Andy Shevchenko <andy@kernel.org>
To: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: 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, pavel@ucw.cz, lee@kernel.org,
	linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 1/4] auxdisplay: Add 7-segment LED display driver
Message-ID: <ZeIb_TaKK1DE6l6U@smile.fi.intel.com>
References: <20240301014203.2033844-1-chris.packham@alliedtelesis.co.nz>
 <20240301014203.2033844-2-chris.packham@alliedtelesis.co.nz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240301014203.2033844-2-chris.packham@alliedtelesis.co.nz>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Fri, Mar 01, 2024 at 02:42:00PM +1300, Chris Packham 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.

..

> + * Driver for a 7 segment LED display

7-segment

..

> + * The GPIOs are wired to the 7 segments in a clockwise fashion starting from
> + * the top.

Not exactly. They can wire them as they wish, we just need to agree on the
sequence of the segments in DT to be mapped to the 7-segment diagram.

..

> + *      -a-
> + *     |   |
> + *     f   b
> + *     |   |
> + *      -g-
> + *     |   |
> + *     e   c
> + *     |   |
> + *      -d-

I would drop this as it's available in UAPI header...

..

> +#include <linux/bitmap.h>
> +#include <linux/container_of.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>

..which you forgot to include here.

..

> +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 *linedisp = &priv->linedisp;
> +	struct linedisp_map *map = linedisp->map;
> +	DECLARE_BITMAP(values, 8);

> +	bitmap_zero(values, 8);

Why do you need this zeroing?

> +	bitmap_set_value8(values, map_to_seg7(&map->map.seg7, linedisp->buf[0]), 0);
> +
> +	gpiod_set_array_value_cansleep(priv->segment_gpios->ndescs, priv->segment_gpios->desc,
> +				       priv->segment_gpios->info, values);
> +}

-- 
With Best Regards,
Andy Shevchenko