Re: [PATCH 5/5] mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization

["Zubair Lutfullah :" <zubair.lutfullah@gmail.com>]

On Fri, Dec 20, 2013 at 09:14:36AM +0100, Sebastian Andrzej Siewior wrote:
> On 12/19/2013 08:01 PM, Zubair Lutfullah : wrote:
> >> The continues-read mode remains unchanged.
> > 
> > For one-shot reading from ADC, the TSC is 'disabled' for that moment.
> > Ok. But for continuous mode, reading from ADC disables TSC for that long time?
> 
> No, the continuous mode remains unchanged. That means while you
> enable/disable the continuous mode the TSC isn't halted / stopped and
> so TSC remains functional the whole time.

I see

> 
> > If it doesn't disable the TSC, does that mean that this fix would
> > correct the one-shot read. But the continuous mode is still prone to the 
> > FSM hanging up?
> 
> This is correct. I didn't get around to look closer at the continuous
> mode. Ideally we would also disable it and add the ADC steps to the TSC
> steps. Then I am not sure what happens (or should happen) after the
> TSC event. We don't have much options except adding the TSC back to it.
> But as I said, I didn't have much time to investigate that.
> I believe the crucial part is when we update the SE register while the
> HW is in progress and this leads to the lockup.

Indeed, due to the separation of TSC/ADC drivers, getting
them both to work simultaneously without fault is tricky.

I'll try and set up my work-bench and test this over the weekend.

> >> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> > ...
> >> @@ -329,34 +347,43 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> >>  	unsigned int fifo1count, read, stepid;
> >>  	bool found = false;
> >>  	u32 step_en;
> >> -	unsigned long timeout = jiffies + usecs_to_jiffies
> >> -				(IDLE_TIMEOUT * adc_dev->channels);
> >> +	unsigned long timeout;
> >>  
> >>  	if (iio_buffer_enabled(indio_dev))
> >>  		return -EBUSY;
> >>  
> >> -	step_en = get_adc_step_mask(adc_dev);
> >> +	step_en = get_adc_chan_step_mask(adc_dev, chan);
> >> +	if (!step_en)
> >> +		return -EINVAL;
> >> +
> >> +	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> >> +	while (fifo1count--)
> >> +		tiadc_readl(adc_dev, REG_FIFO1);
> >> +
> > Would this flush be needed ideally?
> 
> Ideally, no. Ideally (and this is what happens during my testing) is
> that the FIFO was empty and so fifo1count is 0. No reads from the FIFO.
> 
> In the unlikely event that we timed out later there might be something
> in the FIFO (but shouldn't since we gave it enough time to fill it). So
> for that case we purge it because in the loop later we wait for one
> item in the FIFO which would be true immediately.
> 

Ok.


Thanks
Zubair