[RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

[RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

[Jean-François Lessard]

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.

Backwards compatibility with existing users is maintained, while enabling
uniform n-segment sysfs API and richer information for integrated drivers.

Signed-off-by: Jean-François Lessard <jefflessard3@gmail.com>
---
 drivers/auxdisplay/line-display.c | 178 ++++++++++++++++++++++++++++--
 drivers/auxdisplay/line-display.h |   4 +
 2 files changed, 172 insertions(+), 10 deletions(-)

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 8590a4cd2..64ab1d835 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -6,13 +6,15 @@
  * Author: Paul Burton <paul.burton@mips.com>
  *
  * Copyright (C) 2021 Glider bv
+ * Copyright (C) 2025 Jean-François Lessard
  */
 
 #ifndef CONFIG_PANEL_BOOT_MESSAGE
 #include <generated/utsrelease.h>
 #endif
 
-#include <linux/container_of.h>
+#include <linux/list.h>
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/export.h>
 #include <linux/idr.h>
@@ -20,6 +22,7 @@
 #include <linux/kstrtox.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/string.h>
 #include <linux/sysfs.h>
 #include <linux/timer.h>
@@ -31,6 +34,75 @@
 
 #define DEFAULT_SCROLL_RATE	(HZ / 2)
 
+/* forward declarations */
+static const struct device_type linedisp_type;
+
+struct linedisp_attachment {
+	struct list_head list;
+	struct device *device;
+	struct linedisp *linedisp;
+	bool owns_device;  /* true for child device mode, false for attached mode */
+};
+
+static LIST_HEAD(linedisp_attachments);
+static DEFINE_SPINLOCK(linedisp_attachments_lock);
+
+static int create_attachment(struct device *dev, struct linedisp *linedisp, bool owns_device)
+{
+	struct linedisp_attachment *attachment;
+
+	attachment = kzalloc(sizeof(*attachment), GFP_KERNEL);
+	if (!attachment)
+		return -ENOMEM;
+
+	attachment->device = dev;
+	attachment->linedisp = linedisp;
+	attachment->owns_device = owns_device;
+
+	scoped_guard(spinlock, &linedisp_attachments_lock) {
+		list_add(&attachment->list, &linedisp_attachments);
+	}
+
+	return 0;
+}
+
+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) {
+		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)
+{
+	struct linedisp_attachment *attachment;
+	struct linedisp *linedisp = NULL;
+
+	scoped_guard(spinlock, &linedisp_attachments_lock) {
+		list_for_each_entry(attachment, &linedisp_attachments, list) {
+			if (attachment->device == dev) {
+				linedisp = attachment->linedisp;
+				break;
+			}
+		}
+	}
+
+	return linedisp;
+}
+
 /**
  * linedisp_scroll() - scroll the display by a character
  * @t: really a pointer to the private data structure
@@ -133,7 +205,7 @@ static int linedisp_display(struct linedisp *linedisp, const char *msg,
 static ssize_t message_show(struct device *dev, struct device_attribute *attr,
 			    char *buf)
 {
-	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp *linedisp = to_linedisp(dev);
 
 	return sysfs_emit(buf, "%s\n", linedisp->message);
 }
@@ -152,7 +224,7 @@ static ssize_t message_show(struct device *dev, struct device_attribute *attr,
 static ssize_t message_store(struct device *dev, struct device_attribute *attr,
 			     const char *buf, size_t count)
 {
-	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp *linedisp = to_linedisp(dev);
 	int err;
 
 	err = linedisp_display(linedisp, buf, count);
@@ -164,7 +236,7 @@ static DEVICE_ATTR_RW(message);
 static ssize_t scroll_step_ms_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
-	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp *linedisp = to_linedisp(dev);
 
 	return sysfs_emit(buf, "%u\n", jiffies_to_msecs(linedisp->scroll_rate));
 }
@@ -173,7 +245,7 @@ static ssize_t scroll_step_ms_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
-	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp *linedisp = to_linedisp(dev);
 	unsigned int ms;
 	int err;
 
@@ -193,9 +265,19 @@ static ssize_t scroll_step_ms_store(struct device *dev,
 
 static DEVICE_ATTR_RW(scroll_step_ms);
 
+static ssize_t num_chars_show(struct device *dev, struct device_attribute *attr,
+			      char *buf)
+{
+	struct linedisp *linedisp = to_linedisp(dev);
+
+	return sysfs_emit(buf, "%u\n", linedisp->num_chars);
+}
+
+static DEVICE_ATTR_RO(num_chars);
+
 static ssize_t map_seg_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp *linedisp = to_linedisp(dev);
 	struct linedisp_map *map = linedisp->map;
 
 	memcpy(buf, &map->map, map->size);
@@ -205,7 +287,7 @@ static ssize_t map_seg_show(struct device *dev, struct device_attribute *attr, c
 static ssize_t map_seg_store(struct device *dev, struct device_attribute *attr,
 			     const char *buf, size_t count)
 {
-	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp *linedisp = to_linedisp(dev);
 	struct linedisp_map *map = linedisp->map;
 
 	if (count != map->size)
@@ -224,6 +306,7 @@ static DEVICE_ATTR(map_seg14, 0644, map_seg_show, map_seg_store);
 static struct attribute *linedisp_attrs[] = {
 	&dev_attr_message.attr,
 	&dev_attr_scroll_step_ms.attr,
+	&dev_attr_num_chars.attr,
 	&dev_attr_map_seg7.attr,
 	&dev_attr_map_seg14.attr,
 	NULL
@@ -232,7 +315,7 @@ static struct attribute *linedisp_attrs[] = {
 static umode_t linedisp_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
 {
 	struct device *dev = kobj_to_dev(kobj);
-	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp *linedisp = to_linedisp(dev);
 	struct linedisp_map *map = linedisp->map;
 	umode_t mode = attr->mode;
 
@@ -263,7 +346,7 @@ static DEFINE_IDA(linedisp_id);
 
 static void linedisp_release(struct device *dev)
 {
-	struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+	struct linedisp *linedisp = to_linedisp(dev);
 
 	kfree(linedisp->map);
 	kfree(linedisp->message);
@@ -320,6 +403,74 @@ static int linedisp_init_map(struct linedisp *linedisp)
 #define LINEDISP_INIT_TEXT "Linux " UTS_RELEASE "       "
 #endif
 
+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;
+
+	/* initialise a character mapping, if required */
+	err = linedisp_init_map(linedisp);
+	if (err)
+		goto out_free_buf;
+
+	/* 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;
+}
+EXPORT_SYMBOL_NS_GPL(linedisp_attach, "LINEDISP");
+
+void linedisp_detach(struct device *dev)
+{
+	struct linedisp *linedisp = delete_attachment(dev, false);
+
+	if (!linedisp)
+		return;
+
+	timer_delete_sync(&linedisp->timer);
+
+	device_remove_groups(dev, linedisp_groups);
+
+	kfree(linedisp->map);
+	kfree(linedisp->message);
+	kfree(linedisp->buf);
+}
+EXPORT_SYMBOL_NS_GPL(linedisp_detach, "LINEDISP");
+
 /**
  * linedisp_register - register a character line display
  * @linedisp: pointer to character line display structure
@@ -362,10 +513,14 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
 	/* initialise a timer for scrolling the message */
 	timer_setup(&linedisp->timer, linedisp_scroll, 0);
 
-	err = device_add(&linedisp->dev);
+	err = create_attachment(&linedisp->dev, linedisp, true);
 	if (err)
 		goto out_del_timer;
 
+	err = device_add(&linedisp->dev);
+	if (err)
+		goto out_del_attach;
+
 	/* display a default message */
 	err = linedisp_display(linedisp, LINEDISP_INIT_TEXT, -1);
 	if (err)
@@ -375,6 +530,8 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
 
 out_del_dev:
 	device_del(&linedisp->dev);
+out_del_attach:
+	delete_attachment(&linedisp->dev, true);
 out_del_timer:
 	timer_delete_sync(&linedisp->timer);
 out_put_device:
@@ -391,6 +548,7 @@ EXPORT_SYMBOL_NS_GPL(linedisp_register, "LINEDISP");
 void linedisp_unregister(struct linedisp *linedisp)
 {
 	device_del(&linedisp->dev);
+	delete_attachment(&linedisp->dev, true);
 	timer_delete_sync(&linedisp->timer);
 	put_device(&linedisp->dev);
 }
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index 4348d7a2f..36853b639 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -6,6 +6,7 @@
  * Author: Paul Burton <paul.burton@mips.com>
  *
  * Copyright (C) 2021 Glider bv
+ * Copyright (C) 2025 Jean-François Lessard
  */
 
 #ifndef _LINEDISP_H
@@ -81,6 +82,9 @@ struct linedisp {
 	unsigned int id;
 };
 
+int linedisp_attach(struct linedisp *linedisp, struct device *dev,
+		    unsigned int num_chars, const struct linedisp_ops *ops);
+void linedisp_detach(struct device *dev);
 int linedisp_register(struct linedisp *linedisp, struct device *parent,
 		      unsigned int num_chars, const struct linedisp_ops *ops);
 void linedisp_unregister(struct linedisp *linedisp);
-- 
2.43.0

Re: [RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

[Andy Shevchenko]

On Fri, Aug 29, 2025 at 4:03 AM Jean-François Lessard
<jefflessard3@gmail.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?
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.

...

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

...

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

...

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

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

...

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

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

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

> +       /* 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;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

[Jean-François Lessard]

Le 30 août 2025 05 h 15 min 23 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>On Fri, Aug 29, 2025 at 4:03 AM Jean-François Lessard
><jefflessard3@gmail.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;
>> +}
>

Re: [RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

[Andy Shevchenko]

On Sat, Aug 30, 2025 at 4:55 PM Jean-François Lessard
<jefflessard3@gmail.com> wrote:
> Le 30 août 2025 05 h 15 min 23 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
> >On Fri, Aug 29, 2025 at 4:03 AM Jean-François Lessard
> ><jefflessard3@gmail.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 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.

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

...

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

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

[Jean-François Lessard]

Le 30 août 2025 10 h 39 min 20 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>On Sat, Aug 30, 2025 at 4:55 PM Jean-François Lessard
><jefflessard3@gmail.com> wrote:
>> Le 30 août 2025 05 h 15 min 23 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>> >On Fri, Aug 29, 2025 at 4:03 AM Jean-François Lessard
>> ><jefflessard3@gmail.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.

Re: [RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

[Andy Shevchenko]

On Sat, Aug 30, 2025 at 6:21 PM Jean-François Lessard
<jefflessard3@gmail.com> wrote:
> Le 30 août 2025 10 h 39 min 20 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
> >On Sat, Aug 30, 2025 at 4:55 PM Jean-François Lessard
> ><jefflessard3@gmail.com> wrote:
> >> Le 30 août 2025 05 h 15 min 23 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
> >> >On Fri, Aug 29, 2025 at 4:03 AM Jean-François Lessard
> >> ><jefflessard3@gmail.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

Re: [RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

[Jean-François Lessard]

Le 30 août 2025 12 h 18 min 27 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>On Sat, Aug 30, 2025 at 6:21 PM Jean-François Lessard
><jefflessard3@gmail.com> wrote:
>> Le 30 août 2025 10 h 39 min 20 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>> >On Sat, Aug 30, 2025 at 4:55 PM Jean-François Lessard
>> ><jefflessard3@gmail.com> wrote:
>> >> Le 30 août 2025 05 h 15 min 23 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
>> >> >On Fri, Aug 29, 2025 at 4:03 AM Jean-François Lessard
>> >> ><jefflessard3@gmail.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
>

Acknowledged.

>> separate child devices, meeting consistent interface while maintaining
>
>the consistent
>

Acknowledged.

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

Great. I will submit the attach capability in a separate patch clarifying this.

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

Agreed. Series would then be:
1. attachment list + to_linedisp()
2. attach to existing device capability
3. num_digits attribute + ABI docs

I guess this should be an independent patch series than the tm16xx patch series.
Isn't it an issue to submit code without a real user? I mean, tm16xx would be
the first attach/detach consumer.

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

Let's see Geert and others feedback on this. If no feedback, I will submit a 4th
patch to the series that would display static (non-scrolling) padded message
when length <= num_chars.

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

Will do so.

Re: [RFC PATCH] auxdisplay: line-display: support attribute attachment to existing device

[Andy Shevchenko]

On Sat, Aug 30, 2025 at 7:57 PM Jean-François Lessard
<jefflessard3@gmail.com> wrote:
> Le 30 août 2025 12 h 18 min 27 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
> >On Sat, Aug 30, 2025 at 6:21 PM Jean-François Lessard
> ><jefflessard3@gmail.com> wrote:
> >> Le 30 août 2025 10 h 39 min 20 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
> >> >On Sat, Aug 30, 2025 at 4:55 PM Jean-François Lessard
> >> ><jefflessard3@gmail.com> wrote:
> >> >> Le 30 août 2025 05 h 15 min 23 s HAE, Andy Shevchenko <andy.shevchenko@gmail.com> a écrit :
> >> >> >On Fri, Aug 29, 2025 at 4:03 AM Jean-François Lessard
> >> >> ><jefflessard3@gmail.com> wrote:

...

> >This can be made in a series, so the order will suggest the
> >dependencies immediately.
>
> Agreed. Series would then be:
> 1. attachment list + to_linedisp()

Let's see how it will go, but I suspect that to_linedisp() still can
be separated.

> 2. attach to existing device capability
> 3. num_digits attribute + ABI docs
>
> I guess this should be an independent patch series than the tm16xx patch series.
> Isn't it an issue to submit code without a real user? I mean, tm16xx would be
> the first attach/detach consumer.

Make a cross-reference to each other in the respective cover letters.
(IIRC, it's possible as `git format-patch` gives a message ID, but I
might be mistaken, and in such a case just make one reference without
a URL to lore archive)

-- 
With Best Regards,
Andy Shevchenko