Query regarding a IIO consumer touchscreen driver

Query regarding a IIO consumer touchscreen driver

[Sanchayan Maity]

Hello,

I have been working on a IIO consumer touchscreen driver. On one of our Vybrid modules we use the ADC channels for
the touchscreen. I ported an earlier driver in the 3.0 kernel done by my colleague to the recent 3.18/19 kernel to
use the IIO based consumer interface.

The touchscreen driver is here
http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/input/touchscreen/colibri-vf50-ts.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5

The ADC driver with which it interacts is here
http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/iio/adc/vf610_adc.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5

Currently, this works in a limited capaicty I would say. One of the problems I have with this driver is that when
this driver is used I can't get valid readings for any of the other ADC channels by using something like "cat". I
believe this is because of the fact that the workqueue runs a while loop every 10ms and reads the channels. This
clearly does not play well with reading of other channels and results in random reads on trying to read other channels,
for example the temperature channel. The exact reason I have not figured out here. I was thinking may be a device
lock would be required, but, that doesn't help.

The workqueue function
http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/input/touchscreen/colibri-vf50-ts.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5#n102

The primary read raw function
http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/iio/adc/vf610_adc.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5#n449

I was thinking this would need a better design than what has been done? Can someone give some suggestions as what
would be the right design/architecture here. I was thinking if setting up a IIO ring buffer, pushing the readings
to it and then reading from the touchscreen driver would work. One of the aims is also to make it clean enough for
eventual submission to the mainline. 

Thanks & Regards,
Sanchayan Maity.

Re: Query regarding a IIO consumer touchscreen driver

[Jonathan Cameron]

On 09/01/15 05:58, Sanchayan Maity wrote:
> Hello,
> 
> I have been working on a IIO consumer touchscreen driver. On one of our Vybrid modules we use the ADC channels for
> the touchscreen. I ported an earlier driver in the 3.0 kernel done by my colleague to the recent 3.18/19 kernel to
> use the IIO based consumer interface.
> 
> The touchscreen driver is here
> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/input/touchscreen/colibri-vf50-ts.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5
> 
> The ADC driver with which it interacts is here
> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/iio/adc/vf610_adc.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5
> 
> Currently, this works in a limited capaicty I would say. One of the problems I have with this driver is that when
> this driver is used I can't get valid readings for any of the other ADC channels by using something like "cat". I
> believe this is because of the fact that the workqueue runs a while loop every 10ms and reads the channels. This
> clearly does not play well with reading of other channels and results in random reads on trying to read other channels,
> for example the temperature channel. The exact reason I have not figured out here. I was thinking may be a device
> lock would be required, but, that doesn't help.
Certainly sounds like something along those lines. 
> 
> The workqueue function
> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/input/touchscreen/colibri-vf50-ts.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5#n102
> 
> The primary read raw function
> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/iio/adc/vf610_adc.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5#n449
> 
> I was thinking this would need a better design than what has been done? Can someone give some suggestions as what
> would be the right design/architecture here. I was thinking if setting up a IIO ring buffer, pushing the readings
> to it and then reading from the touchscreen driver would work. One of the aims is also to make it clean enough for
> eventual submission to the mainline. 
This sort of use case was what the in kernel buffer interfaces were intended for.
They don't actually use a buffer as such - but rather at the same point where
we demultiplex the incoming data to fill the IIO buffer we can also send data
on to other 'consumer' drivers.

It's rather ancient but I did write a proof of concept input driver at one point.
http://permalink.gmane.org/gmane.linux.kernel.iio/5757
Might be the latest version...

Should give you an idea of the thinking behind it anyway.  We have have discussed
a generic approach to touchscreen drivers based on ADCs in the past, but that
has mostly been for devices with more explicit touch screen features and we
concluded that writing a generic driver would be 'tricky' given how many
subtle variants there are.

Looking at the driver - you are correct that the first step would be to
implement buffered IIO support and go from there.  It's a bit of an irritating
device by the look of it... Every time you want to grab a different channel
you have to explicitly change it (not concept of multichannel scans).
It does have a continuous mode which would be great to support, but that's
not going to be much use for your touch screen as it will only read one
channel.

Another more generic snag is that currently is drivers operate in one or other mode.
Either we do buffered access, or we do polled access.  The only devices
that do both are those that maintain hardware storage of the previous
value for a given channel.

I have some thoughts on how to deal with this by grabbing additional
'of interest' channels during buffered capture and stashing them.  The
main issue is that the userspace interface is then messy or non obvious.

Hope that might give you somewhere to get started.

Jonathan
> 
> Thanks & Regards,
> Sanchayan Maity.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: Query regarding a IIO consumer touchscreen driver

[Sanchayan Maity]

