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

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

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

On Fri, 2026-10-23 at 13:27 +0800, ChunAn Wu wrote:
> Serdev clients instantiated by a driver have no ACPI companion and no
> device tree node. Their modalias read returns -ENODEV, their add uevent
> is suppressed, and they cannot be driver-matched, so creators force a
> bind with device_driver_attach().
> 
> Resolve both TODOs in the serdev core by modelling matching on the SPI
> bus: add a creator-set modalias to struct serdev_device and an id_table
> to struct serdev_device_driver. ACPI and OF are still tried first;
> clients without their own modalias keep returning -ENODEV. The device
> name, derived from the controller, is unchanged.
> 
> Signed-off-by: ChunAn Wu <an.wu@canonical.com>
> ---
> Changes in v3:
> - Rename struct serdev_device::name to ::modalias; dev_name() is unchanged
> - Model matching on the SPI bus: add struct serdev_device_id and
>   id_table; drop driver->name matching
> - Add serdev_device_set_modalias() helper and SERDEV_MODULE_PREFIX
> - No MODULE_DEVICE_TABLE(serdev, ...) support: both in-tree firmware-less
>   clients are created by the same module that provides their driver, so
>   udev has nothing to autoload
> 
> Changes in v2:
> - Instead of silently returning success with no modalias, use a
>   creator-supplied name as a "serial:<name>" modalias fallback and as
>   the matching key, so clients can bind through normal serdev matching
> 
Thats something I would expect on the cover letter.
>  MAINTAINERS                      |  1 +
>  drivers/tty/serdev/core.c        | 54 ++++++++++++++++++++++++++++----
>  include/linux/device-id/serdev.h | 16 ++++++++++
>  include/linux/serdev.h           | 34 ++++++++++++++++++++
>  4 files changed, 99 insertions(+), 6 deletions(-)
>  create mode 100644 include/linux/device-id/serdev.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 917f3f72e31b..06e67aa5d9b0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -24905,6 +24905,7 @@ L:	linux-serial@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/serial/serial.yaml
>  F:	drivers/tty/serdev/
> +F:	include/linux/device-id/serdev.h
>  F:	include/linux/serdev.h
>  F:	rust/helpers/serdev.c
>  F:	rust/kernel/serdev.rs
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index 7500efcdfc21..e178dc57d31b 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->modalias[0])
> +		return -ENODEV;
> +
> +	return sysfs_emit(buf, "%s%s\n", SERDEV_MODULE_PREFIX, serdev->modalias);
>  }
>  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->modalias[0])
> +		return -ENODEV;
> +
> +	return add_uevent_var(env, "MODALIAS=%s%s", SERDEV_MODULE_PREFIX,
> +			      serdev->modalias);
>  }
>  
>  static void serdev_device_release(struct device *dev)
> @@ -86,16 +101,43 @@ static const struct device_type serdev_ctrl_type = {
>  	.release	= serdev_ctrl_release,
>  };
>  
> +static const struct serdev_device_id *serdev_match_id(const struct serdev_device_id *id,
> +						      const char *modalias)
> +{
> +	if (!id || !modalias[0])
> +		return NULL;
> +
> +	while (id->name[0]) {
> +		if (!strcmp(modalias, id->name))
> +			return id;
> +		id++;
> +	}
> +
> +	return NULL;
> +}
> +
>  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;
> +
> +	/*
> +	 * Devices which are instantiated by a driver instead of by firmware
> +	 * have no ACPI companion and no device tree node, match them by the
> +	 * modalias their creator gave them.
> +	 */
Drop comment.

> +	serdev = to_serdev_device(dev);
> +
> +	return !!serdev_match_id(to_serdev_device_driver(drv)->id_table,
> +				 serdev->modalias);
I think you can drop the `is_serdev_device` check and use

```
const struct serdev_device *serdev = to_serdev_device(dev);
const struct serdev_device_driver *sdrv = to_serdev_device_driver(drv);
```

in the top.

The function `serdev_device_match` is only used inside the match
callback on the bus type definition. Other bus devices also do not have
this check. It should be safe.

Alternatively `to_serdev_device` in the same expression as the function
call (like with `to_serdev_device_driver`). It should be consistent
(either both as variables or none).

>  }
>  
>  /**
> diff --git a/include/linux/device-id/serdev.h b/include/linux/device-id/serdev.h
> new file mode 100644
> index 000000000000..1c3b03b7d501
> --- /dev/null
> +++ b/include/linux/device-id/serdev.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef LINUX_DEVICE_ID_SERDEV_H
> +#define LINUX_DEVICE_ID_SERDEV_H
> +
> +/* serdev */
> +
> +/* Large enough to hold the KBUILD_MODNAME of any serdev client driver */
Is this relevant for anyone reading the code? If not, drop the comment.

