[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aLbEcN44RT58ywzq@smile.fi.intel.com>
Date: Tue, 2 Sep 2025 13:18:24 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.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: [PATCH 4/5] auxdisplay: linedisp: support attribute attachment
to auxdisplay devices
On Sun, Aug 31, 2025 at 10:00:28PM -0400, Jean-François Lessard wrote:
> Enable linedisp library integration into existing kernel devices (like LED
> class) to provide a uniform 7-segment userspace API without creating
> separate child devices, meeting the consistent interface while maintaining
> coherent device hierarchies.
>
> This allows uniform 7-segment API across all drivers while solving device
> proliferation and fragmented userspace interfaces.
>
> The provided attributes appear in two locations depending on usage:
You wanted to say "...in one of the two possible..."?
Otherwise it looks like undesired side effect of the change.
> 1. On linedisp.N child devices (legacy linedisp_register())
> 2. On the parent auxdisplay device (new linedisp_attach())
> Functionality is identical in both modes.
>
> Existing consumers of linedisp_register() are unaffected. The new API
> enables drivers like TM16XX to integrate 7-segment display functionality
> seamlessly within their LED class device hierarchy.
...
> +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 */
I would rename this to
bool attached; // with inverted logic
or
bool mode; // with "default" (false) mode to be actually legacy one
(so in both cases I think we want false for the legacy mode).
> +};
...
> +static DEFINE_SPINLOCK(linedisp_attachments_lock);
Why spin lock and not mutex?
...
> +/**
> + * linedisp_attach - attach a character line display
> + * @linedisp: pointer to character line display structure
> + * @dev: pointer of the device to attach to
> + * @num_chars: the number of characters that can be displayed
> + * @ops: character line display operations
> + *
Can you add a description here, please? Important note that it should be freed
by the respective API afterwards.
> + * Return: zero on success, else a negative error code.
> + */
...
> + /* 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;
Can this be racy with user space? Once we publish attributes, the new
message can be already issued and here it puts another message.
OTOH this is done before device_add(), so the attributes are not visible
until the device is added.
...
> +void linedisp_detach(struct device *dev)
> +{
> + struct linedisp *linedisp = delete_attachment(dev, false);
> +
> + if (!linedisp)
> + return;
Please, rewrite as
struct linedisp *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);
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists