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

[Stefan Wahren <stefan.wahren@i2se.com>]

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