On 01/10/2015 04:33 PM, Jonathan Cameron wrote:
> On 09/01/15 05:58, Sanchayan Maity wrote:
>> Hello,
>>
>> I have been working on a IIO consumer touchscreen driver. On one of our Vybrid modules we use the ADC channels for
>> the touchscreen. I ported an earlier driver in the 3.0 kernel done by my colleague to the recent 3.18/19 kernel to
>> use the IIO based consumer interface.
>>
>> The touchscreen driver is here
>> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/input/touchscreen/colibri-vf50-ts.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5
>>
>> The ADC driver with which it interacts is here
>> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/iio/adc/vf610_adc.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5
>>
>> Currently, this works in a limited capaicty I would say. One of the problems I have with this driver is that when
>> this driver is used I can't get valid readings for any of the other ADC channels by using something like "cat". I
>> believe this is because of the fact that the workqueue runs a while loop every 10ms and reads the channels. This
>> clearly does not play well with reading of other channels and results in random reads on trying to read other channels,
>> for example the temperature channel. The exact reason I have not figured out here. I was thinking may be a device
>> lock would be required, but, that doesn't help.
> Certainly sounds like something along those lines. 
>>
>> The workqueue function
>> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/input/touchscreen/colibri-vf50-ts.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5#n102
>>
>> The primary read raw function
>> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/iio/adc/vf610_adc.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5#n449
>>
>> I was thinking this would need a better design than what has been done? Can someone give some suggestions as what
>> would be the right design/architecture here. I was thinking if setting up a IIO ring buffer, pushing the readings
>> to it and then reading from the touchscreen driver would work. One of the aims is also to make it clean enough for
>> eventual submission to the mainline. 
> This sort of use case was what the in kernel buffer interfaces were intended for.
> They don't actually use a buffer as such - but rather at the same point where
> we demultiplex the incoming data to fill the IIO buffer we can also send data
> on to other 'consumer' drivers.

How exactly data can be send to other consumer drivers? I tried going through the 
drivers in iio/adc and the header files, but, couldn't exactly figure this out.
> 
> It's rather ancient but I did write a proof of concept input driver at one point.
> http://permalink.gmane.org/gmane.linux.kernel.iio/5757
> Might be the latest version...

Thanks for the idea.
> 
> Should give you an idea of the thinking behind it anyway.  We have have discussed
> a generic approach to touchscreen drivers based on ADCs in the past, but that
> has mostly been for devices with more explicit touch screen features and we
> concluded that writing a generic driver would be 'tricky' given how many
> subtle variants there are.

I would agree. Our touchscreen requires controlling four GPIO and pin multiplexing
of four ADC channels for enabling/disabling/re-enabling readouts. Haven't been able
to exactly figure how to maintain a clean separation between the input subsytem
driver and the IIO ADC one, due to the GPIO pins, GPIO IRQs and ADC channels. We
first enable the so called plates/gpios to detect IRQ, while keeping two of the
ADC channels as GPIOs. Once the IRQ is detected, we disable IRQ, the touch detection
plates/gpios, change the pinmux to ADC and the continously sample the ADC channels
at 10ms intervals. Not as straight forward as the other drivers which I could see.

> 
> Looking at the driver - you are correct that the first step would be to
> implement buffered IIO support and go from there.  It's a bit of an irritating
> device by the look of it... Every time you want to grab a different channel
> you have to explicitly change it (not concept of multichannel scans).
> It does have a continuous mode which would be great to support, but that's
> not going to be much use for your touch screen as it will only read one
> channel.

Yes, it is tricky. Currently, I couldn't find anything similar in the souce for 
what I wanted to do.

>From what I understand at the moment, the relevant GPIO, IRQ data needs to be
with the ADC driver. I do all the enable/renable of plates/IRQs/ADC in the driver,
push the data in the buffers and the have my touchscreen driver read the data
from the buffers. Though I do not have the separation I would wanted to have
between the data and the two subsystems.
> 
> Another more generic snag is that currently is drivers operate in one or other mode.
> Either we do buffered access, or we do polled access.  The only devices
> that do both are those that maintain hardware storage of the previous
> value for a given channel.
> 
> I have some thoughts on how to deal with this by grabbing additional
> 'of interest' channels during buffered capture and stashing them.  The
> main issue is that the userspace interface is then messy or non obvious.
> 
> Hope that might give you somewhere to get started.
> 
> Jonathan
>>
>> Thanks & Regards,
>> Sanchayan Maity.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
Regards,
Sanchayan.

Re: Query regarding a IIO consumer touchscreen driver

[Jonathan Cameron]

