[PATCH v2 0/3] staging: iio: adc: General code reformatting / cleanup patchset

[PATCH v2 0/3] staging: iio: adc: General code reformatting / cleanup patchset

[Deepak R Varma]

Address code formatting warnings and check messages flagged by
checkpatch script. Also include another improvement for correcting macro
name typos. Changes intended to improve readability of code.

Changes since v1:
	1. Add separate patch for code re-indentation comment
	2. Add separate patch for correcting macro names as suggested by
	   Lars-Peter.
	3. Update variable name from sync* to sinc* as suggested by
	   Stefano. 
	4. Revert patch intended to add space around "-" operator. It
	   was a string with a "-" sign further formatted using
	   stringification. That patch is out of the patchset now.


Deepak R Varma (3):
  staging: iio: adc: ad7192: Re-indent enum labels
  staging: iio: adc: ad7192: Correct macro names from SYNC to SINC
  staging: iio: adc: ad7192: Reformat lines crossing 80 columns

 drivers/staging/iio/adc/ad7192.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

-- 
2.17.1

[PATCH 1/3] staging: iio: adc: ad7192: Re-indent enum labels

[Deepak R Varma]

Re-indent enum labels as per coding style guidelines. Problem
detected by checkpatch script.

Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
---
 drivers/staging/iio/adc/ad7192.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index bf3e2a9cc07f..51b1cd3ad1de 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -156,8 +156,8 @@
  */
 
 enum {
-   AD7192_SYSCALIB_ZERO_SCALE,
-   AD7192_SYSCALIB_FULL_SCALE,
+	AD7192_SYSCALIB_ZERO_SCALE,
+	AD7192_SYSCALIB_FULL_SCALE,
 };
 
 struct ad7192_state {
-- 
2.17.1

Re: [Outreachy kernel] [PATCH 1/3] staging: iio: adc: ad7192: Re-indent enum labels

[Stefano Brivio]

On Thu, 19 Mar 2020 00:38:24 +0530
Deepak R Varma <mh12gx2825@gmail.com> wrote:

> Re-indent enum labels as per coding style guidelines. Problem
> detected by checkpatch script.
> 
> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>

This, and all the patches in this series, also need a 'v2' in the
subject.

-- 
Stefano

[PATCH 2/3] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC

[Deepak R Varma]

Three macros include SYNC in their names which is a typo. Update those
names to SINC. Change suggested by Lars-Peter Clausen.

Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
---
 drivers/staging/iio/adc/ad7192.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index 51b1cd3ad1de..a5b6cc1fc375 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -143,9 +143,9 @@
 #define AD7192_EXT_FREQ_MHZ_MAX	5120000
 #define AD7192_INT_FREQ_MHZ	4915200
 
-#define AD7192_NO_SYNC_FILTER	1
-#define AD7192_SYNC3_FILTER	3
-#define AD7192_SYNC4_FILTER	4
+#define AD7192_NO_SINC_FILTER	1
+#define AD7192_SINC3_FILTER	3
+#define AD7192_SINC4_FILTER	4
 
 /* NOTE:
  * The AD7190/2/5 features a dual use data out ready DOUT/RDY output.
@@ -366,7 +366,7 @@ static int ad7192_setup(struct ad7192_state *st, struct device_node *np)
 		st->conf |= AD7192_CONF_REFSEL;
 
 	st->conf &= ~AD7192_CONF_CHOP;
-	st->f_order = AD7192_NO_SYNC_FILTER;
+	st->f_order = AD7192_NO_SINC_FILTER;
 
 	buf_en = of_property_read_bool(np, "adi,buffer-enable");
 	if (buf_en)
@@ -483,11 +483,11 @@ static void ad7192_get_available_filter_freq(struct ad7192_state *st,
 
 	/* Formulas for filter at page 25 of the datasheet */
 	fadc = DIV_ROUND_CLOSEST(st->fclk,
-				 AD7192_SYNC4_FILTER * AD7192_MODE_RATE(st->mode));
+				 AD7192_SINC4_FILTER * AD7192_MODE_RATE(st->mode));
 	freq[0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
 
 	fadc = DIV_ROUND_CLOSEST(st->fclk,
-				 AD7192_SYNC3_FILTER * AD7192_MODE_RATE(st->mode));
+				 AD7192_SINC3_FILTER * AD7192_MODE_RATE(st->mode));
 	freq[1] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
 
 	fadc = DIV_ROUND_CLOSEST(st->fclk, AD7192_MODE_RATE(st->mode));
@@ -575,25 +575,25 @@ static int ad7192_set_3db_filter_freq(struct ad7192_state *st,
 
 	switch (idx) {
 	case 0:
-		st->f_order = AD7192_SYNC4_FILTER;
+		st->f_order = AD7192_SINC4_FILTER;
 		st->mode &= ~AD7192_MODE_SINC3;
 
 		st->conf |= AD7192_CONF_CHOP;
 		break;
 	case 1:
-		st->f_order = AD7192_SYNC3_FILTER;
+		st->f_order = AD7192_SINC3_FILTER;
 		st->mode |= AD7192_MODE_SINC3;
 
 		st->conf |= AD7192_CONF_CHOP;
 		break;
 	case 2:
-		st->f_order = AD7192_NO_SYNC_FILTER;
+		st->f_order = AD7192_NO_SINC_FILTER;
 		st->mode &= ~AD7192_MODE_SINC3;
 
 		st->conf &= ~AD7192_CONF_CHOP;
 		break;
 	case 3:
-		st->f_order = AD7192_NO_SYNC_FILTER;
+		st->f_order = AD7192_NO_SINC_FILTER;
 		st->mode |= AD7192_MODE_SINC3;
 
 		st->conf &= ~AD7192_CONF_CHOP;
-- 
2.17.1

Re: [Outreachy kernel] [PATCH 2/3] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC

[Stefano Brivio]

On Thu, 19 Mar 2020 00:39:13 +0530
Deepak R Varma <mh12gx2825@gmail.com> wrote:

> Three macros include SYNC in their names which is a typo. Update those
> names to SINC. Change suggested by Lars-Peter Clausen.

There are tags for that, in particular:

Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
Fixes: 77f6a23092c0 ("staging: iio: adc: ad7192: Add low_pass_3db_filter_frequency")

Please keep an eye on messages from/to others on the outreachy-kernel
list, these topics are being discussed quite frequently.

> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> ---
>
> [...]

-- 
Stefano

[PATCH 3/3] staging: iio: adc: ad7192: Reformat lines crossing 80 columns

[Deepak R Varma]

Macro arguments are computed at the time of calling the macro. This
makes the lines cross 80 column width. Add variables to perform the
calculations before hand and use these variable in the macro calls instead.

Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
---
 drivers/staging/iio/adc/ad7192.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index a5b6cc1fc375..2a9c68aa8260 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -480,14 +480,15 @@ static void ad7192_get_available_filter_freq(struct ad7192_state *st,
 						    int *freq)
 {
 	unsigned int fadc;
+	unsigned int sinc3_filter, sinc4_filter;
 
 	/* Formulas for filter at page 25 of the datasheet */
-	fadc = DIV_ROUND_CLOSEST(st->fclk,
-				 AD7192_SINC4_FILTER * AD7192_MODE_RATE(st->mode));
+	sinc4_filter = AD7192_SINC4_FILTER * AD7192_MODE_RATE(st->mode);
+	fadc = DIV_ROUND_CLOSEST(st->fclk, sinc4_filter);
 	freq[0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
 
-	fadc = DIV_ROUND_CLOSEST(st->fclk,
-				 AD7192_SINC3_FILTER * AD7192_MODE_RATE(st->mode));
+	sinc3_filter = AD7192_SINC3_FILTER * AD7192_MODE_RATE(st->mode);
+	fadc = DIV_ROUND_CLOSEST(st->fclk, sinc3_filter);
 	freq[1] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
 
 	fadc = DIV_ROUND_CLOSEST(st->fclk, AD7192_MODE_RATE(st->mode));
-- 
2.17.1

Re: [Outreachy kernel] [PATCH 3/3] staging: iio: adc: ad7192: Reformat lines crossing 80 columns

[Stefano Brivio]

On Thu, 19 Mar 2020 00:40:02 +0530
Deepak R Varma <mh12gx2825@gmail.com> wrote:

> Macro arguments are computed at the time of calling the macro. This
> makes the lines cross 80 column width. Add variables to perform the
> calculations before hand and use these variable in the macro calls instead.
> 
> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7192.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
> index a5b6cc1fc375..2a9c68aa8260 100644
> --- a/drivers/staging/iio/adc/ad7192.c
> +++ b/drivers/staging/iio/adc/ad7192.c
> @@ -480,14 +480,15 @@ static void ad7192_get_available_filter_freq(struct ad7192_state *st,
>  						    int *freq)
>  {
>  	unsigned int fadc;
> +	unsigned int sinc3_filter, sinc4_filter;
>  
>  	/* Formulas for filter at page 25 of the datasheet */
> -	fadc = DIV_ROUND_CLOSEST(st->fclk,
> -				 AD7192_SINC4_FILTER * AD7192_MODE_RATE(st->mode));
> +	sinc4_filter = AD7192_SINC4_FILTER * AD7192_MODE_RATE(st->mode);
> +	fadc = DIV_ROUND_CLOSEST(st->fclk, sinc4_filter);

The name sinc4_filter is at least not derived from another typo anymore,
but it still doesn't make sense to me. Is it a filtering... factor,
as the name would suggest? No. It comes from a "mode" register and it
becomes a divisor of the output data rate.

Besides, you don't need separate variables (why two, even?). As I
already mentioned to you offline, DIV_ROUND_CLOSEST() is used. If '/'
is the DIV_ROUND_CLOSEST() operation, then, I guess:

	a = b / (x * y) <=> a = b / x / y

-- 
Stefano

Re: [PATCH v2 0/3] staging: iio: adc: General code reformatting / cleanup patchset

[Lars-Peter Clausen]

On 3/18/20 8:07 PM, Deepak R Varma wrote:
> Address code formatting warnings and check messages flagged by
> checkpatch script. Also include another improvement for correcting macro
> name typos. Changes intended to improve readability of code.
> 
> Changes since v1:
> 	1. Add separate patch for code re-indentation comment
> 	2. Add separate patch for correcting macro names as suggested by
> 	   Lars-Peter.
> 	3. Update variable name from sync* to sinc* as suggested by
> 	   Stefano.
> 	4. Revert patch intended to add space around "-" operator. It
> 	   was a string with a "-" sign further formatted using
> 	   stringification. That patch is out of the patchset now.
> 
> 
> Deepak R Varma (3):
>    staging: iio: adc: ad7192: Re-indent enum labels
>    staging: iio: adc: ad7192: Correct macro names from SYNC to SINC
>    staging: iio: adc: ad7192: Reformat lines crossing 80 columns

Hi,

The patches all look very good, thanks.

Just one thing, make sure that you base your patches on Jonathan's 
latest development tree. The ad7192 drivers was just moved out of 
staging 2 weeks ago, so the file path in your patches is no longer 
correct. But the patches themselves look good and should be applied 
regardless of whether the driver is in staging or not.

See 
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=togreg&id=b581f748cce00f4cbd6c76c4f301840403dece7b 
for the patch that moved it out of staging.

Jonathan's development tree is the togreg branch from his repository at 
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/.

You can add it as an additional remote to your git checkout using

git remote add iio 
git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
git fetch iio

And then you can reference the dev branch as iio/togreg to for example 
rebase your local branch ontop of it.

- Lars