Re: [PATCH 5/5] mfd: input: iio: ti_amm335x: rework TSC/ADC synchronisation

[Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

> The ADC driver always programms all possible ADC values and discards
> them except for the value IIO asked for. On the am335x-evm the driver
> programs four values and it takes 500us to gather them. Reducing the number
> of coversations down to (required) one also reduces the busy loop down to
> 125us.
> This leads to another error, namely the FIFOCOUNT register is sometimes
> (like one out of 10 attempts) not updated in time leading to timeouts.
> The next read has the fifocount register updated.
> Checking for the ADCStatus status register for beeing idle isn't a good
> choice either. The problem is that it often times out if the TSC is used
> at the same time. The problem is that the HW compelted the conversaton for
> the ADC *and* before the driver noticed it, the HW began to perfom a
> TSC conversation and so the driver never seen the HW idle. The next time
> we would have two values in the fifo but since the driver reads
> everything we always see the current one.
> So instead of polling for the IDLE bit, we check for the fifocount. It
> should be one instead of zero because we request one value.
> This change in turn lead to another error :) Sometimes if TSC & ADC are
> used together the TSC starts becomming interrupts even if nobody
> actually touched the touchscreen. The interrupts are valid because TSC's
> fifo is filled with values. This condition stops after a few ADC reads but
> will occur once TSC & ADC are used at the same time. So not good.
> 
> On top of this (even without the changes I just mentioned) there is a ADC
> & TSC lockup condition which was reported to my Jeff Lance including a test
> case:
> A busyloop of loop of "cat /sys/bus/iio/devices/iio\:device0/in_voltage4_raw"
> and a mug on touch screen. Whith this setup, the hardware will lockup after
> something between 20min and it could take up to a couple of hours. During that
> lockup, the ADCSTAT says 0x30 (or 0x70) which means STEP_ID = IDLE and
> FSM_BUSY = yes. That means it says that it is idle and busy at the same
> time which is an invalid condition.
> 
> For all this reasons I decided to rework this TSC/ADC part and add a
> handshake / synchronisation here:
> First the ADC signals that it needs the HW and writes a 0 mask into the
> SE register. The HW (if active) will complete the current conversation
> and become idle. The TSC driver will gather the values from the FIFO
> (woken up by an interrupt) and won't "enable" another conversation.
> Instead it will wake up the ADC driver which is already waiting. The ADC
> driver will start "its" converstation and once it is done, it will
> enable the TSC events so the TSC will work again.

Spell check this entire block.

Smileys in commit messages are generally a bad idea.

Please insert '\n's between paragraphs.

Proof read, as some of the sentences are not comprehensible.

/* MFD Parts */

> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -24,6 +24,7 @@

> -static void am335x_tsc_se_update(struct ti_tscadc_dev *tsadc)
> +void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
>  {
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tsadc->reg_lock, flags);
> +	tsadc->reg_se_cache |= val;
>  	tscadc_writel(tsadc, REG_SE, tsadc->reg_se_cache);
> +	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
>  }
> +EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
>  
> -void am335x_tsc_se_set(struct ti_tscadc_dev *tsadc, u32 val)
> +void am335x_tsc_se_tsc_set(struct ti_tscadc_dev *tsadc, u32 val)

What's the difference between:

am335x_tsc_se_set()
and
am335x_tsc_se_tsc_set()

Why don't you have a am335x_tsc_se_tsc_set_tsc_se_tsc_set() ;)

Perhaps a better function name might be in order.

>  {
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&tsadc->reg_lock, flags);
> -	tsadc->reg_se_cache |= val;
> -	am335x_tsc_se_update(tsadc);
> +	tsadc->reg_se_cache = val;
> +	if (tsadc->adc_in_use || tsadc->adc_waiting) {
> +		if (tsadc->adc_waiting)
> +			wake_up(&tsadc->reg_se_wait);

So if we're either in-use or waiting, the step mask is never set?

But I guess that the touchscreen driver assumes it has been set.

Is this okay?

> +	} else {
> +		tscadc_writel(tsadc, REG_SE, val);
> +	}

I think this would be better represented as:

        if (tsadc->adc_waiting)
                wake_up(&tsadc->reg_se_wait);
        else if (!tsadc->adc_in_use)
                tscadc_writel(tsadc, REG_SE, val);

>  	spin_unlock_irqrestore(&tsadc->reg_lock, flags);
>  }
> -EXPORT_SYMBOL_GPL(am335x_tsc_se_set);
> +EXPORT_SYMBOL_GPL(am335x_tsc_se_tsc_set);

<snip>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog