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: 7852 bytes --]

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.

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.

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.

A cover letter is ofc still needed.

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

Thanks
- Markus Probst

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

> 
> Regards,
> 
> Hans
> 
> 
> 
> > ---
> >  drivers/tty/serdev/core.c | 36 ++++++++++++++++++++++++++++++------
> >  include/linux/serdev.h    | 31 +++++++++++++++++++++++++++++++
> >  2 files changed, 61 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > index 7500efcdfc21..8354a4688fd8 100644
> > --- a/drivers/tty/serdev/core.c
> > +++ b/drivers/tty/serdev/core.c
> > @@ -29,13 +29,21 @@ static DEFINE_IDA(ctrl_ida);
> >  static ssize_t modalias_show(struct device *dev,
> >  			     struct device_attribute *attr, char *buf)
> >  {
> > +	const struct serdev_device *serdev = to_serdev_device(dev);
> >  	int len;
> >  
> >  	len = acpi_device_modalias(dev, buf, PAGE_SIZE - 1);
> >  	if (len != -ENODEV)
> >  		return len;
> >  
> > -	return of_device_modalias(dev, buf, PAGE_SIZE);
> > +	len = of_device_modalias(dev, buf, PAGE_SIZE);
> > +	if (len != -ENODEV)
> > +		return len;
> > +
> > +	if (!serdev->name[0])
> > +		return -ENODEV;
> > +
> > +	return sysfs_emit(buf, "%s%s\n", SERDEV_MODULE_PREFIX, serdev->name);
> >  }
> >  static DEVICE_ATTR_RO(modalias);
> >  
> > @@ -47,15 +55,22 @@ ATTRIBUTE_GROUPS(serdev_device);
> >  
> >  static int serdev_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
> >  {
> > +	const struct serdev_device *serdev = to_serdev_device(dev);
> >  	int rc;
> >  
> > -	/* TODO: platform modalias */
> > -
> >  	rc = acpi_device_uevent_modalias(dev, env);
> >  	if (rc != -ENODEV)
> >  		return rc;
> >  
> > -	return of_device_uevent_modalias(dev, env);
> > +	rc = of_device_uevent_modalias(dev, env);
> > +	if (rc != -ENODEV)
> > +		return rc;
> > +
> > +	if (!serdev->name[0])
> > +		return -ENODEV;
> > +
> > +	add_uevent_var(env, "MODALIAS=%s%s", SERDEV_MODULE_PREFIX, serdev->name);
> > +	return 0;
> >  }
> >  
> >  static void serdev_device_release(struct device *dev)
> > @@ -88,14 +103,23 @@ static const struct device_type serdev_ctrl_type = {
> >  
> >  static int serdev_device_match(struct device *dev, const struct device_driver *drv)
> >  {
> > +	const struct serdev_device *serdev;
> > +
> >  	if (!is_serdev_device(dev))
> >  		return 0;
> >  
> > -	/* TODO: platform matching */
> >  	if (acpi_driver_match_device(dev, drv))
> >  		return 1;
> >  
> > -	return of_driver_match_device(dev, drv);
> > +	if (of_driver_match_device(dev, drv))
> > +		return 1;
> > +
> > +	/* Fall back to the name set by the creator of the device */
> > +	serdev = to_serdev_device(dev);
> > +	if (!serdev->name[0])
> > +		return 0;
> > +
> > +	return strcmp(serdev->name, drv->name) == 0;
> >  }
> >  
> >  /**
> > diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> > index b6c3d957ec15..26baa0321eef 100644
> > --- a/include/linux/serdev.h
> > +++ b/include/linux/serdev.h
> > @@ -8,6 +8,7 @@
> >  #include <linux/types.h>
> >  #include <linux/device.h>
> >  #include <linux/iopoll.h>
> > +#include <linux/string.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/termios.h>
> >  #include <linux/delay.h>
> > @@ -19,6 +20,10 @@ struct serdev_device;
> >   * serdev device structures
> >   */
> >  
> > +/* Large enough to hold the KBUILD_MODNAME of any serdev client driver */
> > +#define SERDEV_NAME_SIZE	40
> > +#define SERDEV_MODULE_PREFIX	"serial:"
> > +
> >  /**
> >   * struct serdev_device_ops - Callback operations for a serdev device
> >   * @receive_buf:	Function called with data received from device;
> > @@ -35,6 +40,9 @@ struct serdev_device_ops {
> >   * struct serdev_device - Basic representation of an serdev device
> >   * @dev:	Driver model representation of the device.
> >   * @nr:		Device number on serdev bus.
> > + * @name:	Name of the device, set by its creator, empty for devices
> > + *		which are enumerated from firmware, see
> > + *		serdev_device_set_name().
> >   * @ctrl:	serdev controller managing this device.
> >   * @ops:	Device operations.
> >   * @write_comp:	Completion used by serdev_device_write() internally
> > @@ -43,6 +51,7 @@ struct serdev_device_ops {
> >  struct serdev_device {
> >  	struct device dev;
> >  	int nr;
> > +	char name[SERDEV_NAME_SIZE];
> >  	struct serdev_controller *ctrl;
> >  	const struct serdev_device_ops *ops;
> >  	struct completion write_comp;
> > @@ -135,6 +144,28 @@ static inline void serdev_device_set_client_ops(struct serdev_device *serdev,
> >  	serdev->ops = ops;
> >  }
> >  
> > +/**
> > + * serdev_device_set_name() - set the name of a serdev device
> > + * @serdev:	serdev device.
> > + * @name:	non-empty name to use, typically KBUILD_MODNAME of the client
> > + *		driver.
> > + *
> > + * A serdev device is identified by its ACPI companion, by its device tree
> > + * node, or by a name set by its creator. Serdev devices which are
> > + * instantiated by a driver rather than by firmware have neither of the
> > + * former, so they must set a name before serdev_device_add() to be
> > + * identifiable. Doing so gives them a SERDEV_MODULE_PREFIX modalias and
> > + * makes them match a serdev device driver of the same name.
> > + *
> > + * Devices enumerated from firmware have no name and keep deriving their
> > + * modalias and their driver match from firmware only.
> > + */
> > +static inline void serdev_device_set_name(struct serdev_device *serdev,
> > +					  const char *name)
> > +{
> > +	strscpy(serdev->name, name);
> > +}
> > +
> >  static inline
> >  void *serdev_controller_get_drvdata(const struct serdev_controller *ctrl)
> >  {
> > 
> > base-commit: 9e947d96915af63e28896caae85a81b74855621b

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