| Message ID | 20210517150006.8436-1-tangbin@cmss.chinamobile.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | staging: iio: cdc: ad7746: Fix unnecessary check and assignment in ad7746_probe() | expand |
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 > > >
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 >> >> >>
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
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 > > >
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[] = {
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(-)