[PATCH] iio: mxs-lradc: check ranges of ts properties

[PATCH] iio: mxs-lradc: check ranges of ts properties

[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.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/staging/iio/adc/mxs-lradc.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 6757f10..57c3cf6 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -1500,16 +1500,36 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
 	if (ret == 0)
 		lradc->over_sample_cnt = adapt;

+	if (!lradc->over_sample_cnt || lradc->over_sample_cnt > 0x1f) {
+		dev_err(lradc->dev, "Invalid sample count (%u)\n",
+				    lradc->over_sample_cnt);
+		return -EINVAL;
+	}
+
 	lradc->over_sample_delay = 2;
 	ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
 	if (ret == 0)
 		lradc->over_sample_delay = adapt;

+	if (!lradc->over_sample_delay ||
+	    lradc->over_sample_delay > LRADC_DELAY_DELAY_MASK) {
+		dev_err(lradc->dev, "Invalid sample delay (%u)\n",
+				    lradc->over_sample_delay);
+		return -EINVAL;
+	}
+
 	lradc->settling_delay = 10;
 	ret = of_property_read_u32(lradc_node, "fsl,settling", &adapt);
 	if (ret == 0)
 		lradc->settling_delay = adapt;

+	if (!lradc->settling_delay ||
+	    lradc->settling_delay > LRADC_DELAY_DELAY_MASK) {
+		dev_err(lradc->dev, "Invalid settling delay (%u)\n",
+				    lradc->settling_delay);
+		return -EINVAL;
+	}
+
 	return 0;
 }

--
1.7.9.5

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Fabio Estevam]

[Adding Marek]

On Wed, Nov 19, 2014 at 8:19 PM, Stefan Wahren <stefan.wahren@i2se.com> 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.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  drivers/staging/iio/adc/mxs-lradc.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index 6757f10..57c3cf6 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -1500,16 +1500,36 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
>         if (ret == 0)
>                 lradc->over_sample_cnt = adapt;
>
> +       if (!lradc->over_sample_cnt || lradc->over_sample_cnt > 0x1f) {
> +               dev_err(lradc->dev, "Invalid sample count (%u)\n",
> +                                   lradc->over_sample_cnt);
> +               return -EINVAL;
> +       }
> +
>         lradc->over_sample_delay = 2;
>         ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
>         if (ret == 0)
>                 lradc->over_sample_delay = adapt;
>
> +       if (!lradc->over_sample_delay ||
> +           lradc->over_sample_delay > LRADC_DELAY_DELAY_MASK) {
> +               dev_err(lradc->dev, "Invalid sample delay (%u)\n",
> +                                   lradc->over_sample_delay);
> +               return -EINVAL;
> +       }
> +
>         lradc->settling_delay = 10;
>         ret = of_property_read_u32(lradc_node, "fsl,settling", &adapt);
>         if (ret == 0)
>                 lradc->settling_delay = adapt;
>
> +       if (!lradc->settling_delay ||
> +           lradc->settling_delay > LRADC_DELAY_DELAY_MASK) {
> +               dev_err(lradc->dev, "Invalid settling delay (%u)\n",
> +                                   lradc->settling_delay);
> +               return -EINVAL;
> +       }
> +

Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Jonathan Cameron]

On 19/11/14 22:42, Fabio Estevam wrote:
> [Adding Marek]
It will sit in my testing branch for a day or two if Marek still
wants to take a look and comment...
> 
> On Wed, Nov 19, 2014 at 8:19 PM, Stefan Wahren <stefan.wahren@i2se.com> 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.
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Applied to the togreg branch of iio.git - initially pushed out as testing for the
autobuilders to play with it.

Thanks,

