[PATCH 3/5] Staging/iio/adc/touchscreen/MXS: add i.MX23 support to the LRADC driver

Dan Carpenter
2013-09-04T14:27:39Z

On Wed, Sep 04, 2013 at 03:01:44PM +0200, Juergen Beisert wrote:
> @@ -323,10 +338,17 @@ static int mxs_lradc_ts_touched(struct mxs_lradc *lradc)
> uint32_t reg;
>
> /* Enable touch detection. */
> - writel(LRADC_CTRL0_MX28_PLATE_MASK,
> - lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_CLR);
> - writel(LRADC_CTRL0_MX28_TOUCH_DETECT_ENABLE,
> - lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_SET);
> + if (lradc->soc == IMX28_LRADC) {
> + writel(LRADC_CTRL0_MX28_PLATE_MASK,
> + lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_CLR);
> + writel(LRADC_CTRL0_MX28_TOUCH_DETECT_ENABLE,
> + lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_SET);
> + } else {
> + writel(LRADC_CTRL0_MX23_PLATE_MASK,
> + lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_CLR);
> + writel(LRADC_CTRL0_MX23_TOUCH_DETECT_ENABLE,
> + lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_SET);
> + }

I don't like the way this patch takes the driver and makes every line
an if else statement. It would be cleaner to do this:

writel(lradc_plate_mask(lradc),
lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_CLR);
writel(lradc_touch_detect_enable(lradc),
lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_SET);

Btw, LRADC_CTRL0_MX23_TOUCH_DETECT_ENABLE just enables the touch screen
to detect touches? It seems like we could leave the "DETECT_" out of
the name.

Actually it would better yet if there were a function:

static inline void lradc_writel(struct mxs_lradc *lradc, u32 val,
size_t chan, size_t offset)
{
writel(val, lradc->base + chan + offset);
}

That way we could do:

lradc_writel(lradc_enable_touch(lradc), LRADC_CTRL0,
STMP_OFFSET_REG_SET);

ACTUALLY! When I look at it now the third argument is almost always
"set", "clear" or "toggle".

So we could do:

static inline void lradc_reg_set(struct mxs_lradc *lradc, u32 val,
size_t chan)
{
writel(val, lradc->base + chan + STMP_OFFSET_REG_SET);
}

So then it would be:

lradc_reg_clear(lradc_plate_mask(lradc), LRADC_CTRL0);
lradc_reg_set(lradc_enable_touch(lradc), LRADC_CTRL0);

I've just changed 11 lines of code down to 2 lines of code by hiding the
if statement in the header file.

Please redo this patch along those lines.

regards,
dan carpenter