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

[An Wu]

Hi Markus
    Thank you for your review.
For this,
```
#ifdef __KERNEL__
#include <linux/types.h>
typedef unsigned long kernel_ulong_t;
#endif
```

I originally implemented it this way, but checkpatch.pl reported a warning.
Since it's only a warning, I'll go ahead and update it accordingly.
I really appreciate your patience and guidance. Thank you!

BR
    An

On Fri, Oct 23, 2026 at 8:26 PM Markus Probst <markus.probst@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
> >  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)
> >  {