linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging
@ 2021-05-23 17:11 Lucas Stankus
  2021-05-23 17:11 ` [PATCH v2 1/3] staging: iio: cdc: ad7746: remove ordinary comments Lucas Stankus
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Lucas Stankus @ 2021-05-23 17:11 UTC (permalink / raw)
  To: lars, Michael.Hennerich, jic23, gregkh
  Cc: linux-iio, linux-staging, linux-kernel

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/3] staging: iio: cdc: ad7746: remove ordinary comments
  2021-05-23 17:11 [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging Lucas Stankus
@ 2021-05-23 17:11 ` Lucas Stankus
  2021-05-23 17:12 ` [PATCH v2 2/3] staging: iio: cdc: ad7746: clean up probe return Lucas Stankus
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Lucas Stankus @ 2021-05-23 17:11 UTC (permalink / raw)
  To: lars, Michael.Hennerich, jic23, gregkh
  Cc: linux-iio, linux-staging, linux-kernel

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/3] staging: iio: cdc: ad7746: clean up probe return
  2021-05-23 17:11 [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging Lucas Stankus
  2021-05-23 17:11 ` [PATCH v2 1/3] staging: iio: cdc: ad7746: remove ordinary comments Lucas Stankus
@ 2021-05-23 17:12 ` Lucas Stankus
  2021-05-24  7:53   ` Alexandru Ardelean
  2021-05-23 17:12 ` [PATCH v2 3/3] staging: iio: cdc: ad7746: extract capac setup to own function Lucas Stankus
  2021-05-26 17:07 ` [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging Jonathan Cameron
  3 siblings, 1 reply; 8+ messages in thread
From: Lucas Stankus @ 2021-05-23 17:12 UTC (permalink / raw)
  To: lars, Michael.Hennerich, jic23, gregkh
  Cc: linux-iio, linux-staging, linux-kernel

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 3/3] staging: iio: cdc: ad7746: extract capac setup to own function
  2021-05-23 17:11 [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging Lucas Stankus
  2021-05-23 17:11 ` [PATCH v2 1/3] staging: iio: cdc: ad7746: remove ordinary comments Lucas Stankus
  2021-05-23 17:12 ` [PATCH v2 2/3] staging: iio: cdc: ad7746: clean up probe return Lucas Stankus
@ 2021-05-23 17:12 ` Lucas Stankus
  2021-05-24  8:09   ` Alexandru Ardelean
  2021-05-26 17:07 ` [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging Jonathan Cameron
  3 siblings, 1 reply; 8+ messages in thread
From: Lucas Stankus @ 2021-05-23 17:12 UTC (permalink / raw)
  To: lars, Michael.Hennerich, jic23, gregkh
  Cc: linux-iio, linux-staging, linux-kernel

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/3] staging: iio: cdc: ad7746: clean up probe return
  2021-05-23 17:12 ` [PATCH v2 2/3] staging: iio: cdc: ad7746: clean up probe return Lucas Stankus
@ 2021-05-24  7:53   ` Alexandru Ardelean
  0 siblings, 0 replies; 8+ messages in thread
From: Alexandru Ardelean @ 2021-05-24  7:53 UTC (permalink / raw)
  To: Lucas Stankus
  Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron,
	Greg Kroah-Hartman, linux-iio, linux-staging, LKML

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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 3/3] staging: iio: cdc: ad7746: extract capac setup to own function
  2021-05-23 17:12 ` [PATCH v2 3/3] staging: iio: cdc: ad7746: extract capac setup to own function Lucas Stankus
@ 2021-05-24  8:09   ` Alexandru Ardelean
  0 siblings, 0 replies; 8+ messages in thread
From: Alexandru Ardelean @ 2021-05-24  8:09 UTC (permalink / raw)
  To: Lucas Stankus
  Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron,
	Greg Kroah-Hartman, linux-iio, linux-staging, LKML

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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging
  2021-05-23 17:11 [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging Lucas Stankus
                   ` (2 preceding siblings ...)
  2021-05-23 17:12 ` [PATCH v2 3/3] staging: iio: cdc: ad7746: extract capac setup to own function Lucas Stankus
@ 2021-05-26 17:07 ` Jonathan Cameron
  2021-05-31  1:48   ` Lucas Stankus
  3 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2021-05-26 17:07 UTC (permalink / raw)
  To: Lucas Stankus
  Cc: lars, Michael.Hennerich, gregkh, linux-iio, linux-staging, linux-kernel

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(-)
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging
  2021-05-26 17:07 ` [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging Jonathan Cameron
@ 2021-05-31  1:48   ` Lucas Stankus
  0 siblings, 0 replies; 8+ messages in thread
From: Lucas Stankus @ 2021-05-31  1:48 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Lars-Peter Clausen, Hennerich, Michael, Greg Kroah-Hartman,
	linux-iio, linux-staging, Linux Kernel Mailing List

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(-)
> >
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-05-31  1:48 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-23 17:11 [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging Lucas Stankus
2021-05-23 17:11 ` [PATCH v2 1/3] staging: iio: cdc: ad7746: remove ordinary comments Lucas Stankus
2021-05-23 17:12 ` [PATCH v2 2/3] staging: iio: cdc: ad7746: clean up probe return Lucas Stankus
2021-05-24  7:53   ` Alexandru Ardelean
2021-05-23 17:12 ` [PATCH v2 3/3] staging: iio: cdc: ad7746: extract capac setup to own function Lucas Stankus
2021-05-24  8:09   ` Alexandru Ardelean
2021-05-26 17:07 ` [PATCH v2 0/3] staging: iio: cdc: ad7746: initial effort to move out of staging Jonathan Cameron
2021-05-31  1:48   ` Lucas Stankus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).