[PATCH] iio: ti_am335x_adc: Use same step id at FIFOs both ends

[PATCH] iio: ti_am335x_adc: Use same step id at FIFOs both ends

[Jan Kardell]

Since AI lines could be selected at will (linux-3.11) the sending
and receiving ends of the FIFO does not agree about what step is used
for a line. It only works if the last lines are used, like 5,6,7,
and fails if ie 2,4,6 is selected in DT.

Signed-off-by: Jan Kardell <jan.kardell-KSZdJiTw9mzQT0dZR+AlfA@public.gmane.org>
---
 drivers/iio/adc/ti_am335x_adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index a4db302..d5dc4c6 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -374,7 +374,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 			return -EAGAIN;
 		}
 	}
-	map_val = chan->channel + TOTAL_CHANNELS;
+	map_val = adc_dev->channel_step[chan->scan_index];
 
 	/*
 	 * We check the complete FIFO. We programmed just one entry but in case
-- 
1.8.4.5

[PATCH] iio: ti_am335x_adc: Use same step id at FIFOs both ends

[Jan Kardell]

Since AI lines could be selected at will (linux-3.11) the sending
and receiving ends of the FIFO does not agree about what step is used
for a line. It only works if the last lines are used, like 5,6,7,
and fails if ie 2,4,6 is selected in DT.

Signed-off-by: Jan Kardell <jan.kardell@telliq.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index a4db302..d5dc4c6 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -374,7 +374,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 			return -EAGAIN;
 		}
 	}
-	map_val = chan->channel + TOTAL_CHANNELS;
+	map_val = adc_dev->channel_step[chan->scan_index];
 
 	/*
 	 * We check the complete FIFO. We programmed just one entry but in case
-- 
1.8.4.5

Re: [PATCH] iio: ti_am335x_adc: Use same step id at FIFOs both ends

[Zubair Lutfullah :]

On Sat, Jun 21, 2014 at 10:51:35AM +0100, Jonathan Cameron wrote:
> On 11/06/14 22:18, Jan Kardell wrote:
> >Since AI lines could be selected at will (linux-3.11) the sending
> >and receiving ends of the FIFO does not agree about what step is used
> >for a line. It only works if the last lines are used, like 5,6,7,
> >and fails if ie 2,4,6 is selected in DT.
> >
> >Signed-off-by: Jan Kardell <jan.kardell@telliq.com>
> I'd like an ack / review from someone more familiar with this part than I
> am. I have cc'd a few people who might offer a view.

Been a while since I've seen this driver.
I'll try a bit..

> 
> Thanks,
> 
> Jonathan (IIO maintainer)
> >---
> >  drivers/iio/adc/ti_am335x_adc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> >index a4db302..d5dc4c6 100644
> >--- a/drivers/iio/adc/ti_am335x_adc.c
> >+++ b/drivers/iio/adc/ti_am335x_adc.c
> >@@ -374,7 +374,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> >  			return -EAGAIN;
> >  		}
> >  	}
> >-	map_val = chan->channel + TOTAL_CHANNELS;
> >+	map_val = adc_dev->channel_step[chan->scan_index];

Looking at the code again.
Could you tell me if using map_val = step_en will work?

Checking TRM, the channel ID is written with step id.
And if i am not mistaken, step id is already stored in step_en.
That is the step we enable for sampling.
Would make sense to check fifo for that step no?

something like

@@ -324,7 +324,7 @@
 		int *val, int *val2, long mask)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
-	int i, map_val;
+	int i;
 	unsigned int fifo1count, read, stepid;
 	bool found = false;
 	u32 step_en;
@@ -342,7 +342,6 @@
 		if (time_after(jiffies, timeout))
 			return -EAGAIN;
 		}
-	map_val = chan->channel + TOTAL_CHANNELS;
 
 	/*
 	 * When the sub-system is first enabled,
@@ -361,7 +360,7 @@
 		stepid = read & FIFOREAD_CHNLID_MASK;
 		stepid = stepid >> 0x10;
 
-		if (stepid == map_val) {
+		if (stepid == step_en) {
 			read = read & FIFOREAD_DATA_MASK;
 			found = true;
 			*val = (u16) read;


Cheers
ZubairLK



> >
> >  	/*
> >  	 * We check the complete FIFO. We programmed just one entry but in case
> >
> 

Re: [PATCH] iio: ti_am335x_adc: Use same step id at FIFOs both ends

[Jan Kardell]

Zubair Lutfullah : skriver:
> On Sat, Jun 21, 2014 at 10:51:35AM +0100, Jonathan Cameron wrote:
>> On 11/06/14 22:18, Jan Kardell wrote:
>>> Since AI lines could be selected at will (linux-3.11) the sending
>>> and receiving ends of the FIFO does not agree about what step is used
>>> for a line. It only works if the last lines are used, like 5,6,7,
>>> and fails if ie 2,4,6 is selected in DT.
>>>
>>> Signed-off-by: Jan Kardell <jan.kardell@telliq.com>
>> I'd like an ack / review from someone more familiar with this part than I
>> am. I have cc'd a few people who might offer a view.
> Been a while since I've seen this driver.
> I'll try a bit..
>
>> Thanks,
>>
>> Jonathan (IIO maintainer)
>>> ---
>>>   drivers/iio/adc/ti_am335x_adc.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>>> index a4db302..d5dc4c6 100644
>>> --- a/drivers/iio/adc/ti_am335x_adc.c
>>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>>> @@ -374,7 +374,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>>>   			return -EAGAIN;
>>>   		}
>>>   	}
>>> -	map_val = chan->channel + TOTAL_CHANNELS;
>>> +	map_val = adc_dev->channel_step[chan->scan_index];
> Looking at the code again.
> Could you tell me if using map_val = step_en will work?
>
> Checking TRM, the channel ID is written with step id.
> And if i am not mistaken, step id is already stored in step_en.
> That is the step we enable for sampling.
> Would make sense to check fifo for that step no?
step_en is is a bitmask of 16 bits intended for the STEPENABLE
register, one bit for each of the 16 possible steps, while stepid is a
four bit number, 0-15, indentifying the step used. So your suggestion
does not work.

//Jan

> something like
>
> @@ -324,7 +324,7 @@
>   		int *val, int *val2, long mask)
>   {
>   	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> -	int i, map_val;
> +	int i;
>   	unsigned int fifo1count, read, stepid;
>   	bool found = false;
>   	u32 step_en;
> @@ -342,7 +342,6 @@
>   		if (time_after(jiffies, timeout))
>   			return -EAGAIN;
>   		}
> -	map_val = chan->channel + TOTAL_CHANNELS;
>   
>   	/*
>   	 * When the sub-system is first enabled,
> @@ -361,7 +360,7 @@
>   		stepid = read & FIFOREAD_CHNLID_MASK;
>   		stepid = stepid >> 0x10;
>   
> -		if (stepid == map_val) {
> +		if (stepid == step_en) {
>   			read = read & FIFOREAD_DATA_MASK;
>   			found = true;
>   			*val = (u16) read;
>
>
> Cheers
> ZubairLK
>
>
>
>>>   	/*
>>>   	 * We check the complete FIFO. We programmed just one entry but in case
>>>

Re: [PATCH] iio: ti_am335x_adc: Use same step id at FIFOs both ends

[Zubair Lutfullah :]

On Mon, Jul 07, 2014 at 06:10:00PM +0100, Jonathan Cameron wrote:
> On 23/06/14 11:58, Jan Kardell wrote:
> >Zubair Lutfullah : skriver:
> >>On Sat, Jun 21, 2014 at 10:51:35AM +0100, Jonathan Cameron wrote:
> >>>On 11/06/14 22:18, Jan Kardell wrote:
> >>>>Since AI lines could be selected at will (linux-3.11) the sending
> >>>>and receiving ends of the FIFO does not agree about what step is used
> >>>>for a line. It only works if the last lines are used, like 5,6,7,
> >>>>and fails if ie 2,4,6 is selected in DT.
> >>>>
> >>>>Signed-off-by: Jan Kardell <jan.kardell@telliq.com>
> >>>I'd like an ack / review from someone more familiar with this part than I
> >>>am. I have cc'd a few people who might offer a view.
> >>Been a while since I've seen this driver.
> >>I'll try a bit..
> >>
> >>>Thanks,
> >>>
> >>>Jonathan (IIO maintainer)
> >>>>---
> >>>>  drivers/iio/adc/ti_am335x_adc.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> >>>>index a4db302..d5dc4c6 100644
> >>>>--- a/drivers/iio/adc/ti_am335x_adc.c
> >>>>+++ b/drivers/iio/adc/ti_am335x_adc.c
> >>>>@@ -374,7 +374,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> >>>>              return -EAGAIN;
> >>>>          }
> >>>>      }
> >>>>-    map_val = chan->channel + TOTAL_CHANNELS;
> >>>>+    map_val = adc_dev->channel_step[chan->scan_index];
> >>Looking at the code again.
> >>Could you tell me if using map_val = step_en will work?
> >>
> I'm completely lost!  Zubair, has Jan answered your query?
> (he says trying to avoid diving into the data sheets)
> 

Sorry for the long delays.

Yes Jan has answered my query.

It took a while to bring up my Beagle again.

But you can put my <Tested-by>.

And I think I figured out where things went awry.

It used to be that, the single channel read would read all channels.
Despite what was configured in DT. And then return the required channel.

When Sebastian fixed that in  mfd: input: iio: ti_amm335x: Rework TSC/ADC synchronization

This bug crept up. only the required channel is enabled. But when reading the fifo
it is checked against the old incorrect channel id.

I am guessing he would check the driver by enabling all channels like I do. ;)

A very nice bug caught by Jan.

Cheers
ZubairLK