Jonathan
>> ---
>>  drivers/staging/iio/adc/mxs-lradc.c |   20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
>> index 6757f10..57c3cf6 100644
>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>> @@ -1500,16 +1500,36 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
>>         if (ret == 0)
>>                 lradc->over_sample_cnt = adapt;
>>
>> +       if (!lradc->over_sample_cnt || lradc->over_sample_cnt > 0x1f) {
>> +               dev_err(lradc->dev, "Invalid sample count (%u)\n",
>> +                                   lradc->over_sample_cnt);
>> +               return -EINVAL;
>> +       }
>> +
>>         lradc->over_sample_delay = 2;
>>         ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
>>         if (ret == 0)
>>                 lradc->over_sample_delay = adapt;
>>
>> +       if (!lradc->over_sample_delay ||
>> +           lradc->over_sample_delay > LRADC_DELAY_DELAY_MASK) {
>> +               dev_err(lradc->dev, "Invalid sample delay (%u)\n",
>> +                                   lradc->over_sample_delay);
>> +               return -EINVAL;
>> +       }
>> +
>>         lradc->settling_delay = 10;
>>         ret = of_property_read_u32(lradc_node, "fsl,settling", &adapt);
>>         if (ret == 0)
>>                 lradc->settling_delay = adapt;
>>
>> +       if (!lradc->settling_delay ||
>> +           lradc->settling_delay > LRADC_DELAY_DELAY_MASK) {
>> +               dev_err(lradc->dev, "Invalid settling delay (%u)\n",
>> +                                   lradc->settling_delay);
>> +               return -EINVAL;
>> +       }
>> +
> 
> Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Hartmut Knaack]

Fabio Estevam schrieb am 19.11.2014 um 23:42:
> [Adding Marek]
Taking a closer look on how these values are used, I wondered what the real value range of the registers actually are. So, anyone with access to the data sheets, please confirm.
Starting with over_sample_cnt, which according to the DT bindings has a range of 1...31. In mxs_lradc_setup_ts_channel(), currently line 429, the value decreased by one (0...30) gets written to register 0x50 (+ 0x10 for each channel) to bits 24-29. Question: What is the right value range there, 0...30 or 0...31?
In the same function, line 440, the value decreased by one (0...30) is written to register 0x100 into bits 11-15. Same question here: What is the right value range?
The same behavior can be found in mxs_lradc_setup_ts_pressure() in lines 485 and 498.
For over_sample_delay, the DT bindings state a range of 1...2047. In mxs_lradc_setup_ts_channel(), line 440, the value decreased by one (0...2046) is written to register 0x100, bits 0-10. Question: which value range is valid there? The same happens in line 498.
For settling_delay, the DT bindings state a range of 1...2047. In mxs_lradc_setup_ts_channel(), line 458, that value is written to register 0xf0, bits 0-10. Question: what value range is valid here, 1...2047 or 0...2047? The same happens in line 517.
Thanks,

Hartmut
> 
> On Wed, Nov 19, 2014 at 8:19 PM, Stefan Wahren <stefan.wahren@i2se.com> 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.
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>> ---
>>  drivers/staging/iio/adc/mxs-lradc.c |   20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
>> index 6757f10..57c3cf6 100644
>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>> @@ -1500,16 +1500,36 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
>>         if (ret == 0)
>>                 lradc->over_sample_cnt = adapt;
>>
>> +       if (!lradc->over_sample_cnt || lradc->over_sample_cnt > 0x1f) {
>> +               dev_err(lradc->dev, "Invalid sample count (%u)\n",
>> +                                   lradc->over_sample_cnt);
>> +               return -EINVAL;
>> +       }
>> +
>>         lradc->over_sample_delay = 2;
>>         ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
>>         if (ret == 0)
>>                 lradc->over_sample_delay = adapt;
>>
>> +       if (!lradc->over_sample_delay ||
>> +           lradc->over_sample_delay > LRADC_DELAY_DELAY_MASK) {
>> +               dev_err(lradc->dev, "Invalid sample delay (%u)\n",
>> +                                   lradc->over_sample_delay);
>> +               return -EINVAL;
>> +       }
>> +
>>         lradc->settling_delay = 10;
>>         ret = of_property_read_u32(lradc_node, "fsl,settling", &adapt);
>>         if (ret == 0)
>>                 lradc->settling_delay = adapt;
>>
>> +       if (!lradc->settling_delay ||
>> +           lradc->settling_delay > LRADC_DELAY_DELAY_MASK) {
>> +               dev_err(lradc->dev, "Invalid settling delay (%u)\n",
>> +                                   lradc->settling_delay);
>> +               return -EINVAL;
>> +       }
>> +
> 
> Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Stefan Wahren]

Hi Hartmut,

> Hartmut Knaack <knaack.h@gmx.de> hat am 29. November 2014 um 00:28
> geschrieben:
>
>
> Fabio Estevam schrieb am 19.11.2014 um 23:42:
> > [Adding Marek]
> Taking a closer look on how these values are used, I wondered what the real
> value range of the registers actually are. So, anyone with access to the data
> sheets, please confirm.

the reference manual is public:

http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf

> Starting with over_sample_cnt, which according to the DT bindings has a range
> of 1...31. In mxs_lradc_setup_ts_channel(), currently line 429, the value
> decreased by one (0...30) gets written to register 0x50 (+ 0x10 for each
> channel) to bits 24-29. Question: What is the right value range there, 0...30
> or 0...31?

Yes, looks like an off-by-one issue. The register range is 0..31 so i would
assume the DT property range should be logically 1..32. But i don't have checked
the reference manual about that.

> In the same function, line 440, the value decreased by one (0...30) is written
> to register 0x100 into bits 11-15. Same question here: What is the right value
> range?
> The same behavior can be found in mxs_lradc_setup_ts_pressure() in lines 485
> and 498.
> For over_sample_delay, the DT bindings state a range of 1...2047. In
> mxs_lradc_setup_ts_channel(), line 440, the value decreased by one (0...2046)
> is written to register 0x100, bits 0-10. Question: which value range is valid
> there? The same happens in line 498.
> For settling_delay, the DT bindings state a range of 1...2047. In
> mxs_lradc_setup_ts_channel(), line 458, that value is written to register
> 0xf0, bits 0-10. Question: what value range is valid here, 1...2047 or
> 0...2047? The same happens in line 517.
> Thanks,
>
> Hartmut
> >

Stefan

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Hartmut Knaack]

Stefan Wahren schrieb am 29.11.2014 um 12:22:
> Hi Hartmut,
> 
>> Hartmut Knaack <knaack.h@gmx.de> hat am 29. November 2014 um 00:28
>> geschrieben:
>>
>>
>> Fabio Estevam schrieb am 19.11.2014 um 23:42:
>>> [Adding Marek]
>> Taking a closer look on how these values are used, I wondered what the real
>> value range of the registers actually are. So, anyone with access to the data
>> sheets, please confirm.
> 
> the reference manual is public:
> 
> http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf
Thanks.
> 
>> Starting with over_sample_cnt, which according to the DT bindings has a range
>> of 1...31. In mxs_lradc_setup_ts_channel(), currently line 429, the value
>> decreased by one (0...30) gets written to register 0x50 (+ 0x10 for each
>> channel) to bits 24-29. Question: What is the right value range there, 0...30
>> or 0...31?
> 
> Yes, looks like an off-by-one issue. The register range is 0..31 so i would
> assume the DT property range should be logically 1..32. But i don't have checked
> the reference manual about that.
It's NUM_SAMPLES with the description: "This bit field contains the number of conversion cycles to sum together before reporting operation
complete interrupt status. Set this field to zero for a single conversion per interrupt."
So, a range of 0...31 can be assumed.
> 
>> In the same function, line 440, the value decreased by one (0...30) is written
>> to register 0x100 into bits 11-15. Same question here: What is the right value
>> range?
It's LOOP_COUNT with the description: "This bit field specifies the number of times this delay counter will count down and then trigger its
designated targets. This is particularly useful for scheduling multiple samples of an LRADC channel set. If
this field is set to 0x0, then exactly one delay loop will be generated with exactly one event triggering the
target LRADC and/or delay channels."
So, the range is also 0...31.
>> The same behavior can be found in mxs_lradc_setup_ts_pressure() in lines 485
>> and 498.
>> For over_sample_delay, the DT bindings state a range of 1...2047. In
>> mxs_lradc_setup_ts_channel(), line 440, the value decreased by one (0...2046)
>> is written to register 0x100, bits 0-10. Question: which value range is valid
>> there? The same happens in line 498.
It's DELAY with the description: "This 11-bit field counts down to zero. At zero it triggers either a set of LRADC channel conversions or
another delay channel, or both. It can trigger up to all eight LRADCs and all four delay channels in a single
event. This counter operates on a 2KHz clock derived from crystal clock."
So, the range is 0...2047.
>> For settling_delay, the DT bindings state a range of 1...2047. In
>> mxs_lradc_setup_ts_channel(), line 458, that value is written to register
>> 0xf0, bits 0-10. Question: what value range is valid here, 1...2047 or
>> 0...2047? The same happens in line 517.
This is the same as register 0x100, so 0...2047 is the valid range.
Stefan, would you mind to change the DT documentation while you are on it?
>> Thanks,
>>
>> Hartmut
>>>
> 
> Stefan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Kristina Martšenko]

