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 <[email protected]>
---
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
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/[email protected]/
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: [email protected]
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 <[email protected]>
> ---
> 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
>
>
>
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/[email protected]/
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: [email protected]
>
>
> 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 <[email protected]>
>> ---
>> 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
>>
>>
>>
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
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