> +#define SERDEV_NAME_SIZE	40
> +#define SERDEV_MODULE_PREFIX	"serial:"
> +
> +struct serdev_device_id {
> +	char name[SERDEV_NAME_SIZE];
> +	unsigned long driver_data;	/* Data private to the driver */
> +};
In almost any other include/linux/device-id/*.h file, there is a

```
#ifdef __KERNEL__
#include <linux/types.h>
typedef unsigned long kernel_ulong_t;
#endif
```

statement and `kernel_ulong_t` is used.

I don't know why, but there is likely a reason for it. Thus do it the
same way, unless you can explain why this isn't needed here.

> +
> +#endif /* ifndef LINUX_DEVICE_ID_SERDEV_H */
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index b6c3d957ec15..9f91aa3e875d 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -7,7 +7,9 @@
>  
>  #include <linux/types.h>
>  #include <linux/device.h>
> +#include <linux/device-id/serdev.h>
>  #include <linux/iopoll.h>
> +#include <linux/string.h>
>  #include <linux/uaccess.h>
>  #include <linux/termios.h>
>  #include <linux/delay.h>
> @@ -35,6 +37,10 @@ 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.
> + * @modalias:	Name of the driver to use with this device, or the alias
> + *		for it. Empty for devices enumerated from firmware, which
> + *		are identified through their ACPI companion or their device
> + *		tree node instead. See serdev_device_set_modalias().
>   * @ctrl:	serdev controller managing this device.
>   * @ops:	Device operations.
>   * @write_comp:	Completion used by serdev_device_write() internally
> @@ -43,6 +49,7 @@ struct serdev_device_ops {
>  struct serdev_device {
>  	struct device dev;
>  	int nr;
> +	char modalias[SERDEV_NAME_SIZE];
>  	struct serdev_controller *ctrl;
>  	const struct serdev_device_ops *ops;
>  	struct completion write_comp;
> @@ -58,12 +65,15 @@ struct serdev_device {
>   * @probe:	binds this driver to a serdev device.
>   * @remove:	unbinds this driver from the serdev device.
>   * @shutdown:	shut down this serdev device.
> + * @id_table:	List of serdev devices supported by this driver, used to
> + *		match devices which are not enumerated from firmware.
>   */
>  struct serdev_device_driver {
>  	struct device_driver driver;
>  	int	(*probe)(struct serdev_device *);
>  	void	(*remove)(struct serdev_device *);
>  	void	(*shutdown)(struct serdev_device *);
> +	const struct serdev_device_id *id_table;
Just a suggestion:
Move it directly under driver, so the fields (driver->of_match_table,
driver->acpi_match_table, id_table) are next to each other. It has no
real runtime difference, so I won't complain if you leave it there.

>  };
>  
>  #define to_serdev_device_driver(d) container_of_const(d, struct serdev_device_driver, driver)
> @@ -135,6 +145,30 @@ static inline void serdev_device_set_client_ops(struct serdev_device *serdev,
>  	serdev->ops = ops;
>  }
>  
> +/**
> + * serdev_device_set_modalias() - set the modalias of a serdev device
> + * @serdev:	serdev device.
> + * @modalias:	non-empty modalias 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 modalias 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 modalias before serdev_device_add() to be
> + * identifiable. Doing so reports a SERDEV_MODULE_PREFIX modalias to
> + * userspace and makes the device match an id_table entry of a serdev
> + * device driver.
> + *
> + * Devices enumerated from firmware have no modalias of their own and keep
> + * deriving their modalias and their driver match from firmware only. The
> + * device name is not affected either way.
> + */
> +static inline void serdev_device_set_modalias(struct serdev_device *serdev,
> +					      const char *modalias)
> +{
> +	strscpy(serdev->modalias, modalias);
> +}
Drop the function.
Add `const char *modalias` as parameter to `serdev_device_add`. It
should be clear that any driver calling this function manually should
provide a modalias. Also, keep documentation simple (unless there is
need).

Either you add `__serdev_device_add` for internal use (of or acpi), or
it will use NULL. Your choice.


Overall, looks pretty good so far.

Thanks
- Markus Probst

> +
>  static inline
>  void *serdev_controller_get_drvdata(const struct serdev_controller *ctrl)
>  {

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