* [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-22 12:14 ` Stefan Wahren
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Wahren @ 2014-12-22 12:14 UTC (permalink / raw)
To: jic23-DgEjT+Ai2ygdnm+yROfE0A
Cc: festevam-Re5JQEeQqe8AvxtiuMwx3w,
kristina.martsenko-Re5JQEeQqe8AvxtiuMwx3w, knaack.h-Mmb7MZpHnFY,
marex-ynQEQJNshbs, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, galak-sgV2jX0FEOL9JmXXK+q4OQ,
mark.rutland-5wv7dgnIgG8, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Stefan Wahren
The devicetree binding for mxs-lradc defines ranges for the
touchscreen properties. In order to avoid unexpected behavior like
division by zero, we better check these ranges during probe and
abort in error case.
Additionally this patch adds an important note from the reference
manual about the range of sample delay.
Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
---
drivers/staging/iio/adc/mxs-lradc.c | 44 +++++++++++++++++++++++++++--------
1 file changed, 34 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index f053535..990e945 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -436,7 +436,13 @@ static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
*/
mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch));
- /* prepare the delay/loop unit according to the oversampling count */
+ /* prepare the delay/loop unit according to the oversampling count
+ *
+ * from the datasheet:
+ * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
+ * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
+ * the LRADC will not trigger the delay group."
+ */
mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
LRADC_DELAY_TRIGGER_DELAYS(0) |
LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
@@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
return -EINVAL;
}
- lradc->over_sample_cnt = 4;
- ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
- if (ret == 0)
+ if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
+ lradc->over_sample_cnt = 4;
+ } else {
+ if (adapt < 1 || adapt > 32) {
+ dev_err(lradc->dev, "Invalid sample count (%lu)\n",
+ adapt);
+ return -EINVAL;
+ }
lradc->over_sample_cnt = adapt;
+ }
- lradc->over_sample_delay = 2;
- ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
- if (ret == 0)
+ if (of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt)) {
+ lradc->over_sample_delay = 2;
+ } else {
+ if (adapt < 2 || adapt > LRADC_DELAY_DELAY_MASK+1) {
+ dev_err(lradc->dev, "Invalid sample delay (%u)\n",
+ adapt);
+ return -EINVAL;
+ }
lradc->over_sample_delay = adapt;
+ }
- lradc->settling_delay = 10;
- ret = of_property_read_u32(lradc_node, "fsl,settling", &adapt);
- if (ret == 0)
+ if (of_property_read_u32(lradc_node, "fsl,settling", &adapt)) {
+ lradc->settling_delay = 10;
+ } else {
+ if (adapt < 1 || adapt > LRADC_DELAY_DELAY_MASK) {
+ dev_err(lradc->dev, "Invalid settling delay (%u)\n",
+ adapt);
+ return -EINVAL;
+ }
lradc->settling_delay = adapt;
+ }
return 0;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-23 13:37 ` Marek Vasut
0 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2014-12-23 13:37 UTC (permalink / raw)
To: Stefan Wahren
Cc: jic23, festevam, kristina.martsenko, knaack.h, kernel, pawel.moll,
ijc+devicetree, robh+dt, galak, mark.rutland, linux-iio,
devicetree
On Monday, December 22, 2014 at 01:14:36 PM, Stefan Wahren wrote:
> The devicetree binding for mxs-lradc defines ranges for the
> touchscreen properties. In order to avoid unexpected behavior like
> division by zero, we better check these ranges during probe and
> abort in error case.
>
> Additionally this patch adds an important note from the reference
> manual about the range of sample delay.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> drivers/staging/iio/adc/mxs-lradc.c | 44
> +++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 10
> deletions(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> b/drivers/staging/iio/adc/mxs-lradc.c index f053535..990e945 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -436,7 +436,13 @@ static void mxs_lradc_setup_ts_channel(struct
> mxs_lradc *lradc, unsigned ch) */
> mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch));
>
> - /* prepare the delay/loop unit according to the oversampling count */
> + /* prepare the delay/loop unit according to the oversampling count
Very minor coding style flub in this comment above. Multi-line comments should
start with /* and a newline after that ;-)
> + * from the datasheet:
> + * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
> + * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
> + * the LRADC will not trigger the delay group."
> + */
> mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
> LRADC_DELAY_TRIGGER_DELAYS(0) |
> LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> @@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct
> mxs_lradc *lradc, return -EINVAL;
> }
>
> - lradc->over_sample_cnt = 4;
> - ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
> - if (ret == 0)
> + if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
> + lradc->over_sample_cnt = 4;
> + } else {
> + if (adapt < 1 || adapt > 32) {
This is just an idea, but do we not have some kind of a
"of_property_read_u32_range()" thingie, which would include this kind of range
checking ? Would it be worth implementing such thing ? What do you think
please ?
[...]
Otherwise,
Reviewed-by: Marek Vasut <marex@denx.de>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-23 13:37 ` Marek Vasut
0 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2014-12-23 13:37 UTC (permalink / raw)
To: Stefan Wahren
Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A, festevam-Re5JQEeQqe8AvxtiuMwx3w,
kristina.martsenko-Re5JQEeQqe8AvxtiuMwx3w, knaack.h-Mmb7MZpHnFY,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, pawel.moll-5wv7dgnIgG8,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, galak-sgV2jX0FEOL9JmXXK+q4OQ,
mark.rutland-5wv7dgnIgG8, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On Monday, December 22, 2014 at 01:14:36 PM, Stefan Wahren wrote:
> The devicetree binding for mxs-lradc defines ranges for the
> touchscreen properties. In order to avoid unexpected behavior like
> division by zero, we better check these ranges during probe and
> abort in error case.
>
> Additionally this patch adds an important note from the reference
> manual about the range of sample delay.
>
> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> ---
> drivers/staging/iio/adc/mxs-lradc.c | 44
> +++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 10
> deletions(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> b/drivers/staging/iio/adc/mxs-lradc.c index f053535..990e945 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -436,7 +436,13 @@ static void mxs_lradc_setup_ts_channel(struct
> mxs_lradc *lradc, unsigned ch) */
> mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch));
>
> - /* prepare the delay/loop unit according to the oversampling count */
> + /* prepare the delay/loop unit according to the oversampling count
Very minor coding style flub in this comment above. Multi-line comments should
start with /* and a newline after that ;-)
> + * from the datasheet:
> + * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
> + * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
> + * the LRADC will not trigger the delay group."
> + */
> mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
> LRADC_DELAY_TRIGGER_DELAYS(0) |
> LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> @@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct
> mxs_lradc *lradc, return -EINVAL;
> }
>
> - lradc->over_sample_cnt = 4;
> - ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
> - if (ret == 0)
> + if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
> + lradc->over_sample_cnt = 4;
> + } else {
> + if (adapt < 1 || adapt > 32) {
This is just an idea, but do we not have some kind of a
"of_property_read_u32_range()" thingie, which would include this kind of range
checking ? Would it be worth implementing such thing ? What do you think
please ?
[...]
Otherwise,
Reviewed-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-23 22:45 ` Stefan Wahren
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Wahren @ 2014-12-23 22:45 UTC (permalink / raw)
To: Marek Vasut
Cc: kristina.martsenko, kernel, jic23, pawel.moll, knaack.h,
ijc+devicetree, robh+dt, galak, festevam, mark.rutland, linux-iio,
devicetree
Hi Marek,
> Marek Vasut <marex@denx.de> hat am 23. Dezember 2014 um 14:37 geschrieben:
>
>
> On Monday, December 22, 2014 at 01:14:36 PM, Stefan Wahren wrote:
> [...]
>
> Very minor coding style flub in this comment above. Multi-line comments should
> start with /* and a newline after that ;-)
Thanks for your advice.
>
> > + * from the datasheet:
> > + * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
> > + * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
> > + * the LRADC will not trigger the delay group."
> > + */
> > mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
> > LRADC_DELAY_TRIGGER_DELAYS(0) |
> > LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> > @@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct
> > mxs_lradc *lradc, return -EINVAL;
> > }
> >
> > - lradc->over_sample_cnt = 4;
> > - ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
> > - if (ret == 0)
> > + if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
> > + lradc->over_sample_cnt = 4;
> > + } else {
> > + if (adapt < 1 || adapt > 32) {
>
> This is just an idea, but do we not have some kind of a
> "of_property_read_u32_range()" thingie, which would include this kind of range
> checking ? Would it be worth implementing such thing ? What do you think
> please ?
I never heard of such a function. I think it's not the best idea of mixing dt
parsing and range checking in a general function.
But this code does nearly the same thing 3 times. How about defining an array of
property range structures:
static const struct property_value_range mxs_lradc_properties[] = {
{
.name = "fsl,ave-ctrl",
.min_value = 1,
.max_value = 32,
.default_value = 4,
},
{
.name = "fsl,ave-delay",
.min_value = 2,
.max_value = LRADC_DELAY_DELAY_MASK+1,
.default_value = 2,
},
{
.name = "fsl,settling",
.min_value = 1,
.max_value = LRADC_DELAY_DELAY_MASK,
.default_value = 10,
},
};
and a local validate function for these optional parameters.
>
> [...]
>
> Otherwise,
>
> Reviewed-by: Marek Vasut <marex@denx.de>
Thanks
Btw i found an error in the format strings :(
Stefan
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-23 22:45 ` Stefan Wahren
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Wahren @ 2014-12-23 22:45 UTC (permalink / raw)
To: Marek Vasut
Cc: kristina.martsenko-Re5JQEeQqe8AvxtiuMwx3w,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, jic23-DgEjT+Ai2ygdnm+yROfE0A,
pawel.moll-5wv7dgnIgG8, knaack.h-Mmb7MZpHnFY,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, galak-sgV2jX0FEOL9JmXXK+q4OQ,
festevam-Re5JQEeQqe8AvxtiuMwx3w, mark.rutland-5wv7dgnIgG8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Hi Marek,
> Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> hat am 23. Dezember 2014 um 14:37 geschrieben:
>
>
> On Monday, December 22, 2014 at 01:14:36 PM, Stefan Wahren wrote:
> [...]
>
> Very minor coding style flub in this comment above. Multi-line comments should
> start with /* and a newline after that ;-)
Thanks for your advice.
>
> > + * from the datasheet:
> > + * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
> > + * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
> > + * the LRADC will not trigger the delay group."
> > + */
> > mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
> > LRADC_DELAY_TRIGGER_DELAYS(0) |
> > LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> > @@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct
> > mxs_lradc *lradc, return -EINVAL;
> > }
> >
> > - lradc->over_sample_cnt = 4;
> > - ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
> > - if (ret == 0)
> > + if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
> > + lradc->over_sample_cnt = 4;
> > + } else {
> > + if (adapt < 1 || adapt > 32) {
>
> This is just an idea, but do we not have some kind of a
> "of_property_read_u32_range()" thingie, which would include this kind of range
> checking ? Would it be worth implementing such thing ? What do you think
> please ?
I never heard of such a function. I think it's not the best idea of mixing dt
parsing and range checking in a general function.
But this code does nearly the same thing 3 times. How about defining an array of
property range structures:
static const struct property_value_range mxs_lradc_properties[] = {
{
.name = "fsl,ave-ctrl",
.min_value = 1,
.max_value = 32,
.default_value = 4,
},
{
.name = "fsl,ave-delay",
.min_value = 2,
.max_value = LRADC_DELAY_DELAY_MASK+1,
.default_value = 2,
},
{
.name = "fsl,settling",
.min_value = 1,
.max_value = LRADC_DELAY_DELAY_MASK,
.default_value = 10,
},
};
and a local validate function for these optional parameters.
>
> [...]
>
> Otherwise,
>
> Reviewed-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Thanks
Btw i found an error in the format strings :(
Stefan
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-24 0:35 ` Marek Vasut
0 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2014-12-24 0:35 UTC (permalink / raw)
To: Stefan Wahren
Cc: kristina.martsenko, kernel, jic23, pawel.moll, knaack.h,
ijc+devicetree, robh+dt, galak, festevam, mark.rutland, linux-iio,
devicetree
On Tuesday, December 23, 2014 at 11:45:59 PM, Stefan Wahren wrote:
> Hi Marek,
Hi!
> > Marek Vasut <marex@denx.de> hat am 23. Dezember 2014 um 14:37
> > geschrieben:
> >
> >
> > On Monday, December 22, 2014 at 01:14:36 PM, Stefan Wahren wrote:
> > [...]
> >
> > Very minor coding style flub in this comment above. Multi-line comments
> > should start with /* and a newline after that ;-)
>
> Thanks for your advice.
Sure, it's really a minor thing.
> > > + * from the datasheet:
> > > + * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
> > > + * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
> > > + * the LRADC will not trigger the delay group."
> > > + */
> > > mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
> > > LRADC_DELAY_TRIGGER_DELAYS(0) |
> > > LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> > > @@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct
> > > mxs_lradc *lradc, return -EINVAL;
> > > }
> > >
> > > - lradc->over_sample_cnt = 4;
> > > - ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
> > > - if (ret == 0)
> > > + if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
> > > + lradc->over_sample_cnt = 4;
> > > + } else {
> > > + if (adapt < 1 || adapt > 32) {
> >
> > This is just an idea, but do we not have some kind of a
> > "of_property_read_u32_range()" thingie, which would include this kind of
> > range checking ? Would it be worth implementing such thing ? What do you
> > think please ?
>
> I never heard of such a function. I think it's not the best idea of mixing
> dt parsing and range checking in a general function.
It was just an idea, since it would trim down the code duplication a bit.
> But this code does nearly the same thing 3 times. How about defining an
> array of property range structures:
>
> static const struct property_value_range mxs_lradc_properties[] = {
> {
> .name = "fsl,ave-ctrl",
> .min_value = 1,
> .max_value = 32,
> .default_value = 4,
> },
> {
> .name = "fsl,ave-delay",
> .min_value = 2,
> .max_value = LRADC_DELAY_DELAY_MASK+1,
> .default_value = 2,
> },
> {
> .name = "fsl,settling",
> .min_value = 1,
> .max_value = LRADC_DELAY_DELAY_MASK,
> .default_value = 10,
> },
> };
>
> and a local validate function for these optional parameters.
That's becoming a bit too complex for such a simple task. I cannot tell right
now, so I'd prefer of others chimed in.
Have a nice holiday!
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-24 0:35 ` Marek Vasut
0 siblings, 0 replies; 20+ messages in thread
From: Marek Vasut @ 2014-12-24 0:35 UTC (permalink / raw)
To: Stefan Wahren
Cc: kristina.martsenko-Re5JQEeQqe8AvxtiuMwx3w,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, jic23-DgEjT+Ai2ygdnm+yROfE0A,
pawel.moll-5wv7dgnIgG8, knaack.h-Mmb7MZpHnFY,
ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, galak-sgV2jX0FEOL9JmXXK+q4OQ,
festevam-Re5JQEeQqe8AvxtiuMwx3w, mark.rutland-5wv7dgnIgG8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On Tuesday, December 23, 2014 at 11:45:59 PM, Stefan Wahren wrote:
> Hi Marek,
Hi!
> > Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> hat am 23. Dezember 2014 um 14:37
> > geschrieben:
> >
> >
> > On Monday, December 22, 2014 at 01:14:36 PM, Stefan Wahren wrote:
> > [...]
> >
> > Very minor coding style flub in this comment above. Multi-line comments
> > should start with /* and a newline after that ;-)
>
> Thanks for your advice.
Sure, it's really a minor thing.
> > > + * from the datasheet:
> > > + * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
> > > + * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
> > > + * the LRADC will not trigger the delay group."
> > > + */
> > > mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
> > > LRADC_DELAY_TRIGGER_DELAYS(0) |
> > > LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> > > @@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct
> > > mxs_lradc *lradc, return -EINVAL;
> > > }
> > >
> > > - lradc->over_sample_cnt = 4;
> > > - ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
> > > - if (ret == 0)
> > > + if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
> > > + lradc->over_sample_cnt = 4;
> > > + } else {
> > > + if (adapt < 1 || adapt > 32) {
> >
> > This is just an idea, but do we not have some kind of a
> > "of_property_read_u32_range()" thingie, which would include this kind of
> > range checking ? Would it be worth implementing such thing ? What do you
> > think please ?
>
> I never heard of such a function. I think it's not the best idea of mixing
> dt parsing and range checking in a general function.
It was just an idea, since it would trim down the code duplication a bit.
> But this code does nearly the same thing 3 times. How about defining an
> array of property range structures:
>
> static const struct property_value_range mxs_lradc_properties[] = {
> {
> .name = "fsl,ave-ctrl",
> .min_value = 1,
> .max_value = 32,
> .default_value = 4,
> },
> {
> .name = "fsl,ave-delay",
> .min_value = 2,
> .max_value = LRADC_DELAY_DELAY_MASK+1,
> .default_value = 2,
> },
> {
> .name = "fsl,settling",
> .min_value = 1,
> .max_value = LRADC_DELAY_DELAY_MASK,
> .default_value = 10,
> },
> };
>
> and a local validate function for these optional parameters.
That's becoming a bit too complex for such a simple task. I cannot tell right
now, so I'd prefer of others chimed in.
Have a nice holiday!
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-26 9:22 ` Jonathan Cameron
0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2014-12-26 9:22 UTC (permalink / raw)
To: Marek Vasut, Stefan Wahren
Cc: kristina.martsenko, kernel, pawel.moll, knaack.h, ijc+devicetree,
robh+dt, galak, festevam, mark.rutland, linux-iio, devicetree
On 24/12/14 00:35, Marek Vasut wrote:
> On Tuesday, December 23, 2014 at 11:45:59 PM, Stefan Wahren wrote:
>> Hi Marek,
>
> Hi!
>
>>> Marek Vasut <marex@denx.de> hat am 23. Dezember 2014 um 14:37
>>> geschrieben:
>>>
>>>
>>> On Monday, December 22, 2014 at 01:14:36 PM, Stefan Wahren wrote:
>>> [...]
>>>
>>> Very minor coding style flub in this comment above. Multi-line comments
>>> should start with /* and a newline after that ;-)
>>
>> Thanks for your advice.
>
> Sure, it's really a minor thing.
>
>>>> + * from the datasheet:
>>>> + * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
>>>> + * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
>>>> + * the LRADC will not trigger the delay group."
>>>> + */
>>>> mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
>>>> LRADC_DELAY_TRIGGER_DELAYS(0) |
>>>> LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
>>>> @@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct
>>>> mxs_lradc *lradc, return -EINVAL;
>>>> }
>>>>
>>>> - lradc->over_sample_cnt = 4;
>>>> - ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
>>>> - if (ret == 0)
>>>> + if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
>>>> + lradc->over_sample_cnt = 4;
>>>> + } else {
>>>> + if (adapt < 1 || adapt > 32) {
>>>
>>> This is just an idea, but do we not have some kind of a
>>> "of_property_read_u32_range()" thingie, which would include this kind of
>>> range checking ? Would it be worth implementing such thing ? What do you
>>> think please ?
>>
>> I never heard of such a function. I think it's not the best idea of mixing
>> dt parsing and range checking in a general function.
>
> It was just an idea, since it would trim down the code duplication a bit.
It's an interesting thought certainly as this must be a fairly common case...
>
>> But this code does nearly the same thing 3 times. How about defining an
>> array of property range structures:
>>
>> static const struct property_value_range mxs_lradc_properties[] = {
>> {
>> .name = "fsl,ave-ctrl",
>> .min_value = 1,
>> .max_value = 32,
>> .default_value = 4,
>> },
>> {
>> .name = "fsl,ave-delay",
>> .min_value = 2,
>> .max_value = LRADC_DELAY_DELAY_MASK+1,
>> .default_value = 2,
>> },
>> {
>> .name = "fsl,settling",
>> .min_value = 1,
>> .max_value = LRADC_DELAY_DELAY_MASK,
>> .default_value = 10,
>> },
>> };
>>
>> and a local validate function for these optional parameters.
>
> That's becoming a bit too complex for such a simple task. I cannot tell right
> now, so I'd prefer of others chimed in.
I'd leave as it is. If there were a few more cases it might be worth the
function, but probably not for these 3.
A shorter option might be to just use a function with the constants passed
in as parameters for each call.
>
> Have a nice holiday!
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-26 9:22 ` Jonathan Cameron
0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2014-12-26 9:22 UTC (permalink / raw)
To: Marek Vasut, Stefan Wahren
Cc: kristina.martsenko-Re5JQEeQqe8AvxtiuMwx3w,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, pawel.moll-5wv7dgnIgG8,
knaack.h-Mmb7MZpHnFY, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, galak-sgV2jX0FEOL9JmXXK+q4OQ,
festevam-Re5JQEeQqe8AvxtiuMwx3w, mark.rutland-5wv7dgnIgG8,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 24/12/14 00:35, Marek Vasut wrote:
> On Tuesday, December 23, 2014 at 11:45:59 PM, Stefan Wahren wrote:
>> Hi Marek,
>
> Hi!
>
>>> Marek Vasut <marex-ynQEQJNshbs@public.gmane.org> hat am 23. Dezember 2014 um 14:37
>>> geschrieben:
>>>
>>>
>>> On Monday, December 22, 2014 at 01:14:36 PM, Stefan Wahren wrote:
>>> [...]
>>>
>>> Very minor coding style flub in this comment above. Multi-line comments
>>> should start with /* and a newline after that ;-)
>>
>> Thanks for your advice.
>
> Sure, it's really a minor thing.
>
>>>> + * from the datasheet:
>>>> + * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
>>>> + * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
>>>> + * the LRADC will not trigger the delay group."
>>>> + */
>>>> mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
>>>> LRADC_DELAY_TRIGGER_DELAYS(0) |
>>>> LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
>>>> @@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct
>>>> mxs_lradc *lradc, return -EINVAL;
>>>> }
>>>>
>>>> - lradc->over_sample_cnt = 4;
>>>> - ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
>>>> - if (ret == 0)
>>>> + if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
>>>> + lradc->over_sample_cnt = 4;
>>>> + } else {
>>>> + if (adapt < 1 || adapt > 32) {
>>>
>>> This is just an idea, but do we not have some kind of a
>>> "of_property_read_u32_range()" thingie, which would include this kind of
>>> range checking ? Would it be worth implementing such thing ? What do you
>>> think please ?
>>
>> I never heard of such a function. I think it's not the best idea of mixing
>> dt parsing and range checking in a general function.
>
> It was just an idea, since it would trim down the code duplication a bit.
It's an interesting thought certainly as this must be a fairly common case...
>
>> But this code does nearly the same thing 3 times. How about defining an
>> array of property range structures:
>>
>> static const struct property_value_range mxs_lradc_properties[] = {
>> {
>> .name = "fsl,ave-ctrl",
>> .min_value = 1,
>> .max_value = 32,
>> .default_value = 4,
>> },
>> {
>> .name = "fsl,ave-delay",
>> .min_value = 2,
>> .max_value = LRADC_DELAY_DELAY_MASK+1,
>> .default_value = 2,
>> },
>> {
>> .name = "fsl,settling",
>> .min_value = 1,
>> .max_value = LRADC_DELAY_DELAY_MASK,
>> .default_value = 10,
>> },
>> };
>>
>> and a local validate function for these optional parameters.
>
> That's becoming a bit too complex for such a simple task. I cannot tell right
> now, so I'd prefer of others chimed in.
I'd leave as it is. If there were a few more cases it might be worth the
function, but probably not for these 3.
A shorter option might be to just use a function with the constants passed
in as parameters for each call.
>
> Have a nice holiday!
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-26 9:23 ` Jonathan Cameron
0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2014-12-26 9:23 UTC (permalink / raw)
To: Stefan Wahren
Cc: festevam, kristina.martsenko, knaack.h, marex, kernel, pawel.moll,
ijc+devicetree, robh+dt, galak, mark.rutland, linux-iio,
devicetree
On 22/12/14 12:14, Stefan Wahren wrote:
> The devicetree binding for mxs-lradc defines ranges for the
> touchscreen properties. In order to avoid unexpected behavior like
> division by zero, we better check these ranges during probe and
> abort in error case.
>
> Additionally this patch adds an important note from the reference
> manual about the range of sample delay.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> drivers/staging/iio/adc/mxs-lradc.c | 44 +++++++++++++++++++++++++++--------
> 1 file changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index f053535..990e945 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -436,7 +436,13 @@ static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
> */
> mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch));
>
> - /* prepare the delay/loop unit according to the oversampling count */
> + /* prepare the delay/loop unit according to the oversampling count
> + *
> + * from the datasheet:
> + * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
> + * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
> + * the LRADC will not trigger the delay group."
> + */
> mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
> LRADC_DELAY_TRIGGER_DELAYS(0) |
> LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> @@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
> return -EINVAL;
> }
>
> - lradc->over_sample_cnt = 4;
> - ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
> - if (ret == 0)
> + if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
> + lradc->over_sample_cnt = 4;
> + } else {
> + if (adapt < 1 || adapt > 32) {
> + dev_err(lradc->dev, "Invalid sample count (%lu)\n",
> + adapt);
> + return -EINVAL;
> + }
> lradc->over_sample_cnt = adapt;
> + }
>
> - lradc->over_sample_delay = 2;
> - ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
> - if (ret == 0)
> + if (of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt)) {
> + lradc->over_sample_delay = 2;
> + } else {
> + if (adapt < 2 || adapt > LRADC_DELAY_DELAY_MASK+1) {
please run checkpatch.pl over these. Should be spaces around the +
> + dev_err(lradc->dev, "Invalid sample delay (%u)\n",
> + adapt);
> + return -EINVAL;
> + }
> lradc->over_sample_delay = adapt;
> + }
>
> - lradc->settling_delay = 10;
> - ret = of_property_read_u32(lradc_node, "fsl,settling", &adapt);
> - if (ret == 0)
> + if (of_property_read_u32(lradc_node, "fsl,settling", &adapt)) {
> + lradc->settling_delay = 10;
> + } else {
> + if (adapt < 1 || adapt > LRADC_DELAY_DELAY_MASK) {
> + dev_err(lradc->dev, "Invalid settling delay (%u)\n",
> + adapt);
> + return -EINVAL;
> + }
> lradc->settling_delay = adapt;
> + }
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-26 9:23 ` Jonathan Cameron
0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2014-12-26 9:23 UTC (permalink / raw)
To: Stefan Wahren
Cc: festevam-Re5JQEeQqe8AvxtiuMwx3w,
kristina.martsenko-Re5JQEeQqe8AvxtiuMwx3w, knaack.h-Mmb7MZpHnFY,
marex-ynQEQJNshbs, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
pawel.moll-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, galak-sgV2jX0FEOL9JmXXK+q4OQ,
mark.rutland-5wv7dgnIgG8, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 22/12/14 12:14, Stefan Wahren wrote:
> The devicetree binding for mxs-lradc defines ranges for the
> touchscreen properties. In order to avoid unexpected behavior like
> division by zero, we better check these ranges during probe and
> abort in error case.
>
> Additionally this patch adds an important note from the reference
> manual about the range of sample delay.
>
> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> ---
> drivers/staging/iio/adc/mxs-lradc.c | 44 +++++++++++++++++++++++++++--------
> 1 file changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index f053535..990e945 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -436,7 +436,13 @@ static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
> */
> mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch));
>
> - /* prepare the delay/loop unit according to the oversampling count */
> + /* prepare the delay/loop unit according to the oversampling count
> + *
> + * from the datasheet:
> + * "The DELAY fields in HW_LRADC_DELAY0, HW_LRADC_DELAY1,
> + * HW_LRADC_DELAY2, and HW_LRADC_DELAY3 must be non-zero; otherwise,
> + * the LRADC will not trigger the delay group."
> + */
> mxs_lradc_reg_wrt(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
> LRADC_DELAY_TRIGGER_DELAYS(0) |
> LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> @@ -1495,20 +1501,38 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
> return -EINVAL;
> }
>
> - lradc->over_sample_cnt = 4;
> - ret = of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt);
> - if (ret == 0)
> + if (of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
> + lradc->over_sample_cnt = 4;
> + } else {
> + if (adapt < 1 || adapt > 32) {
> + dev_err(lradc->dev, "Invalid sample count (%lu)\n",
> + adapt);
> + return -EINVAL;
> + }
> lradc->over_sample_cnt = adapt;
> + }
>
> - lradc->over_sample_delay = 2;
> - ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
> - if (ret == 0)
> + if (of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt)) {
> + lradc->over_sample_delay = 2;
> + } else {
> + if (adapt < 2 || adapt > LRADC_DELAY_DELAY_MASK+1) {
please run checkpatch.pl over these. Should be spaces around the +
> + dev_err(lradc->dev, "Invalid sample delay (%u)\n",
> + adapt);
> + return -EINVAL;
> + }
> lradc->over_sample_delay = adapt;
> + }
>
> - lradc->settling_delay = 10;
> - ret = of_property_read_u32(lradc_node, "fsl,settling", &adapt);
> - if (ret == 0)
> + if (of_property_read_u32(lradc_node, "fsl,settling", &adapt)) {
> + lradc->settling_delay = 10;
> + } else {
> + if (adapt < 1 || adapt > LRADC_DELAY_DELAY_MASK) {
> + dev_err(lradc->dev, "Invalid settling delay (%u)\n",
> + adapt);
> + return -EINVAL;
> + }
> lradc->settling_delay = adapt;
> + }
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-26 20:50 ` Stefan Wahren
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Wahren @ 2014-12-26 20:50 UTC (permalink / raw)
To: Jonathan Cameron
Cc: kristina.martsenko, kernel, pawel.moll, knaack.h, ijc+devicetree,
robh+dt, galak, festevam, mark.rutland, linux-iio, marex,
devicetree
Hi Jonathan,
> Jonathan Cameron <jic23@kernel.org> hat am 26. Dezember 2014 um 10:23
> geschrieben:
>
> [...]
> >
> > - lradc->over_sample_delay = 2;
> > - ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
> > - if (ret == 0)
> > + if (of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt)) {
> > + lradc->over_sample_delay = 2;
> > + } else {
> > + if (adapt < 2 || adapt > LRADC_DELAY_DELAY_MASK+1) {
> please run checkpatch.pl over these. Should be spaces around the +
i'm afraid my checkpatch.pl doesn't find this issue. I'll fix it in the next
version.
Stefan
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-26 20:50 ` Stefan Wahren
0 siblings, 0 replies; 20+ messages in thread
From: Stefan Wahren @ 2014-12-26 20:50 UTC (permalink / raw)
To: Jonathan Cameron
Cc: kristina.martsenko-Re5JQEeQqe8AvxtiuMwx3w,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, pawel.moll-5wv7dgnIgG8,
knaack.h-Mmb7MZpHnFY, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, galak-sgV2jX0FEOL9JmXXK+q4OQ,
festevam-Re5JQEeQqe8AvxtiuMwx3w, mark.rutland-5wv7dgnIgG8,
linux-iio-u79uwXL29TY76Z2rM5mHXA, marex-ynQEQJNshbs,
devicetree-u79uwXL29TY76Z2rM5mHXA
Hi Jonathan,
> Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> hat am 26. Dezember 2014 um 10:23
> geschrieben:
>
> [...]
> >
> > - lradc->over_sample_delay = 2;
> > - ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
> > - if (ret == 0)
> > + if (of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt)) {
> > + lradc->over_sample_delay = 2;
> > + } else {
> > + if (adapt < 2 || adapt > LRADC_DELAY_DELAY_MASK+1) {
> please run checkpatch.pl over these. Should be spaces around the +
i'm afraid my checkpatch.pl doesn't find this issue. I'll fix it in the next
version.
Stefan
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-26 21:57 ` Jonathan Cameron
0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2014-12-26 21:57 UTC (permalink / raw)
To: Stefan Wahren
Cc: kristina.martsenko, kernel, pawel.moll, knaack.h, ijc+devicetree,
robh+dt, galak, festevam, mark.rutland, linux-iio, marex,
devicetree
On 26/12/14 20:50, Stefan Wahren wrote:
> Hi Jonathan,
>
>> Jonathan Cameron <jic23@kernel.org> hat am 26. Dezember 2014 um 10:23
>> geschrieben:
>>
>> [...]
>>>
>>> - lradc->over_sample_delay = 2;
>>> - ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
>>> - if (ret == 0)
>>> + if (of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt)) {
>>> + lradc->over_sample_delay = 2;
>>> + } else {
>>> + if (adapt < 2 || adapt > LRADC_DELAY_DELAY_MASK+1) {
>> please run checkpatch.pl over these. Should be spaces around the +
>
> i'm afraid my checkpatch.pl doesn't find this issue. I'll fix it in the next
> version.
>
So it doesn't! Sorry about the false comment.
Hmm. I wonder why...
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
@ 2014-12-26 21:57 ` Jonathan Cameron
0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Cameron @ 2014-12-26 21:57 UTC (permalink / raw)
To: Stefan Wahren
Cc: kristina.martsenko-Re5JQEeQqe8AvxtiuMwx3w,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, pawel.moll-5wv7dgnIgG8,
knaack.h-Mmb7MZpHnFY, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, galak-sgV2jX0FEOL9JmXXK+q4OQ,
festevam-Re5JQEeQqe8AvxtiuMwx3w, mark.rutland-5wv7dgnIgG8,
linux-iio-u79uwXL29TY76Z2rM5mHXA, marex-ynQEQJNshbs,
devicetree-u79uwXL29TY76Z2rM5mHXA
On 26/12/14 20:50, Stefan Wahren wrote:
> Hi Jonathan,
>
>> Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> hat am 26. Dezember 2014 um 10:23
>> geschrieben:
>>
>> [...]
>>>
>>> - lradc->over_sample_delay = 2;
>>> - ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
>>> - if (ret == 0)
>>> + if (of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt)) {
>>> + lradc->over_sample_delay = 2;
>>> + } else {
>>> + if (adapt < 2 || adapt > LRADC_DELAY_DELAY_MASK+1) {
>> please run checkpatch.pl over these. Should be spaces around the +
>
> i'm afraid my checkpatch.pl doesn't find this issue. I'll fix it in the next
> version.
>
So it doesn't! Sorry about the false comment.
Hmm. I wonder why...
^ permalink raw reply [flat|nested] 20+ messages in thread