[PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging

[PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging

[Lucas Stankus]

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

[PATCH v2 1/3] staging: iio: cdc: ad7746: remove ordinary comments

[Lucas Stankus]

Remove ordinary comments about typical driver structure.
Also align one comment with wrong indentation.

Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
---
 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

[PATCH v2 2/3] staging: iio: cdc: ad7746: clean up probe return

[Lucas Stankus]

Slight simplication of the probe return on device register.

Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
---
 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

Re: [PATCH v2 2/3] staging: iio: cdc: ad7746: clean up probe return

[Alexandru Ardelean]

On Sun, May 23, 2021 at 8:12 PM Lucas Stankus <lucas.p.stankus@gmail.com> wrote:
>
> Slight simplication of the probe return on device register.
>

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

> Signed-off-by: Lucas Stankus <lucas.p.stankus@gmail.com>
> ---
>  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
>

[PATCH v2 3/3] staging: iio: cdc: ad7746: extract capac setup to own function

[Lucas Stankus]

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 <lucas.p.stankus@gmail.com>
---
 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

Re: [PATCH v2 3/3] staging: iio: cdc: ad7746: extract capac setup to own function

[Alexandru Ardelean]

On Sun, May 23, 2021 at 8:13 PM Lucas Stankus <lucas.p.stankus@gmail.com> wrote:
>
> Refactor the capac register write logic to own function.

Minor typo 'capac' -> 'capdac'

Other than that:

Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com>

>
> Also fixes 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 | 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
>

Re: [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging

[Jonathan Cameron]

On Sun, 23 May 2021 14:11:35 -0300
Lucas Stankus <lucas.p.stankus@gmail.com> 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(-)
> 

Re: [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging

[Lucas Stankus]

On Wed, May 26, 2021 at 2:05 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 23 May 2021 14:11:35 -0300
> Lucas Stankus <lucas.p.stankus@gmail.com> 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(-)
> >
>