| Message ID | 20201105110530.27888-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | [v1,1/4] platform/x86: i2c-multi-instantiate: Drop redundant ACPI_PTR() | expand |
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]); >
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!
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
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]);
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(-)