On 12/01/15 12:54, Sanchayan Maity wrote:
> On 01/10/2015 04:33 PM, Jonathan Cameron wrote:
>> On 09/01/15 05:58, Sanchayan Maity wrote:
>>> Hello,
>>>
>>> I have been working on a IIO consumer touchscreen driver. On one of our Vybrid modules we use the ADC channels for
>>> the touchscreen. I ported an earlier driver in the 3.0 kernel done by my colleague to the recent 3.18/19 kernel to
>>> use the IIO based consumer interface.
>>>
>>> The touchscreen driver is here
>>> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/input/touchscreen/colibri-vf50-ts.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5
>>>
>>> The ADC driver with which it interacts is here
>>> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/iio/adc/vf610_adc.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5
>>>
>>> Currently, this works in a limited capaicty I would say. One of the problems I have with this driver is that when
>>> this driver is used I can't get valid readings for any of the other ADC channels by using something like "cat". I
>>> believe this is because of the fact that the workqueue runs a while loop every 10ms and reads the channels. This
>>> clearly does not play well with reading of other channels and results in random reads on trying to read other channels,
>>> for example the temperature channel. The exact reason I have not figured out here. I was thinking may be a device
>>> lock would be required, but, that doesn't help.
>> Certainly sounds like something along those lines. 
>>>
>>> The workqueue function
>>> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/input/touchscreen/colibri-vf50-ts.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5#n102
>>>
>>> The primary read raw function
>>> http://git.toradex.com/cgit/linux-toradex.git/tree/drivers/iio/adc/vf610_adc.c?h=toradex_vf_3.18-next&id=7142dd741e948e1536632f9db1f3dd3a015450a5#n449
>>>
>>> I was thinking this would need a better design than what has been done? Can someone give some suggestions as what
>>> would be the right design/architecture here. I was thinking if setting up a IIO ring buffer, pushing the readings
>>> to it and then reading from the touchscreen driver would work. One of the aims is also to make it clean enough for
>>> eventual submission to the mainline. 
>> This sort of use case was what the in kernel buffer interfaces were intended for.
>> They don't actually use a buffer as such - but rather at the same point where
>> we demultiplex the incoming data to fill the IIO buffer we can also send data
>> on to other 'consumer' drivers.
> 
> How exactly data can be send to other consumer drivers? I tried going through the 
> drivers in iio/adc and the header files, but, couldn't exactly figure this out.
When you do a iio_push_to_buffers_* if there are multiple consumers registered they
will all get whatever channels they are asking for.  This is done via a fiddly demux
unit in the core code.  In the simple case this does nothing, but we should get
the right data going to the right consumer when needed.  Note the normal 'buffer'
interface of iio is just one such consumer.
>>
>> It's rather ancient but I did write a proof of concept input driver at one point.
>> http://permalink.gmane.org/gmane.linux.kernel.iio/5757
>> Might be the latest version...
> 
> Thanks for the idea.
>>
>> Should give you an idea of the thinking behind it anyway.  We have have discussed
>> a generic approach to touchscreen drivers based on ADCs in the past, but that
>> has mostly been for devices with more explicit touch screen features and we
>> concluded that writing a generic driver would be 'tricky' given how many
>> subtle variants there are.
> 
> I would agree. Our touchscreen requires controlling four GPIO and pin multiplexing
> of four ADC channels for enabling/disabling/re-enabling readouts. Haven't been able
> to exactly figure how to maintain a clean separation between the input subsytem
> driver and the IIO ADC one, due to the GPIO pins, GPIO IRQs and ADC channels. We
> first enable the so called plates/gpios to detect IRQ, while keeping two of the
> ADC channels as GPIOs. Once the IRQ is detected, we disable IRQ, the touch detection
> plates/gpios, change the pinmux to ADC and the continously sample the ADC channels
> at 10ms intervals. Not as straight forward as the other drivers which I could see.
To do this I think we'd need to have carefully synchronised output and input in a fashion
that we haven't begun to look at yet I'm afraid.  I can conceive of how it might work
but would involved nasty things like delayed triggers (one trigger firing another, only
after the first is finished).  It'd be fiddly and require a lot of new infrastructure.
> 
>>
>> Looking at the driver - you are correct that the first step would be to
>> implement buffered IIO support and go from there.  It's a bit of an irritating
>> device by the look of it... Every time you want to grab a different channel
>> you have to explicitly change it (not concept of multichannel scans).
>> It does have a continuous mode which would be great to support, but that's
>> not going to be much use for your touch screen as it will only read one
>> channel.
> 
> Yes, it is tricky. Currently, I couldn't find anything similar in the souce for 
> what I wanted to do.
I'm not aware of anything.   It's been discussed a few times, but no one has
taken on the challenge yet (at least not publically).
> 
> From what I understand at the moment, the relevant GPIO, IRQ data needs to be
> with the ADC driver. I do all the enable/renable of plates/IRQs/ADC in the driver,
> push the data in the buffers and the have my touchscreen driver read the data
> from the buffers. Though I do not have the separation I would wanted to have
> between the data and the two subsystems.
Too fiddly indeed.  Perhaps best bet in the first instance is a unified driver doing
it all, then try to split it up later once we have code to look at.
>>
>> Another more generic snag is that currently is drivers operate in one or other mode.
>> Either we do buffered access, or we do polled access.  The only devices
>> that do both are those that maintain hardware storage of the previous
>> value for a given channel.
>>
>> I have some thoughts on how to deal with this by grabbing additional
>> 'of interest' channels during buffered capture and stashing them.  The
>> main issue is that the userspace interface is then messy or non obvious.
>>
>> Hope that might give you somewhere to get started.
>>
>> Jonathan
>>>
>>> Thanks & Regards,
>>> Sanchayan Maity.
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> Regards,
> Sanchayan.
>