Re: [PATCH v3 2/2] i2c: Add i2c_device_get_match_data() callback

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

On Tue, Aug 01, 2023 at 06:03:18PM +0100, Biju Das wrote:
> Add i2c_device_get_match_data() callback to struct bus_type().
> 
> While at it, introduced i2c_get_match_data_helper() to avoid code
> duplication with i2c_get_match_data().

Have you used --patience when prepared the patch for sending?

...

> -const void *i2c_get_match_data(const struct i2c_client *client)
> +static const void *i2c_get_match_data_helper(const struct i2c_driver *driver,
> +					     const struct i2c_client *client)
>  {

> -	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);

Does it make sense to remove and add an additional parameter? In one case it's
a copy, in another it probably the same, just hidden in the code.

>  	const struct i2c_device_id *match;
> +
> +	match = i2c_match_id(driver->id_table, client);
> +	if (!match)
> +		return NULL;
> +
> +	return (const void *)match->driver_data;
> +}
> +
> +static const void *i2c_device_get_match_data(const struct device *dev)
> +{
> +	/* 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;

> +	if (!dev->driver)
> +		return NULL;

When can this be true?

> +	data = i2c_get_match_data_helper(to_i2c_driver(dev->driver), client);
> +	if (data)
> +		return data;
>  
> -		data = (const void *)match->driver_data;
> +	if (dev->driver->of_match_table) {
> +		const struct of_device_id *match;
> +
> +		match = i2c_of_match_device_sysfs(dev->driver->of_match_table,
> +						  (struct i2c_client *)client);
> +		if (match)
> +			data = match->data;
>  	}
>  
>  	return data;
>  }

...

> -static const struct of_device_id*
> +const struct of_device_id*

While here, add a missing space after of_device_id.

...

> +const struct of_device_id*

Ditto.

Or use below (weird) style in case it occurs more often than usual one.

> +i2c_of_match_device_sysfs(const struct of_device_id *matches,
> +			  struct i2c_client *client);
> +
>  const struct of_device_id
>  *i2c_of_match_device(const struct of_device_id *matches,
>  		     struct i2c_client *client);

...

> +static inline const struct of_device_id
> +*i2c_of_match_device(const struct of_device_id *matches,

As per above.

> +		     struct i2c_client *client)
> +{
> +	return NULL;
> +}

-- 
With Best Regards,
Andy Shevchenko