Message-ID: <67046EE4-FBE6-41FA-AB03-B0E01FA1C274@gmail.com>
Date: Sat, 30 Aug 2025 09:55:54 -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 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

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?

>More comments below.
>
>> Backwards compatibility with existing users is maintained, while enabling
>> uniform n-segment sysfs API and richer information for integrated drivers.
>
>...
>
>> -#include <linux/container_of.h>
>> +#include <linux/list.h>
>
>Keep it ordered.
>

Acknowledged. My oversight.

>...
>
>> +static const struct device_type linedisp_type;
>
>Why? I don't see the use of this in the nearest below blocks of code.
>Can you move it closer to the first user so we can see the point?
>

This shouldn't have been submitted. This was part of a previous approach
I experimented with. Will remove.

>...
>
>> +       scoped_guard(spinlock, &linedisp_attachments_lock) {
>> +               list_add(&attachment->list, &linedisp_attachments);
>> +       }
>
>{} are not needed (same rule as with for-loop of if condition with one liner).
>
>> +       return 0;
>
>But in this case you can simply use guard()().
>

Understood, I'll simply use guard()().
 
>...
>
>> +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?

>> +               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;
>> +}
>
>...
>
>> +static struct linedisp *to_linedisp(struct device *dev)
>
>As per above.
>

Same.

>...
>
>>  static struct attribute *linedisp_attrs[] = {
>>         &dev_attr_message.attr,
>>         &dev_attr_scroll_step_ms.attr,
>> +       &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?

>>         &dev_attr_map_seg7.attr,
>>         &dev_attr_map_seg14.attr,
>>         NULL
>> };
>
>...
>
>> +int linedisp_attach(struct linedisp *linedisp, struct device *dev,
>> +                   unsigned int num_chars, const struct linedisp_ops *ops)
>> +{
>> +       int err;
>> +
>> +       memset(linedisp, 0, sizeof(*linedisp));
>> +       linedisp->ops = ops;
>> +       linedisp->num_chars = num_chars;
>> +       linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
>
>> +       err = -ENOMEM;
>> +       linedisp->buf = kzalloc(linedisp->num_chars, GFP_KERNEL);
>> +       if (!linedisp->buf)
>> +               return err;
>
>You can simply return -ENOMEM without initial assignment.
>

Agreed, will return -ENOMEM.

>> +       /* initialise a character mapping, if required */
>> +       err = linedisp_init_map(linedisp);
>> +       if (err)
>> +               goto out_free_buf;
>
>The __free() can be used instead, but for uniform handling it's
>probably not needed here.
>

I think uniform handling is preferable here. Will keep as is.

>> +       /* initialise a timer for scrolling the message */
>> +       timer_setup(&linedisp->timer, linedisp_scroll, 0);
>> +
>> +       err = create_attachment(dev, linedisp, false);
>> +       if (err)
>> +               goto out_del_timer;
>> +
>> +       /* add attribute groups to target device */
>> +       err = device_add_groups(dev, linedisp_groups);
>> +       if (err)
>> +               goto out_del_attach;
>> +
>> +       /* display a default message */
>> +       err = linedisp_display(linedisp, LINEDISP_INIT_TEXT, -1);
>> +       if (err)
>> +               goto out_rem_groups;
>> +
>> +       return 0;
>> +
>> +out_rem_groups:
>> +       device_remove_groups(dev, linedisp_groups);
>> +out_del_attach:
>> +       delete_attachment(dev, false);
>> +out_del_timer:
>> +       timer_delete_sync(&linedisp->timer);
>> +out_free_buf:
>> +       kfree(linedisp->buf);
>> +       return err;
>> +}
>

Powered by blists - more mailing lists