[PATCH v1 1/4] platform/x86: i2c-multi-instantiate: Drop redundant ACPI_PTR()

[PATCH v1 1/4] platform/x86: i2c-multi-instantiate: Drop redundant ACPI_PTR()

[Andy Shevchenko]

The driver depends on ACPI, ACPI_PTR() resolution is always the same.
Otherwise a compiler may produce a warning.

That said, the rule of thumb either ugly ifdeffery with ACPI_PTR or
none should be used in a driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 6acc8457866e..d92d9a98ec8f 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -189,7 +189,7 @@ MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids);
 static struct platform_driver i2c_multi_inst_driver = {
 	.driver	= {
 		.name = "I2C multi instantiate pseudo device driver",
-		.acpi_match_table = ACPI_PTR(i2c_multi_inst_acpi_ids),
+		.acpi_match_table = i2c_multi_inst_acpi_ids,
 	},
 	.probe = i2c_multi_inst_probe,
 	.remove = i2c_multi_inst_remove,
-- 
2.28.0

[PATCH v1 2/4] platform/x86: i2c-multi-instantiate: Simplify with dev_err_probe()

[Andy Shevchenko]

Common pattern of handling deferred probe can be simplified with
dev_err_probe(). Less code and the error value gets printed.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index d92d9a98ec8f..ce4d921c3301 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -118,9 +118,8 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 		}
 		multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
 		if (IS_ERR(multi->clients[i])) {
-			ret = PTR_ERR(multi->clients[i]);
-			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "Error creating i2c-client, idx %d\n", i);
+			ret = dev_err_probe(dev, PTR_ERR(multi->clients[i]),
+					    "Error creating i2c-client, idx %d\n", i);
 			goto error;
 		}
 	}
-- 
2.28.0

[PATCH v1 3/4] platform/x86: i2c-multi-instantiate: Make number of clients unsigned

[Andy Shevchenko]

There is no need to use signed type for number of clients. Moreover,
it's cleaner to show that we never go negative there.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index ce4d921c3301..422fa88da643 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -27,7 +27,7 @@ struct i2c_inst_data {
 };
 
 struct i2c_multi_inst_data {
-	int num_clients;
+	unsigned int num_clients;
 	struct i2c_client *clients[];
 };
 
@@ -64,8 +64,9 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 	struct i2c_board_info board_info = {};
 	struct device *dev = &pdev->dev;
 	struct acpi_device *adev;
+	unsigned int i;
 	char name[32];
-	int i, ret;
+	int ret;
 
 	match = acpi_match_device(dev->driver->acpi_match_table, dev);
 	if (!match) {
@@ -90,7 +91,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 	for (i = 0; i < multi->num_clients && inst_data[i].type; i++) {
 		memset(&board_info, 0, sizeof(board_info));
 		strlcpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE);
-		snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev),
+		snprintf(name, sizeof(name), "%s-%s.%u", dev_name(dev),
 			 inst_data[i].type, i);
 		board_info.dev_name = name;
 		switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
@@ -119,12 +120,12 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 		multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
 		if (IS_ERR(multi->clients[i])) {
 			ret = dev_err_probe(dev, PTR_ERR(multi->clients[i]),
-					    "Error creating i2c-client, idx %d\n", i);
+					    "Error creating i2c-client, idx %u\n", i);
 			goto error;
 		}
 	}
 	if (i < multi->num_clients) {
-		dev_err(dev, "Error finding driver, idx %d\n", i);
+		dev_err(dev, "Error finding driver, idx %u\n", i);
 		ret = -ENODEV;
 		goto error;
 	}
@@ -133,7 +134,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 	return 0;
 
 error:
-	while (--i >= 0)
+	while (i--)
 		i2c_unregister_device(multi->clients[i]);
 
 	return ret;
@@ -142,7 +143,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 static int i2c_multi_inst_remove(struct platform_device *pdev)
 {
 	struct i2c_multi_inst_data *multi = platform_get_drvdata(pdev);
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < multi->num_clients; i++)
 		i2c_unregister_device(multi->clients[i]);
-- 
2.28.0

Re: [PATCH v1 3/4] platform/x86: i2c-multi-instantiate: Make number of clients unsigned

[Hans de Goede]

Hi,

On 11/5/20 12:05 PM, Andy Shevchenko wrote:
> There is no need to use signed type for number of clients. Moreover,
> it's cleaner to show that we never go negative there.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I'm not a big fan of this change, it feels like needless churn to me.

Integers are signed by default and just because a value cannot become
negative is not really a reason to make it unsigned. E.g. your typical
"int i" is often used as an array index so it cannot go negative, still
it almost always is an "int i" not an "unsigned int i".

IMHO good reasons for deviating from the default signedness and
making a value unsigned are:

1. Because the value cannot go negative and we need more range.
2. To avoid sign-extension when upcasting it to a larger integer type.

Neither is the case here.

I do like the other 3 patches, thank you for those. I'm going to wait
a bit with applying them though, to see where things go with the
"[RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients"

Merging them now may get in the way with merging that series if
Wolfram wants to pick up the entire series (since it also involves
an i2c-core change).

Regards,

Hans




> ---
>  drivers/platform/x86/i2c-multi-instantiate.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
> index ce4d921c3301..422fa88da643 100644
> --- a/drivers/platform/x86/i2c-multi-instantiate.cHi,
> +++ b/drivers/platform/x86/i2c-multi-instantiate.c
> @@ -27,7 +27,7 @@ struct i2c_inst_data {
>  };
>  
>  struct i2c_multi_inst_data {
> -	int num_clients;
> +	unsigned int num_clients;
>  	struct i2c_client *clients[];
>  };
>  
> @@ -64,8 +64,9 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
>  	struct i2c_board_info board_info = {};
>  	struct device *dev = &pdev->dev;
>  	struct acpi_device *adev;
> +	unsigned int i;
>  	char name[32];
> -	int i, ret;
> +	int ret;
>  
>  	match = acpi_match_device(dev->driver->acpi_match_table, dev);
>  	if (!match) {
> @@ -90,7 +91,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
>  	for (i = 0; i < multi->num_clients && inst_data[i].type; i++) {
>  		memset(&board_info, 0, sizeof(board_info));
>  		strlcpy(board_info.type, inst_data[i].type, I2C_NAME_SIZE);
> -		snprintf(name, sizeof(name), "%s-%s.%d", dev_name(dev),
> +		snprintf(name, sizeof(name), "%s-%s.%u", dev_name(dev),
>  			 inst_data[i].type, i);
>  		board_info.dev_name = name;
>  		switch (inst_data[i].flags & IRQ_RESOURCE_TYPE) {
> @@ -119,12 +120,12 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
>  		multi->clients[i] = i2c_acpi_new_device(dev, i, &board_info);
>  		if (IS_ERR(multi->clients[i])) {
>  			ret = dev_err_probe(dev, PTR_ERR(multi->clients[i]),
> -					    "Error creating i2c-client, idx %d\n", i);
> +					    "Error creating i2c-client, idx %u\n", i);
>  			goto error;
>  		}
>  	}
>  	if (i < multi->num_clients) {
> -		dev_err(dev, "Error finding driver, idx %d\n", i);
> +		dev_err(dev, "Error finding driver, idx %u\n", i);
>  		ret = -ENODEV;
>  		goto error;
>  	}
> @@ -133,7 +134,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
>  	return 0;
>  
>  error:
> -	while (--i >= 0)
> +	while (i--)
>  		i2c_unregister_device(multi->clients[i]);
>  
>  	return ret;
> @@ -142,7 +143,7 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
>  static int i2c_multi_inst_remove(struct platform_device *pdev)
>  {
>  	struct i2c_multi_inst_data *multi = platform_get_drvdata(pdev);
> -	int i;
> +	unsigned int i;
>  
>  	for (i = 0; i < multi->num_clients; i++)
>  		i2c_unregister_device(multi->clients[i]);
> 

Re: [PATCH v1 3/4] platform/x86: i2c-multi-instantiate: Make number of clients unsigned

[Andy Shevchenko]

On Mon, Nov 09, 2020 at 12:39:45PM +0100, Hans de Goede wrote:
> On 11/5/20 12:05 PM, Andy Shevchenko wrote:
> > There is no need to use signed type for number of clients. Moreover,
> > it's cleaner to show that we never go negative there.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I'm not a big fan of this change, it feels like needless churn to me.

Feel free to not apply it. I think I don't need to resend w/o it (IIRC the rest
pretty much independent of this change). But if you need a v2, tell me.

> Integers are signed by default and just because a value cannot become
> negative is not really a reason to make it unsigned. E.g. your typical
> "int i" is often used as an array index so it cannot go negative, still
> it almost always is an "int i" not an "unsigned int i".
> 
> IMHO good reasons for deviating from the default signedness and
> making a value unsigned are:
> 
> 1. Because the value cannot go negative and we need more range.
> 2. To avoid sign-extension when upcasting it to a larger integer type.
> 
> Neither is the case here.

I consider one more, i.e. if we know that value may not be negative the
unsigned type gives a hint. I always stumbled over signed integers used for
loop counters since they confuse me (Q in mind: "should I read code carefully
and check if it may or may not be signed? Why it's signed?").

That's why I like the idea of be a bit stricter about types.

Hope this explains my motivation.

> I do like the other 3 patches, thank you for those. I'm going to wait
> a bit with applying them though, to see where things go with the
> "[RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients"
> 
> Merging them now may get in the way with merging that series if
> Wolfram wants to pick up the entire series (since it also involves
> an i2c-core change).

Usually I expect that RFC has less priority than normal series and I wouldn't
expect any maintainer (with some rare exceptions) to take series marked as RFC.
And TBH I was wondering why you marked them as such, to me that was fine to
send as normal one.

Thanks for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/4] platform/x86: i2c-multi-instantiate: Make number of clients unsigned

[Hans de Goede]

Hi,

On 11/9/20 12:53 PM, Andy Shevchenko wrote:
> On Mon, Nov 09, 2020 at 12:39:45PM +0100, Hans de Goede wrote:
>> On 11/5/20 12:05 PM, Andy Shevchenko wrote:
>>> There is no need to use signed type for number of clients. Moreover,
>>> it's cleaner to show that we never go negative there.
>>>
>>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>
>> I'm not a big fan of this change, it feels like needless churn to me.
> 
> Feel free to not apply it. I think I don't need to resend w/o it (IIRC the rest
> pretty much independent of this change). But if you need a v2, tell me.

No need for a v2.

>> Integers are signed by default and just because a value cannot become
>> negative is not really a reason to make it unsigned. E.g. your typical
>> "int i" is often used as an array index so it cannot go negative, still
>> it almost always is an "int i" not an "unsigned int i".
>>
>> IMHO good reasons for deviating from the default signedness and
>> making a value unsigned are:
>>
>> 1. Because the value cannot go negative and we need more range.
>> 2. To avoid sign-extension when upcasting it to a larger integer type.
>>
>> Neither is the case here.
> 
> I consider one more, i.e. if we know that value may not be negative the
> unsigned type gives a hint. I always stumbled over signed integers used for
> loop counters since they confuse me (Q in mind: "should I read code carefully
> and check if it may or may not be signed? Why it's signed?").
> 
> That's why I like the idea of be a bit stricter about types.
> 
> Hope this explains my motivation.

It does and I understand your point of view here.

>> I do like the other 3 patches, thank you for those. I'm going to wait
>> a bit with applying them though, to see where things go with the
>> "[RFC 0/4] platform/x86: i2c-multi-instantiate: Pass ACPI fwnode to instantiated i2c-clients"
>>
>> Merging them now may get in the way with merging that series if
>> Wolfram wants to pick up the entire series (since it also involves
>> an i2c-core change).
> 
> Usually I expect that RFC has less priority than normal series> and I wouldn't
> expect any maintainer (with some rare exceptions) to take series marked as RFC.

Right, but you suggested resending it as a non RFC, and then there is some
cross tree coordination which needs to happen to merge it; and since
this series is just cleanups with no functional changes I would prefer to
delay this one a bit to make the cross tree coordination simpler
(iow keeps i2c-multi-instatiate.c unmodified from rc1 for now).

I hope this explains why I'm delaying your cleanups a bit.

> And TBH I was wondering why you marked them as such, to me that was fine to
> send as normal one.

As I tried to explain in my reaction to the RFC cover-letter I'm not sure we
should apply those patches right now as there is no immediate need for
passing the fwnode and a non 0 chance of regressions. But lets discuss that
in that thread. If we decide to not apply that series for now then I'll apply
this series (minus patch 3) right away.

Regards,

Hans

[PATCH v1 4/4] platform/x86: i2c-multi-instantiate: Use device_get_match_data() to get driver data

[Andy Shevchenko]

Use device_get_match_data() to get driver data instead of boilerplate code.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/i2c-multi-instantiate.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c
index 422fa88da643..da50e9ec64a8 100644
--- a/drivers/platform/x86/i2c-multi-instantiate.c
+++ b/drivers/platform/x86/i2c-multi-instantiate.c
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/types.h>
 
 #define IRQ_RESOURCE_TYPE	GENMASK(1, 0)
@@ -59,7 +60,6 @@ static int i2c_multi_inst_count_resources(struct acpi_device *adev)
 static int i2c_multi_inst_probe(struct platform_device *pdev)
 {
 	struct i2c_multi_inst_data *multi;
-	const struct acpi_device_id *match;
 	const struct i2c_inst_data *inst_data;
 	struct i2c_board_info board_info = {};
 	struct device *dev = &pdev->dev;
@@ -68,12 +68,11 @@ static int i2c_multi_inst_probe(struct platform_device *pdev)
 	char name[32];
 	int ret;
 
-	match = acpi_match_device(dev->driver->acpi_match_table, dev);
-	if (!match) {
+	inst_data = device_get_match_data(dev);
+	if (!inst_data) {
 		dev_err(dev, "Error ACPI match data is missing\n");
 		return -ENODEV;
 	}
-	inst_data = (const struct i2c_inst_data *)match->driver_data;
 
 	adev = ACPI_COMPANION(dev);
 
-- 
2.28.0