On 29/11/14 20:47, Hartmut Knaack wrote:
> Stefan Wahren schrieb am 29.11.2014 um 12:22:
>>> Hartmut Knaack <knaack.h@gmx.de> hat am 29. November 2014 um 00:28
>>> geschrieben:
>>> Fabio Estevam schrieb am 19.11.2014 um 23:42:
>>>> [Adding Marek]
>>> Taking a closer look on how these values are used, I wondered what the real
>>> value range of the registers actually are. So, anyone with access to the data
>>> sheets, please confirm.
>>
>> the reference manual is public:
>>
>> http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf

[...]

>>> For over_sample_delay, the DT bindings state a range of 1...2047. In
>>> mxs_lradc_setup_ts_channel(), line 440, the value decreased by one (0...2046)
>>> is written to register 0x100, bits 0-10. Question: which value range is valid
>>> there? The same happens in line 498.
> It's DELAY with the description: "This 11-bit field counts down to zero. At zero it triggers either a set of LRADC channel conversions or
> another delay channel, or both. It can trigger up to all eight LRADCs and all four delay channels in a single
> event. This counter operates on a 2KHz clock derived from crystal clock."
> So, the range is 0...2047.

There's also a "Note" on pages 2664-2665 of the reference manual:

"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."

So 0 isn't valid, leaving the actual range at 1..2047.

>>> For settling_delay, the DT bindings state a range of 1...2047. In
>>> mxs_lradc_setup_ts_channel(), line 458, that value is written to register
>>> 0xf0, bits 0-10. Question: what value range is valid here, 1...2047 or
>>> 0...2047? The same happens in line 517.
> This is the same as register 0x100, so 0...2047 is the valid range.

Yeah, 1..2047 again.

Note that we subtract 1 from over_sample_delay before writing it to a
register, so its DT range would be 2..2048. But we don't subtract
anything from settling_delay, so its DT range would be 1..2047. Probably
would be nicer to subtract 1 from neither (or both?), and have the DT
ranges be the same.

> Stefan, would you mind to change the DT documentation while you are on it?

Kristina

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Stefan Wahren]

Phew, i underestimated the consequences of my patch.
=20
@Jonathan: could you please drop or revert my patch, before Greg catchs it?

> Kristina Mart=C5=A1enko <kristina.martsenko@gmail.com> hat am 30. Novembe=
r 2014 um
> 13:10 geschrieben:
>
>
> On 29/11/14 20:47, Hartmut Knaack wrote:
> > Stefan Wahren schrieb am 29.11.2014 um 12:22:
> >>> Hartmut Knaack <knaack.h@gmx.de> hat am 29. November 2014 um 00:28
> >>> geschrieben:
> >>> Fabio Estevam schrieb am 19.11.2014 um 23:42:
> >>>> [Adding Marek]
> >>> Taking a closer look on how these values are used, I wondered what th=
e
> >>> real
> >>> value range of the registers actually are. So, anyone with access to =
the
> >>> data
> >>> sheets, please confirm.
> >>
> >> the reference manual is public:
> >>
> >> http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf
>
> [...]

Sorry for being so inprecise, we need to care of both SoCs. The reference m=
anual
for the i.MX23 is here:

http://cache.freescale.com/files/dsp/doc/ref_manual/IMX23RM.pdf

>
> >>> For over_sample_delay, the DT bindings state a range of 1...2047. In
> >>> mxs_lradc_setup_ts_channel(), line 440, the value decreased by one
> >>> (0...2046)
> >>> is written to register 0x100, bits 0-10. Question: which value range =
is
> >>> valid
> >>> there? The same happens in line 498.
> > It's DELAY with the description: "This 11-bit field counts down to zero=
. At
> > zero it triggers either a set of LRADC channel conversions or
> > another delay channel, or both. It can trigger up to all eight LRADCs a=
nd
> > all four delay channels in a single
> > event. This counter operates on a 2KHz clock derived from crystal clock=
."
> > So, the range is 0...2047.
>
> There's also a "Note" on pages 2664-2665 of the reference manual:
>
> "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."
>
> So 0 isn't valid, leaving the actual range at 1..2047.

Thanks for pointing out. It would be wise to add this "note" into the drive=
r.

>
> >>> For settling_delay, the DT bindings state a range of 1...2047. In
> >>> mxs_lradc_setup_ts_channel(), line 458, that value is written to regi=
ster
> >>> 0xf0, bits 0-10. Question: what value range is valid here, 1...2047 o=
r
> >>> 0...2047? The same happens in line 517.
> > This is the same as register 0x100, so 0...2047 is the valid range.
>
> Yeah, 1..2047 again.
>
> Note that we subtract 1 from over_sample_delay before writing it to a
> register, so its DT range would be 2..2048. But we don't subtract
> anything from settling_delay, so its DT range would be 1..2047. Probably
> would be nicer to subtract 1 from neither (or both?), and have the DT
> ranges be the same.

Substracting unsigned values isn't good. But breaking DT compatibly also.

>
> > Stefan, would you mind to change the DT documentation while you are on =
it?

No problem, but these changes need at least a test with a touchscreen and i
don't have any.

>
> Kristina

Stefan

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Stefan Wahren]

> Stefan Wahren <stefan.wahren@i2se.com> hat am 30. November 2014 um 14:29
> geschrieben:
>
>
> Phew, i underestimated the consequences of my patch.
>
> @Jonathan: could you please drop or revert my patch, before Greg catchs i=
t?
>
> > Kristina Mart=C5=A1enko <kristina.martsenko@gmail.com> hat am 30. Novem=
ber 2014
> > um
> > 13:10 geschrieben:
> >
> >
> > On 29/11/14 20:47, Hartmut Knaack wrote:
> > > Stefan Wahren schrieb am 29.11.2014 um 12:22:
> > >>> Hartmut Knaack <knaack.h@gmx.de> hat am 29. November 2014 um 00:28
> > >>> geschrieben:
> > >>> Fabio Estevam schrieb am 19.11.2014 um 23:42:
> > >>>> [Adding Marek]
> > >>> Taking a closer look on how these values are used, I wondered what =
the
> > >>> real
> > >>> value range of the registers actually are. So, anyone with access t=
o the
> > >>> data
> > >>> sheets, please confirm.
> > >>
> > >> the reference manual is public:
> > >>
> > >> http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf
> >
> > [...]
>
> Sorry for being so inprecise, we need to care of both SoCs. The reference
> manual
> for the i.MX23 is here:
>
> http://cache.freescale.com/files/dsp/doc/ref_manual/IMX23RM.pdf
>
> >
> > >>> For over_sample_delay, the DT bindings state a range of 1...2047. I=
n
> > >>> mxs_lradc_setup_ts_channel(), line 440, the value decreased by one
> > >>> (0...2046)
> > >>> is written to register 0x100, bits 0-10. Question: which value rang=
e is
> > >>> valid
> > >>> there? The same happens in line 498.
> > > It's DELAY with the description: "This 11-bit field counts down to ze=
ro.
> > > At
> > > zero it triggers either a set of LRADC channel conversions or
> > > another delay channel, or both. It can trigger up to all eight LRADCs=
 and
> > > all four delay channels in a single
> > > event. This counter operates on a 2KHz clock derived from crystal clo=
ck."
> > > So, the range is 0...2047.
> >
> > There's also a "Note" on pages 2664-2665 of the reference manual:
> >
> > "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."
> >
> > So 0 isn't valid, leaving the actual range at 1..2047.
>
> Thanks for pointing out. It would be wise to add this "note" into the dri=
ver.
>
> >
> > >>> For settling_delay, the DT bindings state a range of 1...2047. In
> > >>> mxs_lradc_setup_ts_channel(), line 458, that value is written to
> > >>> register
> > >>> 0xf0, bits 0-10. Question: what value range is valid here, 1...2047=
 or
> > >>> 0...2047? The same happens in line 517.
> > > This is the same as register 0x100, so 0...2047 is the valid range.
> >
> > Yeah, 1..2047 again.
> >
> > Note that we subtract 1 from over_sample_delay before writing it to a
> > register, so its DT range would be 2..2048. But we don't subtract
> > anything from settling_delay, so its DT range would be 1..2047. Probabl=
y
> > would be nicer to subtract 1 from neither (or both?), and have the DT
> > ranges be the same.
>
> Substracting unsigned values isn't good. But breaking DT compatibly also.
>
> >
> > > Stefan, would you mind to change the DT documentation while you are o=
n it?
>
> No problem, but these changes need at least a test with a touchscreen and=
 i
> don't have any.
>
> >
> > Kristina
>
> Stefan

ping ...

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Jonathan Cameron]

On 08/12/14 19:40, Stefan Wahren wrote:
>> Stefan Wahren <stefan.wahren@i2se.com> hat am 30. November 2014 um 14:29
>> geschrieben:
>>
>>
>> Phew, i underestimated the consequences of my patch.
>>
>> @Jonathan: could you please drop or revert my patch, before Greg catchs it?
Did so.
>>
>>> Kristina Martšenko <kristina.martsenko@gmail.com> hat am 30. November 2014
>>> um
>>> 13:10 geschrieben:
>>>
>>>
>>> On 29/11/14 20:47, Hartmut Knaack wrote:
>>>> Stefan Wahren schrieb am 29.11.2014 um 12:22:
>>>>>> Hartmut Knaack <knaack.h@gmx.de> hat am 29. November 2014 um 00:28
>>>>>> geschrieben:
>>>>>> Fabio Estevam schrieb am 19.11.2014 um 23:42:
>>>>>>> [Adding Marek]
>>>>>> Taking a closer look on how these values are used, I wondered what the
>>>>>> real
>>>>>> value range of the registers actually are. So, anyone with access to the
>>>>>> data
>>>>>> sheets, please confirm.
>>>>>
>>>>> the reference manual is public:
>>>>>
>>>>> http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf
>>>
>>> [...]
>>
>> Sorry for being so inprecise, we need to care of both SoCs. The reference
>> manual
>> for the i.MX23 is here:
>>
>> http://cache.freescale.com/files/dsp/doc/ref_manual/IMX23RM.pdf
>>
>>>
>>>>>> For over_sample_delay, the DT bindings state a range of 1...2047. In
>>>>>> mxs_lradc_setup_ts_channel(), line 440, the value decreased by one
>>>>>> (0...2046)
>>>>>> is written to register 0x100, bits 0-10. Question: which value range is
>>>>>> valid
>>>>>> there? The same happens in line 498.
>>>> It's DELAY with the description: "This 11-bit field counts down to zero.
>>>> At
>>>> zero it triggers either a set of LRADC channel conversions or
>>>> another delay channel, or both. It can trigger up to all eight LRADCs and
>>>> all four delay channels in a single
>>>> event. This counter operates on a 2KHz clock derived from crystal clock."
>>>> So, the range is 0...2047.
>>>
>>> There's also a "Note" on pages 2664-2665 of the reference manual:
>>>
>>> "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."
>>>
>>> So 0 isn't valid, leaving the actual range at 1..2047.
>>
>> Thanks for pointing out. It would be wise to add this "note" into the driver.
>>
>>>
>>>>>> For settling_delay, the DT bindings state a range of 1...2047. In
>>>>>> mxs_lradc_setup_ts_channel(), line 458, that value is written to
>>>>>> register
>>>>>> 0xf0, bits 0-10. Question: what value range is valid here, 1...2047 or
>>>>>> 0...2047? The same happens in line 517.
>>>> This is the same as register 0x100, so 0...2047 is the valid range.
>>>
>>> Yeah, 1..2047 again.
>>>
>>> Note that we subtract 1 from over_sample_delay before writing it to a
>>> register, so its DT range would be 2..2048. But we don't subtract
>>> anything from settling_delay, so its DT range would be 1..2047. Probably
>>> would be nicer to subtract 1 from neither (or both?), and have the DT
>>> ranges be the same.
>>
>> Substracting unsigned values isn't good. But breaking DT compatibly also.
>>
>>>
>>>> Stefan, would you mind to change the DT documentation while you are on it?
>>
>> No problem, but these changes need at least a test with a touchscreen and i
>> don't have any.
>>
>>>
>>> Kristina
>>
>> Stefan
> 
> ping ...
> 

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Hartmut Knaack]

