* [PATCH v3 0/4] staging: iio: adc: General code reformatting / cleanup patchset @ 2020-03-22 19:53 Deepak R Varma 2020-03-22 19:54 ` [PATCH v3 1/4] staging: iio: adc: ad7192: Re-indent enum labels Deepak R Varma ` (3 more replies) 0 siblings, 4 replies; 23+ messages in thread From: Deepak R Varma @ 2020-03-22 19:53 UTC (permalink / raw) To: outreachy-kernel, gregkh, daniel.baluta, kieran.bingham Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 1/4] staging: iio: adc: ad7192: Re-indent enum labels 2020-03-22 19:53 [PATCH v3 0/4] staging: iio: adc: General code reformatting / cleanup patchset Deepak R Varma @ 2020-03-22 19:54 ` Deepak R Varma 2020-03-24 7:18 ` Ardelean, Alexandru 2020-03-26 8:19 ` Ardelean, Alexandru 2020-03-22 19:55 ` [PATCH v3 2/4] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC Deepak R Varma ` (2 subsequent siblings) 3 siblings, 2 replies; 23+ messages in thread From: Deepak R Varma @ 2020-03-22 19:54 UTC (permalink / raw) To: outreachy-kernel, gregkh, daniel.baluta, kieran.bingham Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] staging: iio: adc: ad7192: Re-indent enum labels 2020-03-22 19:54 ` [PATCH v3 1/4] staging: iio: adc: ad7192: Re-indent enum labels Deepak R Varma @ 2020-03-24 7:18 ` Ardelean, Alexandru 2020-03-26 8:19 ` Ardelean, Alexandru 1 sibling, 0 replies; 23+ messages in thread From: Ardelean, Alexandru @ 2020-03-24 7:18 UTC (permalink / raw) To: mh12gx2825, gregkh, outreachy-kernel, kieran.bingham, daniel.baluta Cc: linux-iio, Hennerich, Michael, jic23, lars, knaack.h, pmeerw 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 { ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] staging: iio: adc: ad7192: Re-indent enum labels 2020-03-22 19:54 ` [PATCH v3 1/4] staging: iio: adc: ad7192: Re-indent enum labels Deepak R Varma 2020-03-24 7:18 ` Ardelean, Alexandru @ 2020-03-26 8:19 ` Ardelean, Alexandru 2020-03-26 8:22 ` Ardelean, Alexandru 1 sibling, 1 reply; 23+ messages in thread From: Ardelean, Alexandru @ 2020-03-26 8:19 UTC (permalink / raw) To: mh12gx2825, gregkh, outreachy-kernel, kieran.bingham, daniel.baluta Cc: linux-iio, Hennerich, Michael, jic23, lars, knaack.h, pmeerw 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 { ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/4] staging: iio: adc: ad7192: Re-indent enum labels 2020-03-26 8:19 ` Ardelean, Alexandru @ 2020-03-26 8:22 ` Ardelean, Alexandru 0 siblings, 0 replies; 23+ messages in thread From: Ardelean, Alexandru @ 2020-03-26 8:22 UTC (permalink / raw) To: mh12gx2825, gregkh, outreachy-kernel, kieran.bingham, daniel.baluta Cc: Hennerich, Michael, jic23, lars, linux-iio, pmeerw, knaack.h 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 { ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 2/4] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC 2020-03-22 19:53 [PATCH v3 0/4] staging: iio: adc: General code reformatting / cleanup patchset Deepak R Varma 2020-03-22 19:54 ` [PATCH v3 1/4] staging: iio: adc: ad7192: Re-indent enum labels Deepak R Varma @ 2020-03-22 19:55 ` Deepak R Varma 2020-03-22 23:40 ` Andy Shevchenko 2020-03-26 8:20 ` Ardelean, Alexandru 2020-03-22 19:56 ` [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization Deepak R Varma 2020-03-22 19:57 ` [PATCH v3 4/4] staging: iio: adc: ad7280a: Add comments to clarify stringified arguments Deepak R Varma 3 siblings, 2 replies; 23+ messages in thread From: Deepak R Varma @ 2020-03-22 19:55 UTC (permalink / raw) To: outreachy-kernel, gregkh, daniel.baluta, kieran.bingham Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC 2020-03-22 19:55 ` [PATCH v3 2/4] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC Deepak R Varma @ 2020-03-22 23:40 ` Andy Shevchenko 2020-03-23 17:50 ` DEEPAK VARMA 2020-03-26 8:20 ` Ardelean, Alexandru 1 sibling, 1 reply; 23+ messages in thread From: Andy Shevchenko @ 2020-03-22 23:40 UTC (permalink / raw) To: Deepak R Varma Cc: outreachy-kernel, Greg Kroah-Hartman, Daniel Baluta, kieran.bingham, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald, linux-iio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC 2020-03-22 23:40 ` Andy Shevchenko @ 2020-03-23 17:50 ` DEEPAK VARMA 0 siblings, 0 replies; 23+ messages in thread From: DEEPAK VARMA @ 2020-03-23 17:50 UTC (permalink / raw) To: Andy Shevchenko Cc: outreachy-kernel, Greg Kroah-Hartman, Daniel Baluta, kieran.bingham, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald, linux-iio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/4] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC 2020-03-22 19:55 ` [PATCH v3 2/4] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC Deepak R Varma 2020-03-22 23:40 ` Andy Shevchenko @ 2020-03-26 8:20 ` Ardelean, Alexandru 1 sibling, 0 replies; 23+ messages in thread From: Ardelean, Alexandru @ 2020-03-26 8:20 UTC (permalink / raw) To: mh12gx2825, gregkh, outreachy-kernel, kieran.bingham, daniel.baluta Cc: linux-iio, Hennerich, Michael, jic23, lars, knaack.h, pmeerw 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; ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-22 19:53 [PATCH v3 0/4] staging: iio: adc: General code reformatting / cleanup patchset Deepak R Varma 2020-03-22 19:54 ` [PATCH v3 1/4] staging: iio: adc: ad7192: Re-indent enum labels Deepak R Varma 2020-03-22 19:55 ` [PATCH v3 2/4] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC Deepak R Varma @ 2020-03-22 19:56 ` Deepak R Varma 2020-03-22 23:44 ` Andy Shevchenko 2020-03-22 19:57 ` [PATCH v3 4/4] staging: iio: adc: ad7280a: Add comments to clarify stringified arguments Deepak R Varma 3 siblings, 1 reply; 23+ messages in thread From: Deepak R Varma @ 2020-03-22 19:56 UTC (permalink / raw) To: outreachy-kernel, gregkh, daniel.baluta, kieran.bingham Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-22 19:56 ` [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization Deepak R Varma @ 2020-03-22 23:44 ` Andy Shevchenko 2020-03-23 0:49 ` [Outreachy kernel] " Stefano Brivio 0 siblings, 1 reply; 23+ messages in thread From: Andy Shevchenko @ 2020-03-22 23:44 UTC (permalink / raw) To: Deepak R Varma Cc: outreachy-kernel, Greg Kroah-Hartman, Daniel Baluta, kieran.bingham, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald, linux-iio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-22 23:44 ` Andy Shevchenko @ 2020-03-23 0:49 ` Stefano Brivio 2020-03-23 9:28 ` Andy Shevchenko 0 siblings, 1 reply; 23+ messages in thread From: Stefano Brivio @ 2020-03-23 0:49 UTC (permalink / raw) To: Andy Shevchenko Cc: Deepak R Varma, outreachy-kernel, Greg Kroah-Hartman, Daniel Baluta, kieran.bingham, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald, linux-iio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-23 0:49 ` [Outreachy kernel] " Stefano Brivio @ 2020-03-23 9:28 ` Andy Shevchenko 2020-03-23 12:15 ` Stefano Brivio 0 siblings, 1 reply; 23+ messages in thread From: Andy Shevchenko @ 2020-03-23 9:28 UTC (permalink / raw) To: Stefano Brivio Cc: Deepak R Varma, outreachy-kernel, Greg Kroah-Hartman, Daniel Baluta, kieran.bingham, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald, linux-iio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-23 9:28 ` Andy Shevchenko @ 2020-03-23 12:15 ` Stefano Brivio 2020-03-23 17:52 ` DEEPAK VARMA 0 siblings, 1 reply; 23+ messages in thread From: Stefano Brivio @ 2020-03-23 12:15 UTC (permalink / raw) To: Andy Shevchenko Cc: Deepak R Varma, outreachy-kernel, Greg Kroah-Hartman, Daniel Baluta, kieran.bingham, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald, linux-iio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-23 12:15 ` Stefano Brivio @ 2020-03-23 17:52 ` DEEPAK VARMA 2020-03-24 8:06 ` Ardelean, Alexandru 0 siblings, 1 reply; 23+ messages in thread From: DEEPAK VARMA @ 2020-03-23 17:52 UTC (permalink / raw) To: Stefano Brivio Cc: Andy Shevchenko, outreachy-kernel, Greg Kroah-Hartman, Daniel Baluta, kieran.bingham, Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald, linux-iio 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 > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-23 17:52 ` DEEPAK VARMA @ 2020-03-24 8:06 ` Ardelean, Alexandru 2020-03-25 17:38 ` DEEPAK VARMA 0 siblings, 1 reply; 23+ messages in thread From: Ardelean, Alexandru @ 2020-03-24 8:06 UTC (permalink / raw) To: mh12gx2825, sbrivio, Caprioru, Mircea Cc: kieran.bingham, gregkh, linux-iio, jic23, outreachy-kernel, Hennerich, Michael, lars, andy.shevchenko, daniel.baluta, pmeerw, knaack.h 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 > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-24 8:06 ` Ardelean, Alexandru @ 2020-03-25 17:38 ` DEEPAK VARMA 2020-03-25 19:37 ` Andy Shevchenko ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: DEEPAK VARMA @ 2020-03-25 17:38 UTC (permalink / raw) To: Ardelean, Alexandru Cc: sbrivio, Caprioru, Mircea, kieran.bingham, gregkh, linux-iio, jic23, outreachy-kernel, Hennerich, Michael, lars, andy.shevchenko, daniel.baluta, pmeerw, knaack.h [-- 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; } ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-25 17:38 ` DEEPAK VARMA @ 2020-03-25 19:37 ` Andy Shevchenko 2020-03-25 19:59 ` Stefano Brivio 2020-03-26 8:15 ` Ardelean, Alexandru 2 siblings, 0 replies; 23+ messages in thread From: Andy Shevchenko @ 2020-03-25 19:37 UTC (permalink / raw) To: DEEPAK VARMA Cc: Ardelean, Alexandru, sbrivio, Caprioru, Mircea, kieran.bingham, gregkh, linux-iio, jic23, outreachy-kernel, Hennerich, Michael, lars, daniel.baluta, pmeerw, knaack.h 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-25 17:38 ` DEEPAK VARMA 2020-03-25 19:37 ` Andy Shevchenko @ 2020-03-25 19:59 ` Stefano Brivio 2020-03-26 8:15 ` Ardelean, Alexandru 2 siblings, 0 replies; 23+ messages in thread From: Stefano Brivio @ 2020-03-25 19:59 UTC (permalink / raw) To: DEEPAK VARMA Cc: Ardelean, Alexandru, Caprioru, Mircea, kieran.bingham, gregkh, linux-iio, jic23, outreachy-kernel, Hennerich, Michael, lars, andy.shevchenko, daniel.baluta, pmeerw, knaack.h 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Outreachy kernel] Re: [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization 2020-03-25 17:38 ` DEEPAK VARMA 2020-03-25 19:37 ` Andy Shevchenko 2020-03-25 19:59 ` Stefano Brivio @ 2020-03-26 8:15 ` Ardelean, Alexandru 2 siblings, 0 replies; 23+ messages in thread From: Ardelean, Alexandru @ 2020-03-26 8:15 UTC (permalink / raw) To: mh12gx2825 Cc: sbrivio, kieran.bingham, gregkh, Caprioru, Mircea, linux-iio, jic23, outreachy-kernel, Hennerich, Michael, lars, andy.shevchenko, daniel.baluta, pmeerw, knaack.h 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 > > > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 4/4] staging: iio: adc: ad7280a: Add comments to clarify stringified arguments 2020-03-22 19:53 [PATCH v3 0/4] staging: iio: adc: General code reformatting / cleanup patchset Deepak R Varma ` (2 preceding siblings ...) 2020-03-22 19:56 ` [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization Deepak R Varma @ 2020-03-22 19:57 ` Deepak R Varma 2020-03-24 8:07 ` Ardelean, Alexandru 3 siblings, 1 reply; 23+ messages in thread From: Deepak R Varma @ 2020-03-22 19:57 UTC (permalink / raw) To: outreachy-kernel, gregkh, daniel.baluta, kieran.bingham Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh, linux-iio 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/4] staging: iio: adc: ad7280a: Add comments to clarify stringified arguments 2020-03-22 19:57 ` [PATCH v3 4/4] staging: iio: adc: ad7280a: Add comments to clarify stringified arguments Deepak R Varma @ 2020-03-24 8:07 ` Ardelean, Alexandru 2020-03-28 13:28 ` Jonathan Cameron 0 siblings, 1 reply; 23+ messages in thread From: Ardelean, Alexandru @ 2020-03-24 8:07 UTC (permalink / raw) To: mh12gx2825, gregkh, outreachy-kernel, kieran.bingham, daniel.baluta Cc: linux-iio, Hennerich, Michael, jic23, lars, knaack.h, pmeerw 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, ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/4] staging: iio: adc: ad7280a: Add comments to clarify stringified arguments 2020-03-24 8:07 ` Ardelean, Alexandru @ 2020-03-28 13:28 ` Jonathan Cameron 0 siblings, 0 replies; 23+ messages in thread From: Jonathan Cameron @ 2020-03-28 13:28 UTC (permalink / raw) To: Ardelean, Alexandru Cc: mh12gx2825, gregkh, outreachy-kernel, kieran.bingham, daniel.baluta, linux-iio, Hennerich, Michael, lars, knaack.h, pmeerw 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, ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-03-28 13:28 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-22 19:53 [PATCH v3 0/4] staging: iio: adc: General code reformatting / cleanup patchset Deepak R Varma 2020-03-22 19:54 ` [PATCH v3 1/4] staging: iio: adc: ad7192: Re-indent enum labels Deepak R Varma 2020-03-24 7:18 ` Ardelean, Alexandru 2020-03-26 8:19 ` Ardelean, Alexandru 2020-03-26 8:22 ` Ardelean, Alexandru 2020-03-22 19:55 ` [PATCH v3 2/4] staging: iio: adc: ad7192: Correct macro names from SYNC to SINC Deepak R Varma 2020-03-22 23:40 ` Andy Shevchenko 2020-03-23 17:50 ` DEEPAK VARMA 2020-03-26 8:20 ` Ardelean, Alexandru 2020-03-22 19:56 ` [PATCH v3 3/4] staging: iio: adc: ad7192: get_filter_freq code optimization Deepak R Varma 2020-03-22 23:44 ` Andy Shevchenko 2020-03-23 0:49 ` [Outreachy kernel] " Stefano Brivio 2020-03-23 9:28 ` Andy Shevchenko 2020-03-23 12:15 ` Stefano Brivio 2020-03-23 17:52 ` DEEPAK VARMA 2020-03-24 8:06 ` Ardelean, Alexandru 2020-03-25 17:38 ` DEEPAK VARMA 2020-03-25 19:37 ` Andy Shevchenko 2020-03-25 19:59 ` Stefano Brivio 2020-03-26 8:15 ` Ardelean, Alexandru 2020-03-22 19:57 ` [PATCH v3 4/4] staging: iio: adc: ad7280a: Add comments to clarify stringified arguments Deepak R Varma 2020-03-24 8:07 ` Ardelean, Alexandru 2020-03-28 13:28 ` Jonathan Cameron
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).