Re: [PATCH v4 3/3] i2c: Extend i2c_device_get_match_data() to support i2c sysfs

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Aug 02, 2023 at 12:23:17PM +0100, Biju Das wrote:
> Extend i2c_device_get_match_data() to i2c sysfs interface by changing
> i2c_of_match_device_sysfs() to non-static and call this function as a
> fallback for i2c_get_match_data_helper().

...

>  	/* TODO: use i2c_verify_client() when it accepts const pointer */
>  	const struct i2c_client *client = (dev->type == &i2c_client_type) ?
>  					  to_i2c_client(dev) : NULL;
> +	const void *data;

Oh, yeah, definitely, please follow advice for previous patch I have given.

>  	if (!client || !dev->driver)
>  		return NULL;
>  
> -	return i2c_get_match_data_helper(client);
> +	data = i2c_get_match_data_helper(client);
> +	if (data)
> +		return data;
> +
> +	if (dev->driver->of_match_table) {
> +		const struct of_device_id *match;
> +
> +		match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
> +						  client);

So, with this it makes sense to have

	strucct device_driver *drv = dev->driver;
	...
	if (!drv)
		return NULL;

The above for the first patch even. And here

		match = i2c_of_match_device_sysfs(drv->of_match_table, client);

> +		if (match)
> +			return match->data;
> +	}

> +	return data;

This actually should be

	return NULL;

to avoid (potentially) stale pointers.

>  }

...

> -static const struct of_device_id*
> +const struct of_device_id *
>  i2c_of_match_device_sysfs(const struct of_device_id *matches,
> -				  struct i2c_client *client)
> +			  const struct i2c_client *client)
>  {
>  	const char *name;
>  
> @@ -141,6 +141,7 @@ i2c_of_match_device_sysfs(const struct of_device_id *matches,
>  
>  	return NULL;
>  }
> +EXPORT_SYMBOL_GPL(i2c_of_match_device_sysfs);
>  
>  const struct of_device_id
>  *i2c_of_match_device(const struct of_device_id *matches,
> diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
> index 1247e6e6e975..e4d397b67989 100644
> --- a/drivers/i2c/i2c-core.h
> +++ b/drivers/i2c/i2c-core.h
> @@ -82,8 +82,17 @@ static inline void i2c_acpi_remove_space_handler(struct i2c_adapter *adapter) {
>  
>  #ifdef CONFIG_OF
>  void of_i2c_register_devices(struct i2c_adapter *adap);
> +const struct of_device_id *
> +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> +			  const struct i2c_client *client);
>  #else
>  static inline void of_i2c_register_devices(struct i2c_adapter *adap) { }
> +static inline const struct of_device_id *
> +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> +			  const struct i2c_client *client)
> +{
> +	return NULL;
> +}
>  #endif
>  extern struct notifier_block i2c_of_notifier;

That's what above I meant to see as a separate change.

-- 
With Best Regards,
Andy Shevchenko