Stefan Wahren schrieb am 19.11.2014 um 23:19:
> 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.
> 
This patch is functional correct, but I see some style issues:
To make a review with the DT bindings easier, it would help to compare against the values which got used there (which are not in hex). For sample count, the range is defined as 1...31, so it would look easier like this: if (_cnt < 1 || _cnt > 31) =>error.
Another thing to consider would be to do the boundary check on adapt, and only assign it to over_sample_cnt (or the other elements) if it is valid. Thinking this further, it would even make sense to assign a default value to over_sample_count (and the other ones) only in case that no DT property is set, instead of doing it in advance and overwriting it with the custom value.
A minor style nitpick inline.
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  drivers/staging/iio/adc/mxs-lradc.c |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index 6757f10..57c3cf6 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -1500,16 +1500,36 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
>  	if (ret == 0)
>  		lradc->over_sample_cnt = adapt;
> 
> +	if (!lradc->over_sample_cnt || lradc->over_sample_cnt > 0x1f) {
> +		dev_err(lradc->dev, "Invalid sample count (%u)\n",
> +				    lradc->over_sample_cnt);
The parameter should be indented with the opening parenthesis. Same for the other instances below.
> +		return -EINVAL;
> +	}
> +
>  	lradc->over_sample_delay = 2;
>  	ret = of_property_read_u32(lradc_node, "fsl,ave-delay", &adapt);
>  	if (ret == 0)
>  		lradc->over_sample_delay = adapt;
> 
> +	if (!lradc->over_sample_delay ||
> +	    lradc->over_sample_delay > LRADC_DELAY_DELAY_MASK) {
> +		dev_err(lradc->dev, "Invalid sample delay (%u)\n",
> +				    lradc->over_sample_delay);
> +		return -EINVAL;
> +	}
> +
>  	lradc->settling_delay = 10;
>  	ret = of_property_read_u32(lradc_node, "fsl,settling", &adapt);
>  	if (ret == 0)
>  		lradc->settling_delay = adapt;
> 
> +	if (!lradc->settling_delay ||
> +	    lradc->settling_delay > LRADC_DELAY_DELAY_MASK) {
> +		dev_err(lradc->dev, "Invalid settling delay (%u)\n",
> +				    lradc->settling_delay);
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
> 
> --
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Stefan Wahren]

Hi Hartmut,

thanks for your review. I added Marek and Juergen in CC.

> Hartmut Knaack <knaack.h@gmx.de> hat am 28. November 2014 um 23:47
> geschrieben:
>
>
> Stefan Wahren schrieb am 19.11.2014 um 23:19:
> > 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.
> >
> This patch is functional correct, but I see some style issues:
> To make a review with the DT bindings easier, it would help to compare against
> the values which got used there (which are not in hex). For sample count, the
> range is defined as 1...31, so it would look easier like this: if (_cnt < 1 ||
> _cnt > 31) =>error.

I have concerns about that. The upper range is defined by the bitmask in the
register and the lower range is defined the usage of lradc->over_sample_cnt as a
divisor (mxs_lradc_read_raw_channel). Consequently i should use the "magic
number" 2047 instead of LRADC_DELAY_DELAY_MASK for the other parameters?

> Another thing to consider would be to do the boundary check on adapt, and only
> assign it to over_sample_cnt (or the other elements) if it is valid. Thinking
> this further, it would even make sense to assign a default value to
> over_sample_count (and the other ones) only in case that no DT property is
> set, instead of doing it in advance and overwriting it with the custom value.

Do you think of the following?

	if (!of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
		if (adapt < 1 || adapt > 31) {
			dev_err(lradc->dev, "Invalid sample count (%lu)\n",
				adapt);
			return -EINVAL;
		}
		lradc->over_sample_cnt = adapt;
	} else
		lradc->over_sample_cnt = 4;

> A minor style nitpick inline.
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > ---
> > drivers/staging/iio/adc/mxs-lradc.c | 20 ++++++++++++++++++++
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> > b/drivers/staging/iio/adc/mxs-lradc.c
> > index 6757f10..57c3cf6 100644
> > --- a/drivers/staging/iio/adc/mxs-lradc.c
> > +++ b/drivers/staging/iio/adc/mxs-lradc.c
> > @@ -1500,16 +1500,36 @@ static int mxs_lradc_probe_touchscreen(struct
> > mxs_lradc *lradc,
> > if (ret == 0)
> > lradc->over_sample_cnt = adapt;
> >
> > + if (!lradc->over_sample_cnt || lradc->over_sample_cnt > 0x1f) {
> > + dev_err(lradc->dev, "Invalid sample count (%u)\n",
> > + lradc->over_sample_cnt);
> The parameter should be indented with the opening parenthesis. Same for the
> other instances below.

Fixed in the example above ;-)

I wonder why checkpatch doesn't complain about it.

Stefan

Re: [PATCH] iio: mxs-lradc: check ranges of ts properties

[Hartmut Knaack]

Stefan Wahren schrieb am 29.11.2014 um 12:06:
> Hi Hartmut,
> 
> thanks for your review. I added Marek and Juergen in CC.
> 
>> Hartmut Knaack <knaack.h@gmx.de> hat am 28. November 2014 um 23:47
>> geschrieben:
>>
>>
>> Stefan Wahren schrieb am 19.11.2014 um 23:19:
>>> 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.
>>>
>> This patch is functional correct, but I see some style issues:
>> To make a review with the DT bindings easier, it would help to compare against
>> the values which got used there (which are not in hex). For sample count, the
>> range is defined as 1...31, so it would look easier like this: if (_cnt < 1 ||
>> _cnt > 31) =>error.
> 
> I have concerns about that. The upper range is defined by the bitmask in the
> register and the lower range is defined the usage of lradc->over_sample_cnt as a
> divisor (mxs_lradc_read_raw_channel). Consequently i should use the "magic
> number" 2047 instead of LRADC_DELAY_DELAY_MASK for the other parameters?
> 
>> Another thing to consider would be to do the boundary check on adapt, and only
>> assign it to over_sample_cnt (or the other elements) if it is valid. Thinking
>> this further, it would even make sense to assign a default value to
>> over_sample_count (and the other ones) only in case that no DT property is
>> set, instead of doing it in advance and overwriting it with the custom value.
> 
> Do you think of the following?
> 
> 	if (!of_property_read_u32(lradc_node, "fsl,ave-ctrl", &adapt)) {
> 		if (adapt < 1 || adapt > 31) {
> 			dev_err(lradc->dev, "Invalid sample count (%lu)\n",
> 				adapt);
> 			return -EINVAL;
> 		}
> 		lradc->over_sample_cnt = adapt;
> 	} else
> 		lradc->over_sample_cnt = 4;
> 
Yes, that's what I had in mind. Just keep in mind, that when the if-part uses { }, the else part should use them as well.
>> A minor style nitpick inline.
>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>> drivers/staging/iio/adc/mxs-lradc.c | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
>>> b/drivers/staging/iio/adc/mxs-lradc.c
>>> index 6757f10..57c3cf6 100644
>>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>>> @@ -1500,16 +1500,36 @@ static int mxs_lradc_probe_touchscreen(struct
>>> mxs_lradc *lradc,
>>> if (ret == 0)
>>> lradc->over_sample_cnt = adapt;
>>>
>>> + if (!lradc->over_sample_cnt || lradc->over_sample_cnt > 0x1f) {
>>> + dev_err(lradc->dev, "Invalid sample count (%u)\n",
>>> + lradc->over_sample_cnt);
>> The parameter should be indented with the opening parenthesis. Same for the
>> other instances below.
> 
> Fixed in the example above ;-)
> 
> I wonder why checkpatch doesn't complain about it.
> 
You need to run it with --strict to check for minor nitpicks ;-)
> Stefan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>