Tidy up driver code by removing vague comments, simplifying probe
return, and extracting capdac register write to a separate function.
These small patches are a starting point for improving the ad7746 driver,
hopefully to a point where it's possible to get it out of staging. I'm
looking up to feedback on what could be improved to accomplish that.
changelog v1 -> v2:
- Dropped num_channels fixup patch (applied from previous series).
- Split general code style patch into several atomic ones.
- New patch to catch capdac write boilerplate into a single function.
Lucas Stankus (3):
staging: iio: cdc: ad7746: remove ordinary comments
staging: iio: cdc: ad7746: clean up probe return
staging: iio: cdc: ad7746: extract capac setup to own function
drivers/staging/iio/cdc/ad7746.c | 58 +++++++++++++-------------------
1 file changed, 23 insertions(+), 35 deletions(-)
--
2.31.1
Remove ordinary comments about typical driver structure.
Also align one comment with wrong indentation.
Signed-off-by: Lucas Stankus <[email protected]>
---
drivers/staging/iio/cdc/ad7746.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index c42fffa8b60e..12b2554a85b5 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -82,10 +82,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 */
@@ -562,10 +558,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:
@@ -667,10 +663,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)
{
--
2.31.1
Refactor the capac register write logic to own function.
Also fixes the following checkpatch warning:
CHECK: Alignment should match open parenthesis
Signed-off-by: Lucas Stankus <[email protected]>
---
drivers/staging/iio/cdc/ad7746.c | 36 ++++++++++++++++----------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 367a5990ae35..4221312f0a32 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -209,6 +209,19 @@ static const unsigned char ad7746_cap_filter_rate_table[][2] = {
{16, 62 + 1}, {13, 77 + 1}, {11, 92 + 1}, {9, 110 + 1},
};
+static int ad7746_set_capdac(struct ad7746_chip_info *chip, int channel)
+{
+ int ret = i2c_smbus_write_byte_data(chip->client,
+ AD7746_REG_CAPDACA,
+ chip->capdac[channel][0]);
+ if (ret < 0)
+ return ret;
+
+ return i2c_smbus_write_byte_data(chip->client,
+ AD7746_REG_CAPDACB,
+ chip->capdac[channel][1]);
+}
+
static int ad7746_select_channel(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan)
{
@@ -224,17 +237,11 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
AD7746_CONF_CAPFS_SHIFT;
delay = ad7746_cap_filter_rate_table[idx][1];
+ ret = ad7746_set_capdac(chip, chan->channel);
+ if (ret < 0)
+ return ret;
+
if (chip->capdac_set != chan->channel) {
- ret = i2c_smbus_write_byte_data(chip->client,
- 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]);
- if (ret < 0)
- return ret;
chip->capdac_set = chan->channel;
}
@@ -478,14 +485,7 @@ static int ad7746_write_raw(struct iio_dev *indio_dev,
chip->capdac[chan->channel][chan->differential] = val > 0 ?
AD7746_CAPDAC_DACP(val) | AD7746_CAPDAC_DACEN : 0;
- ret = i2c_smbus_write_byte_data(chip->client,
- AD7746_REG_CAPDACA,
- chip->capdac[chan->channel][0]);
- if (ret < 0)
- goto out;
- ret = i2c_smbus_write_byte_data(chip->client,
- AD7746_REG_CAPDACB,
- chip->capdac[chan->channel][1]);
+ ret = ad7746_set_capdac(chip, chan->channel);
if (ret < 0)
goto out;
--
2.31.1
Slight simplication of the probe return on device register.
Signed-off-by: Lucas Stankus <[email protected]>
---
drivers/staging/iio/cdc/ad7746.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 12b2554a85b5..367a5990ae35 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -734,11 +734,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.31.1
On Sun, May 23, 2021 at 8:12 PM Lucas Stankus <[email protected]> wrote:
>
> Slight simplication of the probe return on device register.
>
Reviewed-by: Alexandru Ardelean <[email protected]>
> Signed-off-by: Lucas Stankus <[email protected]>
> ---
> drivers/staging/iio/cdc/ad7746.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index 12b2554a85b5..367a5990ae35 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -734,11 +734,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.31.1
>
On Sun, May 23, 2021 at 8:13 PM Lucas Stankus <[email protected]> wrote:
>
> Refactor the capac register write logic to own function.
Minor typo 'capac' -> 'capdac'
Other than that:
Reviewed-by: Alexandru Ardelean <[email protected]>
>
> Also fixes the following checkpatch warning:
> CHECK: Alignment should match open parenthesis
>
> Signed-off-by: Lucas Stankus <[email protected]>
> ---
> drivers/staging/iio/cdc/ad7746.c | 36 ++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index 367a5990ae35..4221312f0a32 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -209,6 +209,19 @@ static const unsigned char ad7746_cap_filter_rate_table[][2] = {
> {16, 62 + 1}, {13, 77 + 1}, {11, 92 + 1}, {9, 110 + 1},
> };
>
> +static int ad7746_set_capdac(struct ad7746_chip_info *chip, int channel)
> +{
> + int ret = i2c_smbus_write_byte_data(chip->client,
> + AD7746_REG_CAPDACA,
> + chip->capdac[channel][0]);
> + if (ret < 0)
> + return ret;
> +
> + return i2c_smbus_write_byte_data(chip->client,
> + AD7746_REG_CAPDACB,
> + chip->capdac[channel][1]);
> +}
> +
> static int ad7746_select_channel(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan)
> {
> @@ -224,17 +237,11 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
> AD7746_CONF_CAPFS_SHIFT;
> delay = ad7746_cap_filter_rate_table[idx][1];
>
> + ret = ad7746_set_capdac(chip, chan->channel);
> + if (ret < 0)
> + return ret;
> +
> if (chip->capdac_set != chan->channel) {
> - ret = i2c_smbus_write_byte_data(chip->client,
> - 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]);
> - if (ret < 0)
> - return ret;
>
> chip->capdac_set = chan->channel;
> }
> @@ -478,14 +485,7 @@ static int ad7746_write_raw(struct iio_dev *indio_dev,
> chip->capdac[chan->channel][chan->differential] = val > 0 ?
> AD7746_CAPDAC_DACP(val) | AD7746_CAPDAC_DACEN : 0;
>
> - ret = i2c_smbus_write_byte_data(chip->client,
> - AD7746_REG_CAPDACA,
> - chip->capdac[chan->channel][0]);
> - if (ret < 0)
> - goto out;
> - ret = i2c_smbus_write_byte_data(chip->client,
> - AD7746_REG_CAPDACB,
> - chip->capdac[chan->channel][1]);
> + ret = ad7746_set_capdac(chip, chan->channel);
> if (ret < 0)
> goto out;
>
> --
> 2.31.1
>
On Sun, 23 May 2021 14:11:35 -0300
Lucas Stankus <[email protected]> wrote:
> Tidy up driver code by removing vague comments, simplifying probe
> return, and extracting capdac register write to a separate function.
>
> These small patches are a starting point for improving the ad7746 driver,
> hopefully to a point where it's possible to get it out of staging. I'm
> looking up to feedback on what could be improved to accomplish that.
Usually the easiest way to get such feedback is to propose moving it out of
staging, (with move detection turned off in git format-patch).
Then we'll review it in a similar fashion to a new driver.
Starting point though for any review is ABI. Looks like there is some
custom stuff in here which either needs to go away or be properly
proposed and documented.
This series applied to the togreg branch of iio.git - initially
pushed out as testing to let 0-day poke at it.
Thanks,
Jonathan
>
> changelog v1 -> v2:
> - Dropped num_channels fixup patch (applied from previous series).
> - Split general code style patch into several atomic ones.
> - New patch to catch capdac write boilerplate into a single function.
>
> Lucas Stankus (3):
> staging: iio: cdc: ad7746: remove ordinary comments
> staging: iio: cdc: ad7746: clean up probe return
> staging: iio: cdc: ad7746: extract capac setup to own function
>
> drivers/staging/iio/cdc/ad7746.c | 58 +++++++++++++-------------------
> 1 file changed, 23 insertions(+), 35 deletions(-)
>
On Wed, May 26, 2021 at 2:05 PM Jonathan Cameron <[email protected]> wrote:
>
> On Sun, 23 May 2021 14:11:35 -0300
> Lucas Stankus <[email protected]> wrote:
>
> > Tidy up driver code by removing vague comments, simplifying probe
> > return, and extracting capdac register write to a separate function.
> >
> > These small patches are a starting point for improving the ad7746 driver,
> > hopefully to a point where it's possible to get it out of staging. I'm
> > looking up to feedback on what could be improved to accomplish that.
> Usually the easiest way to get such feedback is to propose moving it out of
> staging, (with move detection turned off in git format-patch).
> Then we'll review it in a similar fashion to a new driver.
Oh okay, sorry for the unconventional patch set then and thanks for
giving me the heads up about the format-patch flag, I'd probably get
that wrong if you didn't =/
>
> Starting point though for any review is ABI. Looks like there is some
> custom stuff in here which either needs to go away or be properly
> proposed and documented.
Nice, I'll look into that.
>
> This series applied to the togreg branch of iio.git - initially
> pushed out as testing to let 0-day poke at it.
>
> Thanks,
>
> Jonathan
>
> >
> > changelog v1 -> v2:
> > - Dropped num_channels fixup patch (applied from previous series).
> > - Split general code style patch into several atomic ones.
> > - New patch to catch capdac write boilerplate into a single function.
> >
> > Lucas Stankus (3):
> > staging: iio: cdc: ad7746: remove ordinary comments
> > staging: iio: cdc: ad7746: clean up probe return
> > staging: iio: cdc: ad7746: extract capac setup to own function
> >
> > drivers/staging/iio/cdc/ad7746.c | 58 +++++++++++++-------------------
> > 1 file changed, 23 insertions(+), 35 deletions(-)
> >
>