RE: [PATCH v2 1/2] Input: exc3000 - Simplify probe()

[Biju Das]

Hi Dmitry Torokhov,

Thanks for the feedback.

> Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
> 
> On Wed, Jul 19, 2023 at 06:43:47AM +0000, Biju Das wrote:
> > Hi Dmitry Torokhov,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
> > >
> > > On Mon, Jul 17, 2023 at 06:45:27PM +0000, Biju Das wrote:
> > > > Hi Dmitry,
> > > >
> > > > > Subject: Re: [PATCH v2 1/2] Input: exc3000 - Simplify probe()
> > > > >
> > > > > On Mon, Jul 17, 2023 at 07:15:50PM +0100, Mark Brown wrote:
> > > > > > On Mon, Jul 17, 2023 at 04:35:02PM +0000, Biju Das wrote:
> > > > > >
> > > > > > > The .device_get_match_data callbacks are missing for I2C and
> > > > > > > SPI bus
> > > > > subsystems.
> > > > > > > Can you please throw some lights on this?
> > > > > >
> > > > > > It's the first time I've ever heard of that callback, I don't
> > > > > > know why whoever added it wouldn't have done those buses in
> > > > > > particular or if it just didn't happen.  Try adding it and if
> > > > > > it works send
> > > the patches?
> > > > >
> > > > > I think there is a disconnect. Right now device_get_match_data
> > > > > callbacks are part of fwnode_operations. I was proposing to add
> > > > > another optional device_get_match_data callback to 'struct
> bus_type'
> > > > > to allow individual buses control how match data is handled,
> > > > > before (or after) jumping into the fwnode-backed
> > > > > device_get_match_data
> > > callbacks.
> > > >
> > > > That is what implemented here [1] and [2] right?
> > > >
> > > >
> > > > First it check for fwnode-backed device_get_match_data callbacks
> > > > and Fallback is bus-type based match.
> > > >
> > > > Looks like you are proposing to unify [1] and [2] and you want the
> > > > logic to be other way around. ie, first bus-type match, then
> > > > fwnode-backed callbacks?
> > > >
> > >
> > > I do not have a strong preference for the ordering, i.e. I think it
> > > is perfectly fine to do the generic fwnode-based lookup and if there
> > > is no match have bus method called as a fallback,
> >
> > That involves a bit of work.
> >
> > const void *device_get_match_data(const struct device *dev);
> >
> > const struct i2c_device_id *i2c_match_id(const struct i2c_device_id
> *id,
> > 					 const struct i2c_client *client);
> >
> > const struct spi_device_id *spi_get_device_id(const struct spi_device
> > *sdev);
> >
> > Basically, the bus-client driver(such as exc3000) needs to pass struct
> > device and device_get_match_data after generic fwnode-based lookup,
> > needs to find the bus type based on struct device and call a new
> > generic
> > void* bus_get_match_data(void*) callback, so that each bus interface
> > can do a match.
> 
> Yes, something like this (which does not seem that involved to me...):

Looks it will work.

But there is some 2 additional checks in core code, every driver which is not bus type need to go through this checks.

Also in Bus specific callback, there are 2 additional checks.

So, performance wise [1] is better.

Moreover, we need to avoid code duplication with [1]

[1] https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/i2c/i2c-core-base.c#L125

What core people thinking about Dmitry's proposal?

Cheers,
Biju


> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c index
> 8c40abed7852..cc0bf7bb6f3a 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1277,7 +1277,13 @@ EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
> 
>  const void *device_get_match_data(const struct device *dev)  {
> -	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data,
> dev);
> +	const void *data;
> +
> +	data = fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data,
> dev);
> +	if (!data && dev->bus && dev->bus->get_match_data)
> +		data = dev->bus->get_match_data(dev);
> +
> +	return data;
>  }
>  EXPORT_SYMBOL_GPL(device_get_match_data);
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 60746652fd52..5fe47bc491a6 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -114,6 +114,26 @@ const struct i2c_device_id *i2c_match_id(const
> struct i2c_device_id *id,  }  EXPORT_SYMBOL_GPL(i2c_match_id);
> 
> +static const void *i2c_device_get_match_data(const struct device *dev)
> +{
> +	const struct i2c_client *client = to_i2c_client(dev);
> +	const struct i2c_driver *driver;
> +	const struct i2c_device_id *match;
> +
> +	if (!dev->driver)
> +		return NULL;
> +
> +	driver = to_i2c_driver(dev->driver);
> +	if (!driver)
> +		return NULL;
> +
> +	match = i2c_match_id(driver->id_table, client);
> +	if (!match)
> +		return NULL;
> +
> +	return (const void *)match->driver_data; }
> +
>  const void *i2c_get_match_data(const struct i2c_client *client)  {
>  	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
> @@ -695,6 +715,7 @@ struct bus_type i2c_bus_type = {
>  	.probe		= i2c_device_probe,
>  	.remove		= i2c_device_remove,
>  	.shutdown	= i2c_device_shutdown,
> +	.get_match_data	= i2c_device_get_match_data,
>  };
>  EXPORT_SYMBOL_GPL(i2c_bus_type);
> 
> diff --git a/include/linux/device/bus.h b/include/linux/device/bus.h
> index ae10c4322754..3f2cba28a1af 100644
> --- a/include/linux/device/bus.h
> +++ b/include/linux/device/bus.h
> @@ -102,6 +102,8 @@ struct bus_type {
>  	int (*dma_configure)(struct device *dev);
>  	void (*dma_cleanup)(struct device *dev);
> 
> +	const void *(*get_match_data)(const struct device *dev);
> +
>  	const struct dev_pm_ops *pm;
> 
>  	const struct iommu_ops *iommu_ops;
> 
> 
> Thanks.
> 
> --
> Dmitry