Message-ID: <6E522C4F-AED7-4C19-9AC5-6C8F2F096E1A@gmail.com>
Date: Tue, 26 Aug 2025 16:44:48 -0400
From: Jean-François Lessard <jefflessard3@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.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

Le 26 août 2025 11 h 22 min 56 s HAE, Andy Shevchenko <andriy.shevchenko@...el.com> a écrit :
>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/
>

Thanks for the correction and link. I'll update to 6.18 with appropriate future
date.

>...
>
>> >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)
>

TM16xx devices are primarily 7-segment displays with additional LED icon and
key scanning capabilities.

>> (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?
>

You're right, I'll update the copyright to 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".
>

I'll move bitmap.h to the source 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.
>

I'll address the regmap evaluation in the cover letter of the next submission
as you requested.

The union removal will be addressed too.

>...
>
>> >> +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.
>

To provide a coherent user interface, I'd like to propose extending linedisp to
optionally attach attributes to an existing device rather than always creating
virtual devices. This would create a unified /sys/class/leds/{label}/ interface
for content, brightness, and icons while maintaining linedisp's established
APIs.

If device attachment isn't feasible, could linedisp at minimum accept a device
name parameter to create /sys/virtual/linedisp-{label} instead of generic
linedisp.{n}?

I'm happy to implement these linedisp enhancements as part of the TM16xx
integration if you find the approach acceptable.

>> 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'll submit patches to extend linedisp with missing functionality (like
single-pass scrolling control) as part of this integration.

>> 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.
>

Correct, my oversight. I'll use the fwnode variant.

>...
>
>> >> +	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).
>

Good point. I'll use a different name to distinguish from 7-segment terminology.

>...
>
>> >> +	/* 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).
>

I think linedisp is the way to go if we can provide a consistent user space API.

Powered by blists - more mailing lists