Message-ID: <aK3RUIt0O-uzEo4-@smile.fi.intel.com>
Date: Tue, 26 Aug 2025 18:22:56 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Jean-François Lessard <jefflessard3@...il.com>
Cc: Andy Shevchenko <andy@...nel.org>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org,
	linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
	Paolo Sabatino <paolo.sabatino@...il.com>,
	Christian Hewitt <christianshewitt@...il.com>,
	Boris Gjenero <boris.gjenero@...il.com>,
	Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: Re: [PATCH v4 3/6] auxdisplay: Add TM16xx 7-segment LED matrix
 display controllers driver

On Mon, Aug 25, 2025 at 01:48:45PM -0400, Jean-François Lessard wrote:
> Le 25 août 2025 11 h 14 min 21 s HAE, Andy Shevchenko <andriy.shevchenko@...el.com> a écrit :
> >On Sun, Aug 24, 2025 at 11:32:29PM -0400, Jean-François Lessard wrote:

...

> >> +Date:		August 2025
> >> +KernelVersion:	6.17
> >
> >The Date should be approximate date of the kernel release (alternatively, -rc1
> >of that). The version is estimated version where ABI can be found first.

> >Both of these need to be changed.
> 
> Given that 6.17-rc3 was just released, should I target 6.18 for the kernel
> version and use a March 2025 date for the estimated release timeframe?

6.18
The date is not in the past, obviously. You can consult with this site:
https://hansen.beer/~dave/phb/

...

> >So, the driver is under auxdisplay, but at the same time it completely relies
> >on LED subsystem... What's going on here?
> 
> The design integrates with the LED subsystem for two main reasons:
> 
> 1. Brightness control:
> The entire display brightness is controlled at the display level
> (individual LED icons can only be on/off via their brightness attributes).
> The LED subsystem provides established mechanisms for this.
> 
> 2. Coherent sysfs interface:
> This provides consistent /sys/class/leds/display for display-level controls
> and /sys/class/leds/display::{function} for individual icons.
> 
> I'm seeking your guidance on the best design for the auxdisplay subsystem.
> 
> >Btw, have you seen
> >https://lore.kernel.org/linux-leds/20231011190017.1230898-1-wse@tuxedocomputers.com/
> >? And  if so, what're your takeaways? (Yes, I know that's about different HW)
> 
> I've read the thread but I'm not clear on the specific point you're making.
> Could you clarify what aspect I should focus on?

If you have a LED matrix, perhaps we can consider different approaches as well.
(It's all about the current HW, is it a 7-segment or arbitrary display, if the
 former, that discussion is unrelated)

> (Though, my personal opinion is that using auxdisplay for keyboard LEDs
> doesn't really make sense. I think it would be better to properly implement
> it the required mechanism into input subsystem, with maybe some
> integration with the leds subsystem. Just a quick opinion, I do not
> master all aspects of this question at all.)

...

> >> + * Copyright (C) 2024 Jean-François Lessard
> >
> >My calendar shows something different.
> 
> The original code was developed in 2024, though it's being submitted in 2025.

But haven't you changed it in 2025?

...

> >> +#include <linux/bitmap.h>
> >
> >Is this used?
> 
> Yes, display->state is a bitmap. I'll move this include to tm16xx_core.c
> since it's not used in the header itself.

Yes, that's what I meant "used by this header file".

...

> >> +	union {
> >> +		struct i2c_client *i2c;
> >> +		struct spi_device *spi;
> >> +	} client;
> >
> >Why? Just drop it. struct device *dev is enough and I can't see the need
> >in this at all.
> 
> I'll remove this union and use container_of(dev, struct i2c_client, dev)
> or container_of(dev, struct spi_device, dev) where the specific client type
> is needed.

This is in correlation with the regmap proposal.

...

> >> +static ssize_t tm16xx_map_seg7_show(struct device *dev,
> >> +				    struct device_attribute *attr, char *buf)
> >> +{
> >> +	memcpy(buf, &map_seg7, sizeof(map_seg7));
> >> +	return sizeof(map_seg7);
> >> +}
> >
> >Can we use LINEDISP library?
> 
> I considered this but have two concerns:
> 
> 1. It creates attributes under a virtual "linedisp.{n}" device,
> which conflicts with the coherent LED sysfs design

It creates the specific attributes for the 7-segment HW, So look at it
from this angle. We have well established library and we expect 7-seg
drivers will use it to make sure that user space may be written in uniform
way.

> 2. Messages scroll indefinitely. There should be control for single-pass scrolling

If we miss that, add it to linedisp. I wouldn't mind, actually I will be in
favour of the development of that library.

> I'm willing to contribute improvements to line-display if needed,
> but this depends on resolving the main LED design question above.

...

> >> +		display->num_digits = 0;
> >> +		fwnode_for_each_child_node(digits_node, child)
> >> +			display->num_digits++;
> >
> >Don't we have a _count API for this?
> 
> I'll use device_get_child_node_count() instead of manual counting loops.

fwnode_get_child_node_count() I assume you meant.

...

> >> +	dev_dbg(dev, "Number of grids: %u\n", display->num_grids);
> >> +	dev_dbg(dev, "Number of segments: %u\n", display->num_segments);
> >
> >I didn't get this. You mean that they are not strictly 7-segment ones?
> 
> The terminology is confusing - "segment" is used both for 7-segment digits
> (which are indeed 7-segment) and for controller matrix coordinates
> (grid,segment) from datasheets. Controllers support varying numbers of segments
> For individual LED icons, not necessarily related to 7-segment displays.
> I'll add a comment to clarify this distinction.

Hmm... Maybe try to rename these 'segments' to something else, like 'hwseg'
(find a better name).

...

> >> +	/* Initialize main LED properties */
> >> +	if (dev->of_node)
> >> +		main->name = dev->of_node->name;
> >> +	if (!main->name)
> >> +		main->name = "display";
> >> +	device_property_read_string(dev, "label", &main->name);
> >
> >My gosh. This is done in the LED core if we even need this...
> 
> This relates to the LED subsystem integration question. If my design approach
> is acceptable, I'll review the LED core implementation to avoid duplicating
> this logic if possible.

I think if you integrate LED for special LED icons and linedisp for 7-segment
into a single driver, it's fine. I just can't speak about LED icons case.
The 7-seg LGTM (assuming linedisp and other existing APIs/ABIs to use, if required).

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists