[PATCH v3 0/4] staging: iio: adc: General code reformatting / cleanup patchset

[PATCH v3 0/4] staging: iio: adc: General code reformatting / cleanup patchset

[Deepak R Varma]

Address code formatting warnings and check messages flagged by
checkpatch script. Includes improvement for correcting macro
name typos and simplified function implementation. Changes 
intended to improve readability of code.

Changes since v2:
	1. Add new patch to the series for ad7280a driver to add
	   comments near code that flags a misleading checkpatch
	   warning.
	2. Add patch version number v3 in individual patch subject 
	   lines.
	3. Simplified current implementation of function get_filter_freq
	   per advice from Stefano.

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 (4):
  staging: iio: adc: ad7192: Re-indent enum labels
  staging: iio: adc: ad7192: Correct macro names from SYNC to SINC
  staging: iio: adc: ad7192: get_filter_freq code optimization
  staging: iio: adc: ad7280a: Add comments to clarify stringified
    arguments

 drivers/iio/adc/ad7192.c          | 37 +++++++++++++------------------
 drivers/staging/iio/adc/ad7280a.c |  4 ++++
 2 files changed, 20 insertions(+), 21 deletions(-)

-- 
2.17.1

[PATCH v3 1/4] 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>
---

Changes since v2:
	- None. Version number increment to follow patch series version.

Changes since v1:
	1. Separated other change into a separate patch as suggested by
	   Greg KH.


 drivers/iio/adc/ad7192.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 8ec28aa8fa8a..02981f3d1794 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -157,8 +157,8 @@
  */
 
 enum {
-   AD7192_SYSCALIB_ZERO_SCALE,
-   AD7192_SYSCALIB_FULL_SCALE,
+	AD7192_SYSCALIB_ZERO_SCALE,
+	AD7192_SYSCALIB_FULL_SCALE,
 };
 
 struct ad7192_state {
-- 
2.17.1

[PATCH v3 2/4] 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.
Fixes: 77f6a23092c0 ("staging: iio: adc: ad7192: Add low_pass_3db_filter_frequency")

Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
Suggested-by: Lars-Peter Clausen <lars@metafoo.de>

---

Changes since v2:
	- None. Version increment to follow patch series versioning.

Changes since v1:
	- None. Patch added in v2 version as suggested by Stefano.


 drivers/iio/adc/ad7192.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 02981f3d1794..d9a220d4217f 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -144,9 +144,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.
@@ -367,7 +367,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)
@@ -484,11 +484,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));
@@ -576,25 +576,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

[PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[Deepak R Varma]

Current implementation of the function ad7192_get_available_filter_freq
repeats calculation of output data rate a few times. We can simplify
these steps by refactoring out the calculation of fADC. This would also
addresses the checkpatch warning of line exceeding 80 character.

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

Changes since v2:
	1. Improved function implementation. No need to use a ew
	   variable. Changes suggested by Stefano.

Changes since v1:
	1. Corrected variable names to follow datasheet terminology.


 drivers/iio/adc/ad7192.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index d9a220d4217f..5dafcf8754dd 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -482,18 +482,13 @@ static void ad7192_get_available_filter_freq(struct ad7192_state *st,
 {
 	unsigned int fadc;
 
-	/* Formulas for filter at page 25 of the datasheet */
-	fadc = DIV_ROUND_CLOSEST(st->fclk,
-				 AD7192_SINC4_FILTER * AD7192_MODE_RATE(st->mode));
-	freq[0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
+	fadc = DIV_ROUND_CLOSEST(st->fclk, AD7192_MODE_RATE(st->mode) * 1024);
 
-	fadc = DIV_ROUND_CLOSEST(st->fclk,
-				 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));
-	freq[2] = DIV_ROUND_CLOSEST(fadc * 230, 1024);
-	freq[3] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
+	/* Formulas for filter at page 25 of the datasheet */
+	freq[0] = DIV_ROUND_CLOSEST(fadc * 240, AD7192_SINC4_FILTER);
+	freq[1] = DIV_ROUND_CLOSEST(fadc * 240, AD7192_SINC3_FILTER);
+	freq[2] = fadc * 230;
+	freq[3] = fadc * 272;
 }
 
 static ssize_t ad7192_show_filter_avail(struct device *dev,
-- 
2.17.1

[PATCH v3 4/4] staging: iio: adc: ad7280a: Add comments to clarify stringified arguments

[Deepak R Varma]

Checkpatch would flash a check message around a stringified macro
argument containing a '-' character. Add comment to indicate the
argument is legitimate and doesn't need fixing.

Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
Suggested-by: Lars-Peter Clausen <lars@metafoo.de>

---

Changes since v2:
	- None. Patch added in v3

Changes since v1:
	- None. Patch added in v3

 drivers/staging/iio/adc/ad7280a.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c
index 19a5f244dcae..bef6bd1295ea 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -824,6 +824,10 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+/* Note: No need to fix checkpatch warning that reads:
+ *	CHECK: spaces preferred around that '-' (ctx:VxV)
+ * The function argument is stringified and doesn't need a fix
+ */
 static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
 			     in_voltage-voltage_thresh_low_value,
 			     0644,
-- 
2.17.1

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

[Andy Shevchenko]

On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com> wrote:
>
> Three macros include SYNC in their names which is a typo. Update those
> names to SINC.

It is good to elaborate the source of the above statement (e.g.
datasheet pages).

> Fixes: 77f6a23092c0 ("staging: iio: adc: ad7192: Add low_pass_3db_filter_frequency")

>

This blank line should go before Fixes tag. The rule is that tags are
forming a block at the end of commit message.

> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>

The code below looks good to me.


>
> ---
>
> Changes since v2:
>         - None. Version increment to follow patch series versioning.
>
> Changes since v1:
>         - None. Patch added in v2 version as suggested by Stefano.
>
>
>  drivers/iio/adc/ad7192.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 02981f3d1794..d9a220d4217f 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -144,9 +144,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.
> @@ -367,7 +367,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)
> @@ -484,11 +484,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));
> @@ -576,25 +576,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
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[Andy Shevchenko]

On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com> wrote:
>
> Current implementation of the function ad7192_get_available_filter_freq
> repeats calculation of output data rate a few times. We can simplify
> these steps by refactoring out the calculation of fADC. This would also
> addresses the checkpatch warning of line exceeding 80 character.

I'm not sure you did an equivalent changes. I believe in the original
code precision is better. Consider low clock frequencies when 10 bit
right shift may hide some bits of the division.

Care to write a python script to check the precision between old and new code?

...

> -       /* Formulas for filter at page 25 of the datasheet */
> -       fadc = DIV_ROUND_CLOSEST(st->fclk,
> -                                AD7192_SINC4_FILTER * AD7192_MODE_RATE(st->mode));
> -       freq[0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);
> +       fadc = DIV_ROUND_CLOSEST(st->fclk, AD7192_MODE_RATE(st->mode) * 1024);
>
> -       fadc = DIV_ROUND_CLOSEST(st->fclk,
> -                                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));
> -       freq[2] = DIV_ROUND_CLOSEST(fadc * 230, 1024);
> -       freq[3] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
> +       /* Formulas for filter at page 25 of the datasheet */
> +       freq[0] = DIV_ROUND_CLOSEST(fadc * 240, AD7192_SINC4_FILTER);
> +       freq[1] = DIV_ROUND_CLOSEST(fadc * 240, AD7192_SINC3_FILTER);
> +       freq[2] = fadc * 230;
> +       freq[3] = fadc * 272;


-- 
With Best Regards,
Andy Shevchenko

Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[Stefano Brivio]

Andy,

On Mon, 23 Mar 2020 01:44:20 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com> wrote:
> >
> > Current implementation of the function ad7192_get_available_filter_freq
> > repeats calculation of output data rate a few times. We can simplify
> > these steps by refactoring out the calculation of fADC. This would also
> > addresses the checkpatch warning of line exceeding 80 character.  
> 
> I'm not sure you did an equivalent changes. I believe in the original
> code precision is better. Consider low clock frequencies when 10 bit
> right shift may hide some bits of the division.

Note that those bits are eventually "hidden" in the same way later,
despite the different sequence, due to DIV_ROUND_CLOSEST() being used
at every step (both before and after the change) without other
operations occurring. Anyway,

> Care to write a python script to check the precision between old and new code?

yes, that would be nice no matter what.

-- 
Stefano

Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[Andy Shevchenko]

On Mon, Mar 23, 2020 at 2:49 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> On Mon, 23 Mar 2020 01:44:20 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com> wrote:
> > >
> > > Current implementation of the function ad7192_get_available_filter_freq
> > > repeats calculation of output data rate a few times. We can simplify
> > > these steps by refactoring out the calculation of fADC. This would also
> > > addresses the checkpatch warning of line exceeding 80 character.
> >
> > I'm not sure you did an equivalent changes. I believe in the original
> > code precision is better. Consider low clock frequencies when 10 bit
> > right shift may hide some bits of the division.
>
> Note that those bits are eventually "hidden" in the same way later,

Even if mathematically (arithmetically) evaluation is correct, we have
to remember that computers are bad with floating point and especially
kernel, which uses integer arithmetic. That said, it's easy to get
off-by-one error (due to precision lost) if we do big division before
(not so big) multiplication.

> despite the different sequence, due to DIV_ROUND_CLOSEST() being used
> at every step (both before and after the change) without other
> operations occurring.

By the way, where AD7192_SINC3_FILTER and AD7192_SINC4_FILTER
multiplications disappear and why?

-- 
With Best Regards,
Andy Shevchenko

Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[Stefano Brivio]

On Mon, 23 Mar 2020 11:28:52 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Mar 23, 2020 at 2:49 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> > On Mon, 23 Mar 2020 01:44:20 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com> wrote:  
> > > >
> > > > Current implementation of the function ad7192_get_available_filter_freq
> > > > repeats calculation of output data rate a few times. We can simplify
> > > > these steps by refactoring out the calculation of fADC. This would also
> > > > addresses the checkpatch warning of line exceeding 80 character.  
> > >
> > > I'm not sure you did an equivalent changes. I believe in the original
> > > code precision is better. Consider low clock frequencies when 10 bit
> > > right shift may hide some bits of the division.  
> >
> > Note that those bits are eventually "hidden" in the same way later,  
> 
> Even if mathematically (arithmetically) evaluation is correct, we have
> to remember that computers are bad with floating point and especially
> kernel, which uses integer arithmetic. That said, it's easy to get
> off-by-one error (due to precision lost) if we do big division before
> (not so big) multiplication.

That's exactly the point I was trying to explain below: swapping steps
in a sequence of DIV_ROUND_CLOSEST() (*not* of arithmetic divisions),
*should* not affect quantisation ("off-by-one") error.

I'm not entirely sure in this case, so a quick "demonstration" in
Python or suchlike as you suggested would be nice to have, indeed.

> > despite the different sequence, due to DIV_ROUND_CLOSEST() being used
> > at every step (both before and after the change) without other
> > operations occurring.  
> 
> By the way, where AD7192_SINC3_FILTER and AD7192_SINC4_FILTER
> multiplications disappear and why?

Those were in fact divisions (multiplications of the divisor). Overall,
these steps are now arranged in a way closer to how they are presented
in the datasheet mentioned here (up to "Chop Enabled" paragraph, page
26).

-- 
Stefano

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

[DEEPAK VARMA]

On Mon, Mar 23, 2020 at 01:40:08AM +0200, Andy Shevchenko wrote:
> On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com> wrote:
> >
> > Three macros include SYNC in their names which is a typo. Update those
> > names to SINC.
> 
> It is good to elaborate the source of the above statement (e.g.
> datasheet pages).
> 
> > Fixes: 77f6a23092c0 ("staging: iio: adc: ad7192: Add low_pass_3db_filter_frequency")
> 
> >
> 
> This blank line should go before Fixes tag. The rule is that tags are
> forming a block at the end of commit message.
> 
> > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> > Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> The code below looks good to me.

Thank you for your feedback. I agree with your comments. I will update
and send this back as next version.

Deepak.

> 
> 
> >
> > ---
> >
> > Changes since v2:
> >         - None. Version increment to follow patch series versioning.
> >
> > Changes since v1:
> >         - None. Patch added in v2 version as suggested by Stefano.
> >
> >
> >  drivers/iio/adc/ad7192.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index 02981f3d1794..d9a220d4217f 100644
> > --- a/drivers/iio/adc/ad7192.c
> > +++ b/drivers/iio/adc/ad7192.c
> > @@ -144,9 +144,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.
> > @@ -367,7 +367,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)
> > @@ -484,11 +484,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));
> > @@ -576,25 +576,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
> >
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[DEEPAK VARMA]

On Mon, Mar 23, 2020 at 01:15:31PM +0100, Stefano Brivio wrote:
> On Mon, 23 Mar 2020 11:28:52 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
> > On Mon, Mar 23, 2020 at 2:49 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> > > On Mon, 23 Mar 2020 01:44:20 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > > On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com> wrote:  
> > > > >
> > > > > Current implementation of the function ad7192_get_available_filter_freq
> > > > > repeats calculation of output data rate a few times. We can simplify
> > > > > these steps by refactoring out the calculation of fADC. This would also
> > > > > addresses the checkpatch warning of line exceeding 80 character.  
> > > >
> > > > I'm not sure you did an equivalent changes. I believe in the original
> > > > code precision is better. Consider low clock frequencies when 10 bit
> > > > right shift may hide some bits of the division.  
> > >
> > > Note that those bits are eventually "hidden" in the same way later,  
> > 
> > Even if mathematically (arithmetically) evaluation is correct, we have
> > to remember that computers are bad with floating point and especially
> > kernel, which uses integer arithmetic. That said, it's easy to get
> > off-by-one error (due to precision lost) if we do big division before
> > (not so big) multiplication.
> 
> That's exactly the point I was trying to explain below: swapping steps
> in a sequence of DIV_ROUND_CLOSEST() (*not* of arithmetic divisions),
> *should* not affect quantisation ("off-by-one") error.
> 
> I'm not entirely sure in this case, so a quick "demonstration" in
> Python or suchlike as you suggested would be nice to have, indeed.
> 
> > > despite the different sequence, due to DIV_ROUND_CLOSEST() being used
> > > at every step (both before and after the change) without other
> > > operations occurring.  
> > 
> > By the way, where AD7192_SINC3_FILTER and AD7192_SINC4_FILTER
> > multiplications disappear and why?
> 
> Those were in fact divisions (multiplications of the divisor). Overall,
> these steps are now arranged in a way closer to how they are presented
> in the datasheet mentioned here (up to "Chop Enabled" paragraph, page
> 26).
> 

Thank you Andy and Stefano for your comments. Its very thoughtful. I am
not much familiar with Python so far, but thinking on evaluating your
suggestion in a sample c program. I will share the outcome shortly.

Deepak.


> -- 
> Stefano
> 

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

[Ardelean, Alexandru]

On Mon, 2020-03-23 at 01:24 +0530, Deepak R Varma wrote:
> [External]
> 
> Re-indent enum labels as per coding style guidelines. Problem
> detected by checkpatch script.
> 

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> ---
> 
> Changes since v2:
> 	- None. Version number increment to follow patch series version.
> 
> Changes since v1:
> 	1. Separated other change into a separate patch as suggested by
> 	   Greg KH.
> 
> 
>  drivers/iio/adc/ad7192.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 8ec28aa8fa8a..02981f3d1794 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -157,8 +157,8 @@
>   */
>  
>  enum {
> -   AD7192_SYSCALIB_ZERO_SCALE,
> -   AD7192_SYSCALIB_FULL_SCALE,
> +	AD7192_SYSCALIB_ZERO_SCALE,
> +	AD7192_SYSCALIB_FULL_SCALE,
>  };
>  
>  struct ad7192_state {

Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[Ardelean, Alexandru]

On Mon, 2020-03-23 at 23:22 +0530, DEEPAK VARMA wrote:
> [External]
> 
> On Mon, Mar 23, 2020 at 01:15:31PM +0100, Stefano Brivio wrote:
> > On Mon, 23 Mar 2020 11:28:52 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > 
> > > On Mon, Mar 23, 2020 at 2:49 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> > > > On Mon, 23 Mar 2020 01:44:20 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > > > On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com>
> > > > > wrote:  
> > > > > > Current implementation of the function
> > > > > > ad7192_get_available_filter_freq
> > > > > > repeats calculation of output data rate a few times. We can simplify
> > > > > > these steps by refactoring out the calculation of fADC. This would
> > > > > > also
> > > > > > addresses the checkpatch warning of line exceeding 80 character.  
> > > > > 
> > > > > I'm not sure you did an equivalent changes. I believe in the original
> > > > > code precision is better. Consider low clock frequencies when 10 bit
> > > > > right shift may hide some bits of the division.  
> > > > 
> > > > Note that those bits are eventually "hidden" in the same way later,  
> > > 
> > > Even if mathematically (arithmetically) evaluation is correct, we have
> > > to remember that computers are bad with floating point and especially
> > > kernel, which uses integer arithmetic. That said, it's easy to get
> > > off-by-one error (due to precision lost) if we do big division before
> > > (not so big) multiplication.
> > 
> > That's exactly the point I was trying to explain below: swapping steps
> > in a sequence of DIV_ROUND_CLOSEST() (*not* of arithmetic divisions),
> > *should* not affect quantisation ("off-by-one") error.
> > 
> > I'm not entirely sure in this case, so a quick "demonstration" in
> > Python or suchlike as you suggested would be nice to have, indeed.
> > 
> > > > despite the different sequence, due to DIV_ROUND_CLOSEST() being used
> > > > at every step (both before and after the change) without other
> > > > operations occurring.  
> > > 
> > > By the way, where AD7192_SINC3_FILTER and AD7192_SINC4_FILTER
> > > multiplications disappear and why?
> > 
> > Those were in fact divisions (multiplications of the divisor). Overall,
> > these steps are now arranged in a way closer to how they are presented
> > in the datasheet mentioned here (up to "Chop Enabled" paragraph, page
> > 26).
> > 
> 
> Thank you Andy and Stefano for your comments. Its very thoughtful. I am
> not much familiar with Python so far, but thinking on evaluating your
> suggestion in a sample c program. I will share the outcome shortly.

+adding Mircea Caprioru

Umm, this math-cleanup looks pretty dangerous.
If possible, I wouldn't change it.
At least without some testing on HW.

Maybe doing math-simulations in Python scripts would also work, but not sure.

> 
> Deepak.
> 
> 
> > -- 
> > Stefano
> > 

Re: [PATCH v3 4/4] staging: iio: adc: ad7280a: Add comments to clarify stringified arguments

[Ardelean, Alexandru]

On Mon, 2020-03-23 at 01:27 +0530, Deepak R Varma wrote:
> [External]
> 
> Checkpatch would flash a check message around a stringified macro
> argument containing a '-' character. Add comment to indicate the
> argument is legitimate and doesn't need fixing.

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> 
> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> ---
> 
> Changes since v2:
> 	- None. Patch added in v3
> 
> Changes since v1:
> 	- None. Patch added in v3
> 
>  drivers/staging/iio/adc/ad7280a.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/iio/adc/ad7280a.c
> b/drivers/staging/iio/adc/ad7280a.c
> index 19a5f244dcae..bef6bd1295ea 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -824,6 +824,10 @@ static irqreturn_t ad7280_event_handler(int irq, void
> *private)
>  	return IRQ_HANDLED;
>  }
>  
> +/* Note: No need to fix checkpatch warning that reads:
> + *	CHECK: spaces preferred around that '-' (ctx:VxV)
> + * The function argument is stringified and doesn't need a fix
> + */
>  static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
>  			     in_voltage-voltage_thresh_low_value,
>  			     0644,

Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[DEEPAK VARMA]

[-- Attachment #1: Type: text/plain, Size: 3966 bytes --]

On Tue, Mar 24, 2020 at 08:06:34AM +0000, Ardelean, Alexandru wrote:
> On Mon, 2020-03-23 at 23:22 +0530, DEEPAK VARMA wrote:
> > [External]
> > 
> > On Mon, Mar 23, 2020 at 01:15:31PM +0100, Stefano Brivio wrote:
> > > On Mon, 23 Mar 2020 11:28:52 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > 
> > > > On Mon, Mar 23, 2020 at 2:49 AM Stefano Brivio <sbrivio@redhat.com> wrote:
> > > > > On Mon, 23 Mar 2020 01:44:20 +0200
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > > > > On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com>
> > > > > > wrote:  
> > > > > > > Current implementation of the function
> > > > > > > ad7192_get_available_filter_freq
> > > > > > > repeats calculation of output data rate a few times. We can simplify
> > > > > > > these steps by refactoring out the calculation of fADC. This would
> > > > > > > also
> > > > > > > addresses the checkpatch warning of line exceeding 80 character.  
> > > > > > 
> > > > > > I'm not sure you did an equivalent changes. I believe in the original
> > > > > > code precision is better. Consider low clock frequencies when 10 bit
> > > > > > right shift may hide some bits of the division.  
> > > > > 
> > > > > Note that those bits are eventually "hidden" in the same way later,  
> > > > 
> > > > Even if mathematically (arithmetically) evaluation is correct, we have
> > > > to remember that computers are bad with floating point and especially
> > > > kernel, which uses integer arithmetic. That said, it's easy to get
> > > > off-by-one error (due to precision lost) if we do big division before
> > > > (not so big) multiplication.
> > > 
> > > That's exactly the point I was trying to explain below: swapping steps
> > > in a sequence of DIV_ROUND_CLOSEST() (*not* of arithmetic divisions),
> > > *should* not affect quantisation ("off-by-one") error.
> > > 
> > > I'm not entirely sure in this case, so a quick "demonstration" in
> > > Python or suchlike as you suggested would be nice to have, indeed.
> > > 
> > > > > despite the different sequence, due to DIV_ROUND_CLOSEST() being used
> > > > > at every step (both before and after the change) without other
> > > > > operations occurring.  
> > > > 
> > > > By the way, where AD7192_SINC3_FILTER and AD7192_SINC4_FILTER
> > > > multiplications disappear and why?
> > > 
> > > Those were in fact divisions (multiplications of the divisor). Overall,
> > > these steps are now arranged in a way closer to how they are presented
> > > in the datasheet mentioned here (up to "Chop Enabled" paragraph, page
> > > 26).
> > > 
> > 
> > Thank you Andy and Stefano for your comments. Its very thoughtful. I am
> > not much familiar with Python so far, but thinking on evaluating your
> > suggestion in a sample c program. I will share the outcome shortly.
> 
> +adding Mircea Caprioru
> 
> Umm, this math-cleanup looks pretty dangerous.
> If possible, I wouldn't change it.
> At least without some testing on HW.
> 
> Maybe doing math-simulations in Python scripts would also work, but not sure.
> 

Hello All,
I further reviewed current and proposed implementation of the
get_filter_freq() function[Thank you Stefano for your time]. We realised that I
was wrong in swapping DIV_ROUND_CLOSEST calls with mixing
multiplication in it. It is indeed incorrect to mix multiplication if we
want to reorder the calls. Comparison of the results from current and
proposed implementation proved it. In short, the patch I sent is wrong.
My apologies for any trouble.

We have further improved the test program with a revised implementation
[attached with this email] and found that this revision appears to
provide more accurate results [I think].

May I please request you to review the attached test program, verify the
results and share your feedback.

Thank you for your patience and the opportunity to learn a few new
things!

Deepak.

> > 
> > Deepak.
> > 
> > 
> > > -- 
> > > Stefano
> > > 

[-- Attachment #2: test_adc_freq.c --]
[-- Type: text/x-csrc, Size: 3231 bytes --]

#include <stdio.h>
#include <string.h>
#include <math.h>

#define AD7192_MODE_RATE(x)     ((x) & 0x3FF)
#define AD7192_NO_SINC_FILTER   1
#define AD7192_SINC3_FILTER     3
#define AD7192_SINC4_FILTER     4
#define AD7192_EXT_FREQ_MHZ_MIN 2457600
#define AD7192_EXT_FREQ_MHZ_MAX 5120000
#define AD7192_INT_FREQ_MHZ     4915200

#define DIV_ROUND_CLOSEST(x, divisor)(			\
{							\
	typeof(x) __x = x;				\
	typeof(divisor) __d = divisor;			\
	(((typeof(x))-1) > 0 ||				\
	((typeof(divisor))-1) > 0 ||			\
	(((__x) > 0) == ((__d) > 0))) ?			\
		(((__x) + ((__d) / 2)) / (__d)) :	\
		(((__x) - ((__d) / 2)) / (__d));	\
}							\
)

struct adc7192_state {
	unsigned int mode;
	unsigned int fclk;
};

/* ***
 * Current implementation 
 * ***
 */
void old_func_get_freq(struct adc7192_state *st, int *freq)
{
	unsigned int fadc;

	/* Formulas for filter at page 25 of the datasheet */
	fadc = DIV_ROUND_CLOSEST(st->fclk,
			AD7192_SINC4_FILTER * AD7192_MODE_RATE(st->mode));
	freq[0] = DIV_ROUND_CLOSEST(fadc * 240, 1024);

	fadc = DIV_ROUND_CLOSEST(st->fclk,
			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));
	freq[2] = DIV_ROUND_CLOSEST(fadc * 230, 1024);
	freq[3] = DIV_ROUND_CLOSEST(fadc * 272, 1024);
}

/* ***
 * Proposed implementation 
 * ***
 */
void new_func_get_freq(struct adc7192_state *st, int *freq)
{
	/* Formulas for filter at page 25 of the datasheet */
	freq[0] = DIV_ROUND_CLOSEST(st->fclk * 240, AD7192_MODE_RATE(st->mode) *
						    AD7192_SINC4_FILTER * 1024);
	freq[1] = DIV_ROUND_CLOSEST(st->fclk * 240, AD7192_MODE_RATE(st->mode) *
						    AD7192_SINC3_FILTER * 1024);
	freq[2] = DIV_ROUND_CLOSEST(st->fclk * 230, AD7192_MODE_RATE(st->mode) *
						    1024);
	freq[3] = DIV_ROUND_CLOSEST(st->fclk * 272, AD7192_MODE_RATE(st->mode) *
						    1024);
}


/* ***
 * sample function using floats to demonstrate accuracy of new results 
 * ***
 */
void test(struct adc7192_state *st, float *freq)
{
	freq[0] = (float)st->fclk * 240 /
		  (AD7192_MODE_RATE(st->mode) * AD7192_SINC4_FILTER * 1024);
	freq[1] = (float)st->fclk * 240 /
		  (AD7192_MODE_RATE(st->mode) * AD7192_SINC3_FILTER * 1024);
	freq[2] = (float)st->fclk * 230 /
		  (AD7192_MODE_RATE(st->mode) * 1024);
	freq[3] = (float)st->fclk * 272 /
		  (AD7192_MODE_RATE(st->mode) * 1024);
}

int main(void)
{
	struct adc7192_state st;
	int o[4], n[4], i, j;
	float f[4];

	for (i = AD7192_EXT_FREQ_MHZ_MIN; i < AD7192_EXT_FREQ_MHZ_MAX; i++) {
		st.fclk = i;
		for (j = 1; j < 1023; j++) {
			st.mode = j;
			old_func_get_freq(&st, o);
			new_func_get_freq(&st, n);

			if (memcmp(o, n, sizeof(o))) {
				printf("fclk: %d, mode: %d\n", i, j);
				printf("old: %i %i %i %i\n",
					o[0], o[1], o[2], o[3]);
				printf("new: %i %i %i %i\n",
					n[0], n[1], n[2], n[3]);

				test(&st, f);
				printf("float: %f %f %f %f\n",
					f[0], f[1], f[2], f[3]);

				if (fabs(f[0] - n[0]) <= fabs(f[0] - o[0]) &&
				    fabs(f[1] - n[1]) <= fabs(f[1] - o[1]) &&
				    fabs(f[2] - n[2]) <= fabs(f[2] - o[2]) &&
				    fabs(f[3] - n[3]) <= fabs(f[3] - o[3]))
					printf("  improvement\n");
				else
					return 1;
			}

		}
	}

	return 0;
}

Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[Andy Shevchenko]

On Wed, Mar 25, 2020 at 7:38 PM DEEPAK VARMA <mh12gx2825@gmail.com> wrote:
> On Tue, Mar 24, 2020 at 08:06:34AM +0000, Ardelean, Alexandru wrote:

> I further reviewed current and proposed implementation of the
> get_filter_freq() function[Thank you Stefano for your time]. We realised that I
> was wrong in swapping DIV_ROUND_CLOSEST calls with mixing
> multiplication in it. It is indeed incorrect to mix multiplication if we
> want to reorder the calls. Comparison of the results from current and
> proposed implementation proved it. In short, the patch I sent is wrong.
> My apologies for any trouble.


No problem, that's what community is for!
And everybody has been learning always something new.

> We have further improved the test program with a revised implementation
> [attached with this email] and found that this revision appears to
> provide more accurate results [I think].
>
> May I please request you to review the attached test program, verify the
> results and share your feedback.

Do you have GH account? I would highly recommend to use its Gist
facility for such small (one file) type of programs. It allows you to
update them and see versioning. Dunno if possible to comment, but
anyway more advantages.
Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[Stefano Brivio]

On Wed, 25 Mar 2020 23:08:17 +0530
DEEPAK VARMA <mh12gx2825@gmail.com> wrote:

> On Tue, Mar 24, 2020 at 08:06:34AM +0000, Ardelean, Alexandru wrote:
> > On Mon, 2020-03-23 at 23:22 +0530, DEEPAK VARMA wrote:  
> > > [External]
> > > 
> > > On Mon, Mar 23, 2020 at 01:15:31PM +0100, Stefano Brivio wrote:  
> > > > On Mon, 23 Mar 2020 11:28:52 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > >   
> > > > > On Mon, Mar 23, 2020 at 2:49 AM Stefano Brivio <sbrivio@redhat.com> wrote:  
> > > > > > On Mon, 23 Mar 2020 01:44:20 +0200
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:    
> > > > > > > On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <mh12gx2825@gmail.com>
> > > > > > > wrote:    
> > > > > > > > Current implementation of the function
> > > > > > > > ad7192_get_available_filter_freq
> > > > > > > > repeats calculation of output data rate a few times. We can simplify
> > > > > > > > these steps by refactoring out the calculation of fADC. This would
> > > > > > > > also
> > > > > > > > addresses the checkpatch warning of line exceeding 80 character.    
> > > > > > > 
> > > > > > > I'm not sure you did an equivalent changes. I believe in the original
> > > > > > > code precision is better. Consider low clock frequencies when 10 bit
> > > > > > > right shift may hide some bits of the division.    
> > > > > > 
> > > > > > Note that those bits are eventually "hidden" in the same way later,    
> > > > > 
> > > > > Even if mathematically (arithmetically) evaluation is correct, we have
> > > > > to remember that computers are bad with floating point and especially
> > > > > kernel, which uses integer arithmetic. That said, it's easy to get
> > > > > off-by-one error (due to precision lost) if we do big division before
> > > > > (not so big) multiplication.  
> > > > 
> > > > That's exactly the point I was trying to explain below: swapping steps
> > > > in a sequence of DIV_ROUND_CLOSEST() (*not* of arithmetic divisions),
> > > > *should* not affect quantisation ("off-by-one") error.
> > > > 
> > > > I'm not entirely sure in this case, so a quick "demonstration" in
> > > > Python or suchlike as you suggested would be nice to have, indeed.
> > > >   
> > > > > > despite the different sequence, due to DIV_ROUND_CLOSEST() being used
> > > > > > at every step (both before and after the change) without other
> > > > > > operations occurring.    
> > > > > 
> > > > > By the way, where AD7192_SINC3_FILTER and AD7192_SINC4_FILTER
> > > > > multiplications disappear and why?  
> > > > 
> > > > Those were in fact divisions (multiplications of the divisor). Overall,
> > > > these steps are now arranged in a way closer to how they are presented
> > > > in the datasheet mentioned here (up to "Chop Enabled" paragraph, page
> > > > 26).
> > > >   
> > > 
> > > Thank you Andy and Stefano for your comments. Its very thoughtful. I am
> > > not much familiar with Python so far, but thinking on evaluating your
> > > suggestion in a sample c program. I will share the outcome shortly.  
> > 
> > +adding Mircea Caprioru
> > 
> > Umm, this math-cleanup looks pretty dangerous.
> > If possible, I wouldn't change it.
> > At least without some testing on HW.
> > 
> > Maybe doing math-simulations in Python scripts would also work, but not sure.
> >   
> 
> Hello All,
> I further reviewed current and proposed implementation of the
> get_filter_freq() function[Thank you Stefano for your time]. We realised that I
> was wrong in swapping DIV_ROUND_CLOSEST calls with mixing
> multiplication in it. It is indeed incorrect to mix multiplication if we
> want to reorder the calls.

Specifically, my wrong assumption was that we were just reordering
DIV_ROUND_CLOSEST() operations -- that's actually fine: with rounding,
integer division (not mixed with other operations) is associative.

However, here, we had those 0.23, 0.24, 0.272 factors. I missed them.

If we factor together these multiplications and do them first, of
course, the result is more accurate, which, I assume from the usage of
DIV_ROUND_CLOSEST() and the typical application of the ADC, is quite
relevant here.

The observation here is that, by doing all of them first, the code is
more accurate (e.g. with a fclk of 2458464 and 904 as "mode" we get 722
for freq[4] instead of 723 -- 722.377747 with real division) and
actually becomes more readable, too.

> [...]
>
> May I please request you to review the attached test program, verify the
> results and share your feedback.

As an alternative to what Andy suggested, I guess you could also post it
inline, just like we do for patch reviews. Commenting becomes natural
and the discussion can be referenced later via archives.

-- 
Stefano

Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization

[Ardelean, Alexandru]

On Wed, 2020-03-25 at 23:08 +0530, DEEPAK VARMA wrote:
> [External]
> 
> On Tue, Mar 24, 2020 at 08:06:34AM +0000, Ardelean, Alexandru wrote:
> > On Mon, 2020-03-23 at 23:22 +0530, DEEPAK VARMA wrote:
> > > [External]
> > > 
> > > On Mon, Mar 23, 2020 at 01:15:31PM +0100, Stefano Brivio wrote:
> > > > On Mon, 23 Mar 2020 11:28:52 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > 
> > > > > On Mon, Mar 23, 2020 at 2:49 AM Stefano Brivio <sbrivio@redhat.com>
> > > > > wrote:
> > > > > > On Mon, 23 Mar 2020 01:44:20 +0200
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > > > > > On Sun, Mar 22, 2020 at 9:57 PM Deepak R Varma <
> > > > > > > mh12gx2825@gmail.com>
> > > > > > > wrote:  
> > > > > > > > Current implementation of the function
> > > > > > > > ad7192_get_available_filter_freq
> > > > > > > > repeats calculation of output data rate a few times. We can
> > > > > > > > simplify
> > > > > > > > these steps by refactoring out the calculation of fADC. This
> > > > > > > > would
> > > > > > > > also
> > > > > > > > addresses the checkpatch warning of line exceeding 80
> > > > > > > > character.  
> > > > > > > 
> > > > > > > I'm not sure you did an equivalent changes. I believe in the
> > > > > > > original
> > > > > > > code precision is better. Consider low clock frequencies when 10
> > > > > > > bit
> > > > > > > right shift may hide some bits of the division.  
> > > > > > 
> > > > > > Note that those bits are eventually "hidden" in the same way
> > > > > > later,  
> > > > > 
> > > > > Even if mathematically (arithmetically) evaluation is correct, we have
> > > > > to remember that computers are bad with floating point and especially
> > > > > kernel, which uses integer arithmetic. That said, it's easy to get
> > > > > off-by-one error (due to precision lost) if we do big division before
> > > > > (not so big) multiplication.
> > > > 
> > > > That's exactly the point I was trying to explain below: swapping steps
> > > > in a sequence of DIV_ROUND_CLOSEST() (*not* of arithmetic divisions),
> > > > *should* not affect quantisation ("off-by-one") error.
> > > > 
> > > > I'm not entirely sure in this case, so a quick "demonstration" in
> > > > Python or suchlike as you suggested would be nice to have, indeed.
> > > > 
> > > > > > despite the different sequence, due to DIV_ROUND_CLOSEST() being
> > > > > > used
> > > > > > at every step (both before and after the change) without other
> > > > > > operations occurring.  
> > > > > 
> > > > > By the way, where AD7192_SINC3_FILTER and AD7192_SINC4_FILTER
> > > > > multiplications disappear and why?
> > > > 
> > > > Those were in fact divisions (multiplications of the divisor). Overall,
> > > > these steps are now arranged in a way closer to how they are presented
> > > > in the datasheet mentioned here (up to "Chop Enabled" paragraph, page
> > > > 26).
> > > > 
> > > 
> > > Thank you Andy and Stefano for your comments. Its very thoughtful. I am
> > > not much familiar with Python so far, but thinking on evaluating your
> > > suggestion in a sample c program. I will share the outcome shortly.
> > 
> > +adding Mircea Caprioru
> > 
> > Umm, this math-cleanup looks pretty dangerous.
> > If possible, I wouldn't change it.
> > At least without some testing on HW.
> > 
> > Maybe doing math-simulations in Python scripts would also work, but not
> > sure.
> > 
> 
> Hello All,
> I further reviewed current and proposed implementation of the
> get_filter_freq() function[Thank you Stefano for your time]. We realised that
> I
> was wrong in swapping DIV_ROUND_CLOSEST calls with mixing
> multiplication in it. It is indeed incorrect to mix multiplication if we
> want to reorder the calls. Comparison of the results from current and
> proposed implementation proved it. In short, the patch I sent is wrong.
> My apologies for any trouble.
> 
> We have further improved the test program with a revised implementation
> [attached with this email] and found that this revision appears to
> provide more accurate results [I think].
> 
> May I please request you to review the attached test program, verify the
> results and share your feedback.
> 
> Thank you for your patience and the opportunity to learn a few new
> things!
> 

Hey,

Many thanks for the test program.
I admit, it is a good way for convincing me [and my paranoia about changing math
in the ADI drivers]. I don't want to say that the math we did is the best, but
since it was tested... ¯\_(ツ)_/¯
[ Also, there's plenty of ADI drivers that we have to look at, so that also
makes me paranoid ]

I took a look and ran your program.
I like the improved results.

Only one suggestion I have for it; maybe use an extra variable for part of the
divisor; see here:

void new_func_get_freq1(struct adc7192_state *st, int *freq)
{
        unsigned int div;

        /* Formulas for filter at page 25 of the datasheet */
        div = AD7192_MODE_RATE(st->mode) * 1024;
        freq[0] = DIV_ROUND_CLOSEST(st->fclk * 240, div * AD7192_SINC4_FILTER);
        freq[1] = DIV_ROUND_CLOSEST(st->fclk * 240, div * AD7192_SINC3_FILTER);

        freq[2] = DIV_ROUND_CLOSEST(st->fclk * 230, div);
        freq[3] = DIV_ROUND_CLOSEST(st->fclk * 272, div);
}

if you want to you can go extra-further a bit and re-add the fadc for the first
2 frequencies; so something like:


void new_func_get_freq2(struct adc7192_state *st, int *freq)
{
        unsigned int div, fadc;

        /* Formulas for filter at page 25 of the datasheet */
        fadc = st->fclk * 240;
        div = AD7192_MODE_RATE(st->mode) * 1024;
        freq[0] = DIV_ROUND_CLOSEST(fadc, div * AD7192_SINC4_FILTER);
        freq[1] = DIV_ROUND_CLOSEST(fadc, div * AD7192_SINC3_FILTER);

        freq[2] = DIV_ROUND_CLOSEST(st->fclk * 230, div);
        freq[3] = DIV_ROUND_CLOSEST(st->fclk * 272, div);
}

either version is fine from my side;

Thanks
Alex


> Deepak.
> 
> > > Deepak.
> > > 
> > > 
> > > > -- 
> > > > Stefano
> > > > 

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

[Ardelean, Alexandru]

On Mon, 2020-03-23 at 01:24 +0530, Deepak R Varma wrote:
> [External]
> 
> Re-indent enum labels as per coding style guidelines. Problem
> detected by checkpatch script.

It's customary to keep the review tags you receive from earlier patchsets in
your next sets.
Uou can now add it [typically before your Signed-off-by tag] starting with V3
and onwards.
You don't need to send a V4 just for this.

So, I added [for this]:

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>


> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> ---
> 
> Changes since v2:
> 	- None. Version number increment to follow patch series version.
> 
> Changes since v1:
> 	1. Separated other change into a separate patch as suggested by
> 	   Greg KH.
> 
> 
>  drivers/iio/adc/ad7192.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 8ec28aa8fa8a..02981f3d1794 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -157,8 +157,8 @@
>   */
>  
>  enum {
> -   AD7192_SYSCALIB_ZERO_SCALE,
> -   AD7192_SYSCALIB_FULL_SCALE,
> +	AD7192_SYSCALIB_ZERO_SCALE,
> +	AD7192_SYSCALIB_FULL_SCALE,
>  };
>  
>  struct ad7192_state {

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

[Ardelean, Alexandru]

On Mon, 2020-03-23 at 01:25 +0530, Deepak R Varma wrote:
> [External]
> 
> Three macros include SYNC in their names which is a typo. Update those
> names to SINC.
> Fixes: 77f6a23092c0 ("staging: iio: adc: ad7192: Add
> low_pass_3db_filter_frequency")

you can keep the commit description as long as it needs to be; no need to wrap
it; 

> 

other than that:

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
> 
> ---
> 
> Changes since v2:
> 	- None. Version increment to follow patch series versioning.
> 
> Changes since v1:
> 	- None. Patch added in v2 version as suggested by Stefano.
> 
> 
>  drivers/iio/adc/ad7192.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 02981f3d1794..d9a220d4217f 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -144,9 +144,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.
> @@ -367,7 +367,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)
> @@ -484,11 +484,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));
> @@ -576,25 +576,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;

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

[Ardelean, Alexandru]

On Thu, 2020-03-26 at 08:19 +0000, Ardelean, Alexandru wrote:
> [External]
> 
> On Mon, 2020-03-23 at 01:24 +0530, Deepak R Varma wrote:
> > [External]
> > 
> > Re-indent enum labels as per coding style guidelines. Problem
> > detected by checkpatch script.
> 
> It's customary to keep the review tags you receive from earlier patchsets in
> your next sets.
> Uou can now add it [typically before your Signed-off-by tag] starting with V3
> and onwards.
> You don't need to send a V4 just for this.
> 
> So, I added [for this]:
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>


Wait; my bad; I am getting lost in emails now.
I found a Reviewed-by tag on this set.
Disregard this comment.

> 
> 
> > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> > ---
> > 
> > Changes since v2:
> > 	- None. Version number increment to follow patch series version.
> > 
> > Changes since v1:
> > 	1. Separated other change into a separate patch as suggested by
> > 	   Greg KH.
> > 
> > 
> >  drivers/iio/adc/ad7192.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> > index 8ec28aa8fa8a..02981f3d1794 100644
> > --- a/drivers/iio/adc/ad7192.c
> > +++ b/drivers/iio/adc/ad7192.c
> > @@ -157,8 +157,8 @@
> >   */
> >  
> >  enum {
> > -   AD7192_SYSCALIB_ZERO_SCALE,
> > -   AD7192_SYSCALIB_FULL_SCALE,
> > +	AD7192_SYSCALIB_ZERO_SCALE,
> > +	AD7192_SYSCALIB_FULL_SCALE,
> >  };
> >  
> >  struct ad7192_state {

Re: [PATCH v3 4/4] staging: iio: adc: ad7280a: Add comments to clarify stringified arguments

[Jonathan Cameron]

On Tue, 24 Mar 2020 08:07:28 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Mon, 2020-03-23 at 01:27 +0530, Deepak R Varma wrote:
> > [External]
> > 
> > Checkpatch would flash a check message around a stringified macro
> > argument containing a '-' character. Add comment to indicate the
> > argument is legitimate and doesn't need fixing.  
> 
> Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

One minor thing inline, otherwise thanks for doing this.

Jonathan

> 
> > 
> > Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> > Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
> > 
> > ---
> > 
> > Changes since v2:
> > 	- None. Patch added in v3
> > 
> > Changes since v1:
> > 	- None. Patch added in v3
> > 
> >  drivers/staging/iio/adc/ad7280a.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7280a.c
> > b/drivers/staging/iio/adc/ad7280a.c
> > index 19a5f244dcae..bef6bd1295ea 100644
> > --- a/drivers/staging/iio/adc/ad7280a.c
> > +++ b/drivers/staging/iio/adc/ad7280a.c
> > @@ -824,6 +824,10 @@ static irqreturn_t ad7280_event_handler(int irq, void
> > *private)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +/* Note: No need to fix checkpatch warning that reads:
Trivial, but please use standard multiline comment syntax (for most but not
all of the kernel)

/*
 * Note:
..
 */
> > + *	CHECK: spaces preferred around that '-' (ctx:VxV)
> > + * The function argument is stringified and doesn't need a fix
> > + */
> >  static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
> >  			     in_voltage-voltage_thresh_low_value,
> >  			     0644,