RE: [PATCH v5 RESEND 2/4] i2c: Add i2c_device_get_match_data() callback

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v5 RESEND 2/4] i2c: Add i2c_device_get_match_data()
> callback
> 
> On Thu, Aug 03, 2023 at 11:31:00AM +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().
> 
> It seems you are missing to Cc Andi for all these... (not your fault,
> rather unfortunately).
> 
> Yes, while he is not directly involved into core changes the drivers are
> pretty much should consider this change.

Sure.

> 
> ...
> 
> >  	data = device_get_match_data(&client->dev);
> > -	if (!data) {
> > -		match = i2c_match_id(driver->id_table, client);
> > -		if (!match)
> > -			return NULL;
> > +	if (data)
> > +		return data;
> >
> > -		data = (const void *)match->driver_data;
> > -	}
> > -
> > -	return data;
> 
> Looking at this, it _might_ make sense to split another patch to prepare
> for better difference here.

OK.

> 
>  -	if (!data) {
>  -		match = i2c_match_id(driver->id_table, client);
>  -		if (!match)
>  -			return NULL;
>  +	if (data)
>  +		return data;
>  +
>  +	match = i2c_match_id(driver->id_table, client);
>  +	if (!match)
>  +		return NULL;
>  +
>  +	return (const void *)match->driver_data;
> 
>  Just play with this idea.

Does these below 2 patches ok?

PATCH x:
-------
Subject: [PATCH 1/2] i2c: Enhance i2c_get_match_data()

Enhance i2c_get_match_data() for a faster path for device_get_
match_data().

While at it, add const to struct i2c_driver to prevent overriding
the driver pointer.

Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
---
 drivers/i2c/i2c-core-base.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 60746652fd52..7005dfe64066 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -116,20 +116,19 @@ EXPORT_SYMBOL_GPL(i2c_match_id);
 
 const void *i2c_get_match_data(const struct i2c_client *client)
 {
-	struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
+	const struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
 	const struct i2c_device_id *match;
 	const void *data;
 
 	data = device_get_match_data(&client->dev);
-	if (!data) {
-		match = i2c_match_id(driver->id_table, client);
-		if (!match)
-			return NULL;
+	if (data)
+		return data;
 
-		data = (const void *)match->driver_data;
-	}
+	match = i2c_match_id(driver->id_table, client);
+	if (!match)
+		return NULL;
 
-	return data;
+	return (const void *)match->driver_data;
 }
 EXPORT_SYMBOL(i2c_get_match_data);

Patch x+1:
---------

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

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().

Suggested-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
Suggested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
---
 drivers/i2c/i2c-core-base.c | 53 ++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 7005dfe64066..d543460e47c2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -114,15 +114,10 @@ const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
 }
 EXPORT_SYMBOL_GPL(i2c_match_id);
 
-const void *i2c_get_match_data(const struct i2c_client *client)
+static const void *i2c_get_match_data_helper(const struct i2c_client *client)
 {
 	const struct i2c_driver *driver = to_i2c_driver(client->dev.driver);
 	const struct i2c_device_id *match;
-	const void *data;
-
-	data = device_get_match_data(&client->dev);
-	if (data)
-		return data;
 
 	match = i2c_match_id(driver->id_table, client);
 	if (!match)
@@ -130,6 +125,51 @@ const void *i2c_get_match_data(const struct i2c_client *client)
 
 	return (const void *)match->driver_data;
 }
+
+static const void *i2c_device_get_match_data(const struct device *dev)
+{
+	const struct device_driver *drv = dev->driver;
+	const struct i2c_client *client;
+	const void *data;
+
+	/*
+	 * It is not guaranteed that the function is always called on a device
+	 * bound to a driver (even though we normally expect this to be the
+	 * case).
+	 */
+	if (!drv)
+		return NULL;
+
+	/* TODO: use i2c_verify_client() when it accepts const pointer */
+	client = (dev->type == &i2c_client_type) ? to_i2c_client(dev) : NULL;
+	if (!client)
+		return NULL;
+
+	data = i2c_get_match_data_helper(client);
+	if (data)
+		return data;
+
+	if (drv->of_match_table) {
+		const struct of_device_id *match;
+
+		match = i2c_of_match_device_sysfs(drv->of_match_table, client);
+		if (match)
+			return match->data;
+	}
+
+	return NULL;
+}
+
+const void *i2c_get_match_data(const struct i2c_client *client)
+{
+	const void *data;
+
+	data = device_get_match_data(&client->dev);
+	if (data)
+		return data;
+
+	return i2c_get_match_data_helper(client);
+}
 EXPORT_SYMBOL(i2c_get_match_data);
 
 static int i2c_device_match(struct device *dev, struct device_driver *drv)
@@ -694,6 +734,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);

Cheers,
Biju





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux