| Message ID | 687a1923add71303da13e3a838e97a6ffc7dcda7.1620766020.git.lucas.p.stankus@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: iio: cdc: ad7746: initial effort to move out of staging | expand |
Hi Lucas, On Tue, May 11, 2021 at 05:54:01PM -0300, Lucas Stankus wrote: > Remove vague comments, align temperature comment with indent block and > simplify probe return on device register. > > Also fix the following checkpatch warning: > CHECK: Alignment should match open parenthesis > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com> > --- > drivers/staging/iio/cdc/ad7746.c | 31 ++++++++++--------------------- > 1 file changed, 10 insertions(+), 21 deletions(-) > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c > index dfd71e99e872..e03d010b2f4c 100644 > --- a/drivers/staging/iio/cdc/ad7746.c > +++ b/drivers/staging/iio/cdc/ad7746.c > @@ -84,10 +84,6 @@ > #define AD7746_CAPDAC_DACEN BIT(7) > #define AD7746_CAPDAC_DACP(x) ((x) & 0x7F) > > -/* > - * struct ad7746_chip_info - chip specific information > - */ > - Comment deletions should go in a separate patch > struct ad7746_chip_info { > struct i2c_client *client; > struct mutex lock; /* protect sensor state */ > @@ -232,13 +228,14 @@ static int ad7746_select_channel(struct iio_dev *indio_dev, > > if (chip->capdac_set != chan->channel) { > ret = i2c_smbus_write_byte_data(chip->client, > - AD7746_REG_CAPDACA, > - chip->capdac[chan->channel][0]); > + AD7746_REG_CAPDACA, > + chip->capdac[chan->channel][0]); > if (ret < 0) > return ret; > + > ret = i2c_smbus_write_byte_data(chip->client, > - AD7746_REG_CAPDACB, > - chip->capdac[chan->channel][1]); > + AD7746_REG_CAPDACB, > + chip->capdac[chan->channel][1]); > if (ret < 0) > return ret; Alignments of function argument form a different logical change and should go on a separate patch... > > @@ -564,10 +561,10 @@ static int ad7746_read_raw(struct iio_dev *indio_dev, > > switch (chan->type) { > case IIO_TEMP: > - /* > - * temperature in milli degrees Celsius > - * T = ((*val / 2048) - 4096) * 1000 > - */ > + /* > + * temperature in milli degrees Celsius > + * T = ((*val / 2048) - 4096) * 1000 > + */ > *val = (*val * 125) / 256; > break; > case IIO_VOLTAGE: > @@ -669,10 +666,6 @@ static const struct iio_info ad7746_info = { > .write_raw = ad7746_write_raw, > }; > > -/* > - * device probe and remove > - */ > - > static int ad7746_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -730,11 +723,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 return value fix should go in a separate patch > > static const struct i2c_device_id ad7746_id[] = { > -- > 2.31.1 > > so, in my opinion, this patch could be split in three different patches. Thank you, fabio
On Tue, 11 May 2021 17:54:01 -0300 Lucas Stankus <lucas.p.stankus@gmail.com> wrote: > Remove vague comments, align temperature comment with indent block and > simplify probe return on device register. > > Also fix the following checkpatch warning: > CHECK: Alignment should match open parenthesis > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com> As Fabio pointed out, finer grained patches with one type of change per patch would be good. > --- > drivers/staging/iio/cdc/ad7746.c | 31 ++++++++++--------------------- > 1 file changed, 10 insertions(+), 21 deletions(-) > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c > index dfd71e99e872..e03d010b2f4c 100644 > --- a/drivers/staging/iio/cdc/ad7746.c > +++ b/drivers/staging/iio/cdc/ad7746.c > @@ -84,10 +84,6 @@ > #define AD7746_CAPDAC_DACEN BIT(7) > #define AD7746_CAPDAC_DACP(x) ((x) & 0x7F) > > -/* > - * struct ad7746_chip_info - chip specific information > - */ > - > struct ad7746_chip_info { > struct i2c_client *client; > struct mutex lock; /* protect sensor state */ > @@ -232,13 +228,14 @@ static int ad7746_select_channel(struct iio_dev *indio_dev, > > if (chip->capdac_set != chan->channel) { > ret = i2c_smbus_write_byte_data(chip->client, > - AD7746_REG_CAPDACA, > - chip->capdac[chan->channel][0]); > + AD7746_REG_CAPDACA, > + chip->capdac[chan->channel][0]); > if (ret < 0) > return ret; > + > ret = i2c_smbus_write_byte_data(chip->client, > - AD7746_REG_CAPDACB, > - chip->capdac[chan->channel][1]); > + AD7746_REG_CAPDACB, > + chip->capdac[chan->channel][1]); > if (ret < 0) > return ret; I wondered if it might be sensible to factor this code out to reduce the indent and make things more readable. Having taken a look it seems there is another place with exactly the same call sequence. From how it's used there, I'm assuming this is updating the offsets. As such, I would introduce an ad7746_offsets_set(struct iio_dev *indio_dev, int channel) or similar. > > @@ -564,10 +561,10 @@ static int ad7746_read_raw(struct iio_dev *indio_dev, > > switch (chan->type) { > case IIO_TEMP: > - /* > - * temperature in milli degrees Celsius > - * T = ((*val / 2048) - 4096) * 1000 > - */ > + /* > + * temperature in milli degrees Celsius > + * T = ((*val / 2048) - 4096) * 1000 > + */ > *val = (*val * 125) / 256; > break; > case IIO_VOLTAGE: > @@ -669,10 +666,6 @@ static const struct iio_info ad7746_info = { > .write_raw = ad7746_write_raw, > }; > > -/* > - * device probe and remove > - */ > - > static int ad7746_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -730,11 +723,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[] = {
On Thu, May 13, 2021 at 12:59 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Tue, 11 May 2021 17:54:01 -0300 > Lucas Stankus <lucas.p.stankus@gmail.com> wrote: > > > Remove vague comments, align temperature comment with indent block and > > simplify probe return on device register. > > > > Also fix the following checkpatch warning: > > CHECK: Alignment should match open parenthesis > > > > Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com> > > As Fabio pointed out, finer grained patches with one type of change per > patch would be good. Thank you both for the review and sorry for the radio silence, I'll split the patch in the v2. > > > --- > > drivers/staging/iio/cdc/ad7746.c | 31 ++++++++++--------------------- > > 1 file changed, 10 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c > > index dfd71e99e872..e03d010b2f4c 100644 > > --- a/drivers/staging/iio/cdc/ad7746.c > > +++ b/drivers/staging/iio/cdc/ad7746.c > > @@ -84,10 +84,6 @@ > > #define AD7746_CAPDAC_DACEN BIT(7) > > #define AD7746_CAPDAC_DACP(x) ((x) & 0x7F) > > > > -/* > > - * struct ad7746_chip_info - chip specific information > > - */ > > - > > struct ad7746_chip_info { > > struct i2c_client *client; > > struct mutex lock; /* protect sensor state */ > > @@ -232,13 +228,14 @@ static int ad7746_select_channel(struct iio_dev *indio_dev, > > > > if (chip->capdac_set != chan->channel) { > > ret = i2c_smbus_write_byte_data(chip->client, > > - AD7746_REG_CAPDACA, > > - chip->capdac[chan->channel][0]); > > + AD7746_REG_CAPDACA, > > + chip->capdac[chan->channel][0]); > > if (ret < 0) > > return ret; > > + ret = i2c_smbus_write_byte_data(chip->client, > > - AD7746_REG_CAPDACB, > > - chip->capdac[chan->channel][1]); > > + AD7746_REG_CAPDACB, > > + chip->capdac[chan->channel][1]); > > if (ret < 0) > > return ret; > > I wondered if it might be sensible to factor this code out to reduce the indent > and make things more readable. Having taken a look it seems there is another > place with exactly the same call sequence. From how it's used there, I'm > assuming this is updating the offsets. As such, I would introduce an > > ad7746_offsets_set(struct iio_dev *indio_dev, int channel) > > or similar. > Makes sense, I'll do that in the v2 as well. > > > > > @@ -564,10 +561,10 @@ static int ad7746_read_raw(struct iio_dev *indio_dev, > > > > switch (chan->type) { > > case IIO_TEMP: > > - /* > > - * temperature in milli degrees Celsius > > - * T = ((*val / 2048) - 4096) * 1000 > > - */ > > + /* > > + * temperature in milli degrees Celsius > > + * T = ((*val / 2048) - 4096) * 1000 > > + */ > > *val = (*val * 125) / 256; > > break; > > case IIO_VOLTAGE: > > @@ -669,10 +666,6 @@ static const struct iio_info ad7746_info = { > > .write_raw = ad7746_write_raw, > > }; > > > > -/* > > - * device probe and remove > > - */ > > - > > static int ad7746_probe(struct i2c_client *client, > > const struct i2c_device_id *id) > > { > > @@ -730,11 +723,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[] = { >
diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c index dfd71e99e872..e03d010b2f4c 100644 --- a/drivers/staging/iio/cdc/ad7746.c +++ b/drivers/staging/iio/cdc/ad7746.c @@ -84,10 +84,6 @@ #define AD7746_CAPDAC_DACEN BIT(7) #define AD7746_CAPDAC_DACP(x) ((x) & 0x7F) -/* - * struct ad7746_chip_info - chip specific information - */ - struct ad7746_chip_info { struct i2c_client *client; struct mutex lock; /* protect sensor state */ @@ -232,13 +228,14 @@ static int ad7746_select_channel(struct iio_dev *indio_dev, if (chip->capdac_set != chan->channel) { ret = i2c_smbus_write_byte_data(chip->client, - AD7746_REG_CAPDACA, - chip->capdac[chan->channel][0]); + AD7746_REG_CAPDACA, + chip->capdac[chan->channel][0]); if (ret < 0) return ret; + ret = i2c_smbus_write_byte_data(chip->client, - AD7746_REG_CAPDACB, - chip->capdac[chan->channel][1]); + AD7746_REG_CAPDACB, + chip->capdac[chan->channel][1]); if (ret < 0) return ret; @@ -564,10 +561,10 @@ static int ad7746_read_raw(struct iio_dev *indio_dev, switch (chan->type) { case IIO_TEMP: - /* - * temperature in milli degrees Celsius - * T = ((*val / 2048) - 4096) * 1000 - */ + /* + * temperature in milli degrees Celsius + * T = ((*val / 2048) - 4096) * 1000 + */ *val = (*val * 125) / 256; break; case IIO_VOLTAGE: @@ -669,10 +666,6 @@ static const struct iio_info ad7746_info = { .write_raw = ad7746_write_raw, }; -/* - * device probe and remove - */ - static int ad7746_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -730,11 +723,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[] = {
Remove vague comments, align temperature comment with indent block and simplify probe return on device register. Also fix the following checkpatch warning: CHECK: Alignment should match open parenthesis Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com> --- drivers/staging/iio/cdc/ad7746.c | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-)