Message-ID: <587AE3BE-CD1F-4160-AA21-12B875E4EE81@gmail.com>
Date: Sat, 30 Aug 2025 11:21:40 -0400
From: Jean-François Lessard <jefflessard3@...il.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
CC: Andy Shevchenko <andy@...nel.org>, Geert Uytterhoeven <geert@...ux-m68k.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

Le 30 août 2025 10 h 39 min 20 s HAE, Andy Shevchenko <andy.shevchenko@...il.com> a écrit :
>On Sat, Aug 30, 2025 at 4:55 PM Jean-François Lessard
><jefflessard3@...il.com> wrote:
>> Le 30 août 2025 05 h 15 min 23 s HAE, Andy Shevchenko <andy.shevchenko@...il.com> a écrit :
>> >On Fri, Aug 29, 2025 at 4:03 AM Jean-François Lessard
>> ><jefflessard3@...il.com> wrote:
>> >>
>> >> Modernize the line-display core library:
>> >> - Add the ability to attach line-display's sysfs attribute group directly
>> >>   to an existing parent device (not only via a child device) using
>> >>   device_add_groups(), allowing coherent integration with subsystems like
>> >>   the LED class.
>> >> - Implement a global linedisp_attachment mapping for unified and race-free
>> >>   attribute access, context lookup, and cleanup, shared between
>> >>   traditional register/unregister and new attach/detach paths.
>> >> - Modify sysfs attribute accessors to retrieve context using a consistent
>> >>   to_linedisp() mechanism.
>> >> - Add a new num_chars read-only attribute reporting the number of display
>> >>   digits to allow static non-scrolling message from userspace.
>> >> - Ensures thread safety and proper list removal for all attachments
>> >>   operations.
>> >
>> >Thanks for working on this, can you split it into 5 patches?
>>
>> My pleasure! Thanks for your feedback.
>>
>> 5 patches? I can't see how these changes could be split into 5 patches.
>> Maybe the commit message is too verbose and needs to be shortened.
>>
>> If needed to be split into multiple patches, it could be:
>> 1. Add attach/detach capability
>> 2. Add num_chars sysfs attribute + ABI doc
>
>Yeah, the commit message was a list of 5 things, hence 5 patches. I
>haven't read closely to map each listed feature to the possible
>change.  The description of to_linedisp() sounds like it can be split
>to a separate patch. I really want to see the attachment/detachment
>patch separate with the explanation "why?". I am not sure I see the
>point.
>

I will shorten commit message and clarify the "why":

Enable linedisp library integration into existing kernel devices
(like LED class) to provide uniform 7-segment userspace API without creating
separate child devices, meeting consistent interface while maintaining
coherent device hierarchies.

This allows uniform 7-segment API across all drivers while solving device
proliferation and fragmented userspace interfaces.

Note that there is no point in introducing to_linedisp() based on attachments
without attach/detach capability. Current implementation is actually correct
if only supporting registration of child devices.

>> I am also thinking to add a 3rd one to pad the message when length is smaller
>> than num_chars. Current behavior is to wrap around which seems weird to me.
>> E.g. for 4 chars:
>>
>> Current behavior:
>> cat "123" > message
>> results in "1231" being displayed
>>
>> Padded behavior:
>> cat "123" > message
>> would result in "123<blank>" being displayed
>>
>> Any thoughts on that?
>
>It's basically the question of circular vs. one time message exposure.
>When it's one time, the padding makes sense, otherwise the current
>(circular) behaviour seems normal to me.
>

I get your point but I should have noted that current behavior is to wrap but
does NOT scroll. That's why it seems wrong to me. It should either wrap AND
scroll, or otherwise simply pad.

>> >More comments below.
>> >
>> >> Backwards compatibility with existing users is maintained, while enabling
>> >> uniform n-segment sysfs API and richer information for integrated drivers.
>
>...
>
>> >> +static struct linedisp *delete_attachment(struct device *dev, bool owns_device)
>> >> +{
>> >> +       struct linedisp_attachment *attachment, *tmp;
>> >> +       struct linedisp *linedisp = NULL;
>> >> +
>> >> +       scoped_guard(spinlock, &linedisp_attachments_lock) {
>> >
>> >Use guard()() and drop NULL assignment.
>>
>> I'll use guard()().
>>
>> NULL assignment was to prevent invalid pointer in case the device is not found
>> in the list. But you are maybe suggesting to drop declaration of linedisp and
>> then directly return the linedisp within the loop and to return NULL after the
>> loop?
>
>This won't work as you want to clean up the things.
>
>> >> +               list_for_each_entry_safe(attachment, tmp, &linedisp_attachments, list) {
>> >> +                       if (attachment->device == dev &&
>> >> +                           attachment->owns_device == owns_device) {
>> >> +                               linedisp = attachment->linedisp;
>> >> +                               list_del(&attachment->list);
>> >> +                               kfree(attachment);
>> >> +                               break;
>> >> +                       }
>> >> +               }
>> >> +       }
>> >> +
>> >> +       return linedisp;
>> >> +}
>
>The usual approach here is to use list_entry_is_head() after the loop.
>
>  list_for_each_entry() {
>    if (...)
>      break;
>  }
>
>  if (list_entry_is_head(...))
>    return NULL;
>
>  linedisp = ...
>  ...
>  return linedisp;
>

Thanks for this pattern. I'll implement it.

>...
>
>> >> +static struct linedisp *to_linedisp(struct device *dev)
>> >
>> >As per above.
>>
>> Same.
>
>Same.
>
>...
>
>> >> +       &dev_attr_num_chars.attr,
>> >
>> >This needs a Documentation update for a new ABI.
>>
>> Agreed. I think it's also worth documenting the other ABIs along the way since
>> they are not documented yet. Then, what Contact should be documented?
>> Is there an auxdisplay mailing list?
>
>Your or no-one's? Is it a mandatory field? In any case the ABI must be
>documented, w.o. doing that any good patch is simply NAK
>automatically.
>

I thought contact was mandatory but after looking closer, there are multiple
existing precedents where there is no documented contact. I think it would be
appropriate to add my contact to the added num_chars property documentation,
but to add documentation of the other existing properties with no contact.

Powered by blists - more mailing lists