Message-ID: <CAHp75VeetsQ0NgVEnhic3wdUs-w0q7nQoGX4rO3aNdq8feypRg@mail.gmail.com>
Date: Sat, 30 Aug 2025 19:18:27 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Jean-François Lessard <jefflessard3@...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

On Sat, Aug 30, 2025 at 6:21 PM Jean-François Lessard
<jefflessard3@...il.com> wrote:
> 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:

...

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

a uniform

> separate child devices, meeting consistent interface while maintaining

the consistent

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

Yes, this is a good start.

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

This can be made in a series, so the order will suggest the
dependencies immediately.

...

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

Ah, that's an interesting observation. I think we need to ask Geert
and others about their opinions on this. To me it seems like you have
a point.

...

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

Good for me.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists