Re: [PATCH v2 02/10] serdev: Add serdev device based driver match support

[Manivannan Sadhasivam]

On Thu, Nov 27, 2025 at 06:32:04AM -0800, Bartosz Golaszewski wrote:
> On Tue, 25 Nov 2025 15:45:06 +0100, Manivannan Sadhasivam via B4 Relay
> <devnull+manivannan.sadhasivam.oss.qualcomm.com@xxxxxxxxxx> said:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> >
> > Add support to match serdev devices with serdev drivers based on the serdev
> > ID table defined in serdev_device_driver::id_table.
> >
> > The matching function, serdev_driver_match_device() uses the serdev device
> > name to match against the entries in serdev_device_driver::id_table.
> >
> > If there is no serdev id_table for the driver, then serdev_device_match()
> > will fallback to ACPI and DT based matching.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/tty/serdev/core.c         | 23 ++++++++++++++++++++++-
> >  include/linux/mod_devicetable.h   |  7 +++++++
> >  include/linux/serdev.h            |  4 ++++
> >  scripts/mod/devicetable-offsets.c |  3 +++
> >  4 files changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> > index b33e708cb245..2b5582cd5063 100644
> > --- a/drivers/tty/serdev/core.c
> > +++ b/drivers/tty/serdev/core.c
> > @@ -85,12 +85,33 @@ static const struct device_type serdev_ctrl_type = {
> >  	.release	= serdev_ctrl_release,
> >  };
> >
> > +static int serdev_driver_match_device(struct device *dev, const struct device_driver *drv)
> > +{
> > +	const struct serdev_device_driver *serdev_drv = to_serdev_device_driver(drv);
> > +	struct serdev_device *serdev = to_serdev_device(dev);
> > +	const struct serdev_device_id *id;
> > +
> > +	if (!serdev_drv->id_table)
> > +		return 0;
> > +
> > +	for (id = serdev_drv->id_table; id->name[0]; id++) {
> > +		if (!strcmp(dev_name(dev), id->name)) {
> > +			serdev->id = id;
> > +			return 1;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> I don't know if Rob agrees with me but I would very much prefer to see
> software-node-based approach instead of an ID table matching.
> 
> Could you in the pwrseq driver, create a software node for the serdev device
> you allocate, set its "compatible" to "qcom,wcn7850-bt" and match against it
> here?
> 
> This has several benefits: if you ever need to pass more properties to the
> serdev devices, you already have a medium for that and you can also leave
> serdev_device_add() alone. You're comparing the entire name here - what if
> someone sets device's ID to some value and the name will be "WCN7850.2"?
> 
> You could also drop the serdev_id field from struct serdev_device. For matching
> you could even reuse the of_device_id from the device driver.
> 

I tried this approach and I really liked it since it gets rid of the yet-another
id_table for serdev (which I didn't like it btw). But there is one concern
though. We need a generic 'device_get_match_data' implementation for swnode.
While trying to implement it, I stumbled upon this patch [1] which does the same
for other usecase, but there was a disagreement on whether swnode should be used
for driver matching or not. For my usecase, I find it very useful and
reasonable, but Dmitry Torokhov believes otherwise.

Maybe I'll include this patch in the next version, CC Dmitry and see where it
goes.

> Which also makes me think that maybe we should finally think about a generic,
> fwnode-based device driver matching in the driver model...
> 

Yes, that would be useful too and will allow me to get rid of the custom
matching logic in serdev core.

- Mani

[1] https://lore.kernel.org/all/20240427203650.582989-1-sui.jingfeng@xxxxxxxxx

-- 
மணிவண்ணன் சதாசிவம்