Re: [PATCH v2 1/3] serdev: handle devices without an ACPI or device tree node

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 4343 bytes --]

On Wed, 2026-10-21 at 20:07 +0200, Hans de Goede wrote:
> Hi,
> 
> On 21-Oct-26 17:08, Markus Probst wrote:
> > On Wed, 2026-10-21 at 16:32 +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 21-Oct-26 11:21, ChunAn Wu wrote:
> > > > Driver-created serdev clients may lack an ACPI companion or device tree
> > > > node.
> > > > 
> > > > Add a creator-supplied name for these clients. Use it as a serial
> > > > modalias fallback and to match a serdev driver of the same name. Keep
> > > > returning -ENODEV for unnamed clients.
> > > > 
> > > > Signed-off-by: ChunAn Wu <an.wu@canonical.com>
> > > 
> > > For v3 of this series please include a cover-letter and explain
> > > why this series is an improvement over the` existing situation.
> > > 
> > > The dell-uart-backlight and lenovo-yoga-tab2-pro-1380-fastcharger
> > > drivers are both pretty special cases and here you are adding
> > > a chunk of code to the serdev-core for those special cases,
> > > so I wonder: why?
> > > 
> > > The current manual attach approach taking in these 2 special
> > > drivers works fine.
> > > 
> > > What does this new approach gain us ?
> > 
> > That was my review comment [1] that resulted in this.
> 
> Thanks, that is useful info and the kinda info I would
> expect in a cover-letter :)
> > From what I can tell from revision 1, ChunAn Wu is trying to fix that
> > accessing uevent and modalias of the device (via sysfs) results in a -
> > ENODEV error.
> 
> I see. Makes me wonder though how that -ENODEV is a problem,
> it is weird. But is it a problem?
> 
> The v1 patch writes the following about this:
> 
> "This makes the modalias sysfs read fail and suppresses the device
> add uevent, even though the device is registered and bound to its driver."
> 
> Ok, but the backlight driver ultimately ends up adding a
> /sys/class/backlight device which I assume does get
> a uevent, or is the problem that one never getting a uevent
> because of its parent not getting one ?
> 
> And the lenovo-yoga-tab2-pro-1380-fastcharger driver does all
> it needs to do fully in kernel and does not even export
> a userspace interface.
I am not sure which issue is solves precisely. Maybe udev doesn't like
it?

ChunAn Wu has to answer that.

Returning -ENODEV on an existing device there is likely not intended.


> 
> > As mentioned in my review:
> > 
> > - "/* TODO: platform modalias */"
> > - "/* TODO: platform matching */"
> > still exist in the serdev codebase (drivers/tty/serdev/core.c) and are
> > unresolved. Resolving those also fixes the issue mentioned above.
> > Therefore I think this would be a better approach.
> 
> With that said given that there are TODO comments for this
> I guess addressing the TODO-s is ok.
> > A cover letter is ofc still needed.
> 
> Ack.
> 
> > > Also if you are going to do this may I suggest looking at doing
> > > some sort of of-compatible matching based on a compatible string
> > > inside a swnode ? I do not know what that would look like but
> > > that is something which seems like it might be generally useful
> > > also outside these 2 special cases.
> > 
> > I haven't used them before, but I don't think of-compatible string
> > matching inside a swnode is done anywhere in the kernel (At least I
> > haven't found a case).
> > 
> > They can be added supplementary if needed.
> 
> The idea was to make this a generic match mechanism / helper
> function busses can call, like the current of and acpi
> match (and modalias) helpers. And then have matching by
> a compatible string instead of by name since matching by name
> only works if there is only 1 instance.
Please take a look at my previous review comment [1].

In this I suggested adding a `id_table` field to `serdev_driver`.
This allows to match multiple instances, including providing different
private datas.

software node is only relevant I think if you already use
`fwnode_property_read_string` inside a driver next to acpi and of, and
also want to handle devices without of or acpi.

Thanks
- Markus Probst

[1]
https://lore.kernel.org/linux-serial/0be5624e3778c3426f88daf2d67376435f474497.camel@posteo.de/

> 
> Anyways this is just an idea and somewhat out of scope for
> this patch-set.
> 
> Regards,
> 
> Hans
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]