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

From: Hans de Goede <hdegoede@redhat.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Mark Gross <mgross@linux.intel.com>,
	platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v1 3/4] platform/x86: i2c-multi-instantiate: Make number of clients unsigned
Date: Mon, 9 Nov 2020 12:39:45 +0100	[thread overview]
Message-ID: <f1ca37a0-85db-0edd-7f4c-14143254ebfe@redhat.com> (raw)
In-Reply-To: <20201105110530.27888-3-andriy.shevchenko@linux.intel.com>

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]);
> 