[PATCH] staging: iio: cdc: ad7746: Fix unnecessary check and assignment in ad7746_probe()

[PATCH] staging: iio: cdc: ad7746: Fix unnecessary check and assignment in ad7746_probe()

[Tang Bin]

In the function ad7746_probe(), the return value of
devm_iio_device_register() can be zero or ret, thus it is
unnecessary to repeated check here. And delete unused
initialized value of 'ret', because it will be assigned by
the function i2c_smbus_write_byte_data().

Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
---
 drivers/staging/iio/cdc/ad7746.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index dfd71e99e..d3b6e68df 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -680,7 +680,7 @@ static int ad7746_probe(struct i2c_client *client,
 	struct ad7746_chip_info *chip;
 	struct iio_dev *indio_dev;
 	unsigned char regval = 0;
-	int ret = 0;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
 	if (!indio_dev)
@@ -730,11 +730,7 @@ static int ad7746_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
-	if (ret)
-		return ret;
-
-	return 0;
+	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
 }
 
 static const struct i2c_device_id ad7746_id[] = {
-- 
2.20.1.windows.1

Re: [PATCH] staging: iio: cdc: ad7746: Fix unnecessary check and assignment in ad7746_probe()

[Marcelo Schmitt]

Hi Tang,

The patch looks overall good, though I think it could be split into two
pieces: one for simplifying ret declaration and another for removing the
check after device register.
Despite that, I guess Lucas might already be working on similar changes.
https://lore.kernel.org/linux-iio/cover.1620766020.git.lucas.p.stankus@gmail.com/

As general advice, I would recommend avoiding using generic words such
as fix in the subject line. It's often better to say something about the
nature of what is being done.


Cc: lucas.p.stankus@gmail.com


Best regards,

Marcelo

On 05/17, Tang Bin wrote:
> In the function ad7746_probe(), the return value of
> devm_iio_device_register() can be zero or ret, thus it is
> unnecessary to repeated check here. And delete unused
> initialized value of 'ret', because it will be assigned by
> the function i2c_smbus_write_byte_data().
> 
> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
> ---
>  drivers/staging/iio/cdc/ad7746.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index dfd71e99e..d3b6e68df 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -680,7 +680,7 @@ static int ad7746_probe(struct i2c_client *client,
>  	struct ad7746_chip_info *chip;
>  	struct iio_dev *indio_dev;
>  	unsigned char regval = 0;
> -	int ret = 0;
> +	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>  	if (!indio_dev)
> @@ -730,11 +730,7 @@ static int ad7746_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>  }
>  
>  static const struct i2c_device_id ad7746_id[] = {
> -- 
> 2.20.1.windows.1
> 
> 
> 

Re: [PATCH] staging: iio: cdc: ad7746: Fix unnecessary check andassignment in ad7746_probe()

[tangbin]

Hi Marcelo:

On 2021/5/18 6:14, Marcelo Schmitt wrote:
> Hi Tang,
>
> The patch looks overall good, though I think it could be split into two
> pieces: one for simplifying ret declaration and another for removing the
> check after device register.
> Despite that, I guess Lucas might already be working on similar changes.
> https://lore.kernel.org/linux-iio/cover.1620766020.git.lucas.p.stankus@gmail.com/

Thanks for your reply, I really don't know someone has send the similar one.

I forget this patch.


> As general advice, I would recommend avoiding using generic words such
> as fix in the subject line. It's often better to say something about the
> nature of what is being done.

OK, got it!

Thanks

Tang Bin

>
> Cc: lucas.p.stankus@gmail.com
>
>
> Best regards,
>
> Marcelo
>
> On 05/17, Tang Bin wrote:
>> In the function ad7746_probe(), the return value of
>> devm_iio_device_register() can be zero or ret, thus it is
>> unnecessary to repeated check here. And delete unused
>> initialized value of 'ret', because it will be assigned by
>> the function i2c_smbus_write_byte_data().
>>
>> Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
>> ---
>>   drivers/staging/iio/cdc/ad7746.c | 8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
>> index dfd71e99e..d3b6e68df 100644
>> --- a/drivers/staging/iio/cdc/ad7746.c
>> +++ b/drivers/staging/iio/cdc/ad7746.c
>> @@ -680,7 +680,7 @@ static int ad7746_probe(struct i2c_client *client,
>>   	struct ad7746_chip_info *chip;
>>   	struct iio_dev *indio_dev;
>>   	unsigned char regval = 0;
>> -	int ret = 0;
>> +	int ret;
>>   
>>   	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>>   	if (!indio_dev)
>> @@ -730,11 +730,7 @@ static int ad7746_probe(struct i2c_client *client,
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>> -	if (ret)
>> -		return ret;
>> -
>> -	return 0;
>> +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>>   }
>>   
>>   static const struct i2c_device_id ad7746_id[] = {
>> -- 
>> 2.20.1.windows.1
>>
>>
>>

Re: [PATCH] staging: iio: cdc: ad7746: Fix unnecessary check and assignment in ad7746_probe()

[Dan Carpenter]

On Mon, May 17, 2021 at 11:00:06PM +0800, Tang Bin wrote:
> @@ -730,11 +730,7 @@ static int ad7746_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>  }

This sort of thing is done deliberately as a style choice...  I probably
wouldn't have written it that way myself, but there really isn't a
downside to leaving it as-is.

The unused "int ret = 0;" just introduces a static checker warning about
unused assignments and disables the static checker warning for
uninitialized variables so we want to remove that.

regards,
dan carpenter

Re: [PATCH] staging: iio: cdc: ad7746: Fix unnecessary check andassignment in ad7746_probe()

[tangbin]

Hi Dan：

On 2021/5/18 15:52, Dan Carpenter wrote:
> On Mon, May 17, 2021 at 11:00:06PM +0800, Tang Bin wrote:
>> @@ -730,11 +730,7 @@ static int ad7746_probe(struct i2c_client *client,
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>> -	if (ret)
>> -		return ret;
>> -
>> -	return 0;
>> +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>>   }
> This sort of thing is done deliberately as a style choice...  I probably
> wouldn't have written it that way myself, but there really isn't a
> downside to leaving it as-is.
>
> The unused "int ret = 0;" just introduces a static checker warning about
> unused assignments and disables the static checker warning for
> uninitialized variables so we want to remove that.
>
Got it, I will send this patch for you.

Thanks

Tang Bin

Re: [PATCH] staging: iio: cdc: ad7746: Fix unnecessary check andassignment in ad7746_probe()

[Jonathan Cameron]

On Tue, 18 May 2021 17:27:07 +0800
tangbin <tangbin@cmss.chinamobile.com> wrote:

> Hi Dan：
> 
> On 2021/5/18 15:52, Dan Carpenter wrote:
> > On Mon, May 17, 2021 at 11:00:06PM +0800, Tang Bin wrote:  
> >> @@ -730,11 +730,7 @@ static int ad7746_probe(struct i2c_client *client,
> >>   	if (ret < 0)
> >>   		return ret;
> >>   
> >> -	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> >> -	if (ret)
> >> -		return ret;
> >> -
> >> -	return 0;
> >> +	return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> >>   }  
> > This sort of thing is done deliberately as a style choice...  I probably
> > wouldn't have written it that way myself, but there really isn't a
> > downside to leaving it as-is.
> >
> > The unused "int ret = 0;" just introduces a static checker warning about
> > unused assignments and disables the static checker warning for
> > uninitialized variables so we want to remove that.
> >  
> Got it, I will send this patch for you.

I fall a bit different on this and would consider the above a cleanup
though one I'd prefer to get with more significant stuff rather
than on it's own.  However, there is already a patch in revision
that includes the same change from Lucas Stankus.

> 
> Thanks
> 
> Tang Bin
> 
> 
>