Hi Hartmut,
thanks for your review. I added Marek and Juergen in CC.
> Hartmut Knaack <knaack.h@xxxxxx> 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@xxxxxxxx>
> > ---
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
