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

[Hans de Goede <hansg@kernel.org>]

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 ?

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.

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