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
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
