Re: [PATCH 2/2] iio: mxs-lradc: check ranges of ts properties
From: Marek Vasut <hidden>
Date: 2014-12-23 13:37:22
Also in:
linux-iio
On Monday, December 22, 2014 at 01:14:36 PM, Stefan Wahren wrote:
quoted hunk
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 <redacted> --- drivers/staging/iio/adc/mxs-lradc.c | 44 +++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 10 deletions(-)diff --git a/drivers/staging/iio/adc/mxs-lradc.cb/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(structmxs_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 ;-)
quoted hunk
+ * 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(structmxs_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 <redacted>