Messages in this thread Patch in this message | | | Date | Thu, 19 Dec 2013 08:42:06 +0000 | From | Lee Jones <> | Subject | Re: [PATCH 5/5] mfd: input: iio: ti_amm335x: rework TSC/ADC synchronisation |
| |
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |