Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Maurus Cuelenaere]

Op 19-10-09 12:55, linux-arm-kernel-request@lists.infradead.org schreef:
> This touchscreen driver is for touchscreen controller on Samsung
> S3C2410/S3C2440 SoC chip. S3C2410/S3C2440 has the on chip touchscreen
> controller based on it's analog to digital converter(8-channel analog
> inputs, touchscreen uses 4 of them). This driver uses the exsiting S3C
> ADC driver to make the touchscreen controller of S3C2410/S3C2440 work
> well together with other ADC devices connected to the S3C24XX SoC chip.
>
> The patch is created against kernel 2.6.32-rc4 and tested with S3C2440
> SoC and Samsung LTE430WQ-F0C touchscreen.
>
>
> Signed-off-by: Shine Liu<liuxian@redflag-linux.com>
> Signed-off-by: Shine Liu<shinel@foxmail.com>
>    

Do you know that there's another patch (at Openmoko) created by Nelson 
Castillo that does the same, but also has support for kernel-space 
touchscreen filters? (I think [1] is his latest version)
I don't know how your patch performs, but according to [2] the filters 
should help a lot avoiding jitter etc.

I'm not sure whether Nelson has submitted his patches for mainline 
review yet and what the status is on the kernel filters, but IMHO doing 
some filtering in kernel space (see the "Why are we doing filtering in 
kernel space?" part of [2]) which results in a "cleaner" output is 
preferred over reporting possible "jittery" data.

Regards,
Maurus Cuelenaere


[1] 
http://nelson-patches.googlecode.com/svn/trunk/openmoko/kernel/ts-with-s3c-adc/
[2] http://wiki.openmoko.org/wiki/Touchscreen_Filters

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Maurus Cuelenaere]

Op 19-10-09 12:55, linux-arm-kernel-request at lists.infradead.org schreef:
> This touchscreen driver is for touchscreen controller on Samsung
> S3C2410/S3C2440 SoC chip. S3C2410/S3C2440 has the on chip touchscreen
> controller based on it's analog to digital converter(8-channel analog
> inputs, touchscreen uses 4 of them). This driver uses the exsiting S3C
> ADC driver to make the touchscreen controller of S3C2410/S3C2440 work
> well together with other ADC devices connected to the S3C24XX SoC chip.
>
> The patch is created against kernel 2.6.32-rc4 and tested with S3C2440
> SoC and Samsung LTE430WQ-F0C touchscreen.
>
>
> Signed-off-by: Shine Liu<liuxian@redflag-linux.com>
> Signed-off-by: Shine Liu<shinel@foxmail.com>
>    

Do you know that there's another patch (at Openmoko) created by Nelson 
Castillo that does the same, but also has support for kernel-space 
touchscreen filters? (I think [1] is his latest version)
I don't know how your patch performs, but according to [2] the filters 
should help a lot avoiding jitter etc.

I'm not sure whether Nelson has submitted his patches for mainline 
review yet and what the status is on the kernel filters, but IMHO doing 
some filtering in kernel space (see the "Why are we doing filtering in 
kernel space?" part of [2]) which results in a "cleaner" output is 
preferred over reporting possible "jittery" data.

Regards,
Maurus Cuelenaere


[1] 
http://nelson-patches.googlecode.com/svn/trunk/openmoko/kernel/ts-with-s3c-adc/
[2] http://wiki.openmoko.org/wiki/Touchscreen_Filters

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Russell King - ARM Linux]

On Mon, Oct 19, 2009 at 01:28:13PM +0200, Maurus Cuelenaere wrote:
> Do you know that there's another patch (at Openmoko) created by Nelson  
> Castillo that does the same, but also has support for kernel-space  
> touchscreen filters? (I think [1] is his latest version)
> I don't know how your patch performs, but according to [2] the filters  
> should help a lot avoiding jitter etc.
>
> I'm not sure whether Nelson has submitted his patches for mainline  
> review yet and what the status is on the kernel filters, but IMHO doing  
> some filtering in kernel space (see the "Why are we doing filtering in  
> kernel space?" part of [2]) which results in a "cleaner" output is  
> preferred over reporting possible "jittery" data.

It doesn't really describe why doing the filtering in kernel space is
apparantly soo much better than doing it in tslib - it's seems to just
do some handwaving kind of argument:

   Let's say we would like to deliver a TS event to user space each 10
   milliseconds. In the GTA02 with the current configuration the
   Analog/Digital conversion time of a sample is 0.4697 milliseconds, thus
   can get 18 samples for each event that we send to user-space. Sending the
   event (with 18 samples) takes us about 8.45 milliseconds.

So far so good.  But, according to my maths, 8.45ms is less than 10ms.

                                                               Sometimes we
   even decide that the event should not be sent to user-space (because the
   hardware didn't provide reliable data), and our tests show that it's the
   right thing to do. In previous versions of the driver light taps would
   confuse the driver (that would report bad clicks) and this is no longer an
   issue.

Err, that doesn't seem to follow on from the previous point.

The reason that we decided raw events should be passed to userspace and
the processing done there is that it allows all of the _policy_ of
deciding how to process touchscreen events to be configured.  There's
lots of different parameters and ways to filter touchscreens, and some
are specific to individual touchscreen setups.

This is why tslib is entirely modular, and allows any combination of
modules to be loaded to process the touchscreen samples - it's there
to provide a totally flexible infrastructure to filter and scale
the output from the touchscreen in whatever way is required for the
hardware.

I don't think there is any single filtering system which can properly
filter the output of any one touchscreen, and I don't think that putting
this filtering in the kernel is a sane approach.  It _may_ be a sane
approach for one particular touchscreen setup, but it certainly isn't
for all.

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Russell King - ARM Linux]

On Mon, Oct 19, 2009 at 01:28:13PM +0200, Maurus Cuelenaere wrote:
> Do you know that there's another patch (at Openmoko) created by Nelson  
> Castillo that does the same, but also has support for kernel-space  
> touchscreen filters? (I think [1] is his latest version)
> I don't know how your patch performs, but according to [2] the filters  
> should help a lot avoiding jitter etc.
>
> I'm not sure whether Nelson has submitted his patches for mainline  
> review yet and what the status is on the kernel filters, but IMHO doing  
> some filtering in kernel space (see the "Why are we doing filtering in  
> kernel space?" part of [2]) which results in a "cleaner" output is  
> preferred over reporting possible "jittery" data.

It doesn't really describe why doing the filtering in kernel space is
apparantly soo much better than doing it in tslib - it's seems to just
do some handwaving kind of argument:

   Let's say we would like to deliver a TS event to user space each 10
   milliseconds. In the GTA02 with the current configuration the
   Analog/Digital conversion time of a sample is 0.4697 milliseconds, thus
   can get 18 samples for each event that we send to user-space. Sending the
   event (with 18 samples) takes us about 8.45 milliseconds.

So far so good.  But, according to my maths, 8.45ms is less than 10ms.

                                                               Sometimes we
   even decide that the event should not be sent to user-space (because the
   hardware didn't provide reliable data), and our tests show that it's the
   right thing to do. In previous versions of the driver light taps would
   confuse the driver (that would report bad clicks) and this is no longer an
   issue.

Err, that doesn't seem to follow on from the previous point.

The reason that we decided raw events should be passed to userspace and
the processing done there is that it allows all of the _policy_ of
deciding how to process touchscreen events to be configured.  There's
lots of different parameters and ways to filter touchscreens, and some
are specific to individual touchscreen setups.

This is why tslib is entirely modular, and allows any combination of
modules to be loaded to process the touchscreen samples - it's there
to provide a totally flexible infrastructure to filter and scale
the output from the touchscreen in whatever way is required for the
hardware.

I don't think there is any single filtering system which can properly
filter the output of any one touchscreen, and I don't think that putting
this filtering in the kernel is a sane approach.  It _may_ be a sane
approach for one particular touchscreen setup, but it certainly isn't
for all.

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Shine Liu]

On Mon, 2009-10-19 at 13:28 +0200, Maurus Cuelenaere wrote:

> Do you know that there's another patch (at Openmoko) created by Nelson 
> Castillo that does the same, but also has support for kernel-space 
> touchscreen filters? (I think [1] is his latest version)
> I don't know how your patch performs, but according to [2] the filters 
> should help a lot avoiding jitter etc.
> 
> I'm not sure whether Nelson has submitted his patches for mainline 
> review yet and what the status is on the kernel filters, but IMHO doing 
> some filtering in kernel space (see the "Why are we doing filtering in 
> kernel space?" part of [2]) which results in a "cleaner" output is 
> preferred over reporting possible "jittery" data.

Yes, I do,  because part of my patch comes from source code of openmoko
project. I've also noticed there's 4 touchscreen filters in the openmoko
project used by s3c24xx touchscreen driver.

I think there should be a s3c24xx touchscreen driver in the mailine, but
I haven't found anyone submitted a s3c24xx touchscreen driver for the
mainline. So I wrote the driver myself and refered the openmoko
implementation.

I didn't use the kernel-space touchscreen filters, because I found tslib
is able to meet the common requirement.

I have read the mail of Russell King. I think we may add the s3c24xx ts
driver first, and the kernel-space touchscreen filters can be added
later if most of us think it is valuable or usefull. After all, I don't
think there is anyone like the mailine without s3c24xx ts driver
support.


Cheers,

Shine Liu

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Shine Liu]

On Mon, 2009-10-19 at 13:28 +0200, Maurus Cuelenaere wrote:

> Do you know that there's another patch (at Openmoko) created by Nelson 
> Castillo that does the same, but also has support for kernel-space 
> touchscreen filters? (I think [1] is his latest version)
> I don't know how your patch performs, but according to [2] the filters 
> should help a lot avoiding jitter etc.
> 
> I'm not sure whether Nelson has submitted his patches for mainline 
> review yet and what the status is on the kernel filters, but IMHO doing 
> some filtering in kernel space (see the "Why are we doing filtering in 
> kernel space?" part of [2]) which results in a "cleaner" output is 
> preferred over reporting possible "jittery" data.

Yes, I do,  because part of my patch comes from source code of openmoko
project. I've also noticed there's 4 touchscreen filters in the openmoko
project used by s3c24xx touchscreen driver.

I think there should be a s3c24xx touchscreen driver in the mailine, but
I haven't found anyone submitted a s3c24xx touchscreen driver for the
mainline. So I wrote the driver myself and refered the openmoko
implementation.

I didn't use the kernel-space touchscreen filters, because I found tslib
is able to meet the common requirement.

I have read the mail of Russell King. I think we may add the s3c24xx ts
driver first, and the kernel-space touchscreen filters can be added
later if most of us think it is valuable or usefull. After all, I don't
think there is anyone like the mailine without s3c24xx ts driver
support.


Cheers,

Shine Liu

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Juergen Beisert]

On Montag, 19. Oktober 2009, Maurus Cuelenaere wrote:
> Op 19-10-09 12:55, linux-arm-kernel-request@lists.infradead.org schreef:
> > This touchscreen driver is for touchscreen controller on Samsung
> > S3C2410/S3C2440 SoC chip. S3C2410/S3C2440 has the on chip touchscreen
> > controller based on it's analog to digital converter(8-channel analog
> > inputs, touchscreen uses 4 of them). This driver uses the exsiting S3C
> > ADC driver to make the touchscreen controller of S3C2410/S3C2440 work
> > well together with other ADC devices connected to the S3C24XX SoC chip.
> >
> > The patch is created against kernel 2.6.32-rc4 and tested with S3C2440
> > SoC and Samsung LTE430WQ-F0C touchscreen.
> >
> >
> > Signed-off-by: Shine Liu<liuxian@redflag-linux.com>
> > Signed-off-by: Shine Liu<shinel@foxmail.com>
>
> Do you know that there's another patch (at Openmoko) created by Nelson
> Castillo that does the same, but also has support for kernel-space
> touchscreen filters? (I think [1] is his latest version)
> I don't know how your patch performs, but according to [2] the filters
> should help a lot avoiding jitter etc.
>
> I'm not sure whether Nelson has submitted his patches for mainline
> review yet and what the status is on the kernel filters, but IMHO doing
> some filtering in kernel space (see the "Why are we doing filtering in
> kernel space?" part of [2]) which results in a "cleaner" output is
> preferred over reporting possible "jittery" data.

Filtering can be done perfectly in userland (like all the other ts do). IMHO I 
can't see any need to do it in kernel space.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Juergen Beisert]

On Montag, 19. Oktober 2009, Maurus Cuelenaere wrote:
> Op 19-10-09 12:55, linux-arm-kernel-request at lists.infradead.org schreef:
> > This touchscreen driver is for touchscreen controller on Samsung
> > S3C2410/S3C2440 SoC chip. S3C2410/S3C2440 has the on chip touchscreen
> > controller based on it's analog to digital converter(8-channel analog
> > inputs, touchscreen uses 4 of them). This driver uses the exsiting S3C
> > ADC driver to make the touchscreen controller of S3C2410/S3C2440 work
> > well together with other ADC devices connected to the S3C24XX SoC chip.
> >
> > The patch is created against kernel 2.6.32-rc4 and tested with S3C2440
> > SoC and Samsung LTE430WQ-F0C touchscreen.
> >
> >
> > Signed-off-by: Shine Liu<liuxian@redflag-linux.com>
> > Signed-off-by: Shine Liu<shinel@foxmail.com>
>
> Do you know that there's another patch (at Openmoko) created by Nelson
> Castillo that does the same, but also has support for kernel-space
> touchscreen filters? (I think [1] is his latest version)
> I don't know how your patch performs, but according to [2] the filters
> should help a lot avoiding jitter etc.
>
> I'm not sure whether Nelson has submitted his patches for mainline
> review yet and what the status is on the kernel filters, but IMHO doing
> some filtering in kernel space (see the "Why are we doing filtering in
> kernel space?" part of [2]) which results in a "cleaner" output is
> preferred over reporting possible "jittery" data.

Filtering can be done perfectly in userland (like all the other ts do). IMHO I 
can't see any need to do it in kernel space.

jbe

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | Phone: +49-8766-939 228     |
Vertretung Sued/Muenchen, Germany             | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686              | http://www.pengutronix.de/  |

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Arnaud Patard]

Shine Liu <shinel@foxmail.com> writes:

Hi,

> On Mon, 2009-10-19 at 13:28 +0200, Maurus Cuelenaere wrote:
>
>> Do you know that there's another patch (at Openmoko) created by Nelson 
>> Castillo that does the same, but also has support for kernel-space 
>> touchscreen filters? (I think [1] is his latest version)
>> I don't know how your patch performs, but according to [2] the filters 
>> should help a lot avoiding jitter etc.
>> 
>> I'm not sure whether Nelson has submitted his patches for mainline 
>> review yet and what the status is on the kernel filters, but IMHO doing 
>> some filtering in kernel space (see the "Why are we doing filtering in 
>> kernel space?" part of [2]) which results in a "cleaner" output is 
>> preferred over reporting possible "jittery" data.
>
> Yes, I do,  because part of my patch comes from source code of openmoko
> project. I've also noticed there's 4 touchscreen filters in the openmoko
> project used by s3c24xx touchscreen driver.
>
> I think there should be a s3c24xx touchscreen driver in the mailine, but
> I haven't found anyone submitted a s3c24xx touchscreen driver for the
> mainline. So I wrote the driver myself and refered the openmoko
> implementation.

Please have a look at the arm-kernel mailing list archive. There has
been some discussion about that. The idea was to compare between the
openmoko's version and Simtec's version and choose the best version. Both
are coming from the h1940 tree (fwiw written by me and it would have
been nice to tell in your header you've taken the code from openmoko's
tree) and both have been updated to take into account the adc support
given by arch/arm/plat-s3c24xx/adc.c. 

As regards why there's no driver yet, there's a reason. The adc
stuff didn't exist when the driver was written and it was not a good
idea to merge it because enabling it was meaning no hwmon support. Same
problem if the hwmon support was enabled, no touchscreen support. So, it
was decided to wait for a proper adc support. Now that Ben wrote it,
it's time to discuss about merging it.

So, please have a look at the driver which was posted by Ben some days
ago and comments/reply to his mail if you have problem with his
version. The driver has been in Simtec's patchset since quite a long
time so I tend to believe that this one is working nicely.

Thanks,
Arnaud

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Arnaud Patard (Rtp)]

Shine Liu <shinel@foxmail.com> writes:

Hi,

> On Mon, 2009-10-19 at 13:28 +0200, Maurus Cuelenaere wrote:
>
>> Do you know that there's another patch (at Openmoko) created by Nelson 
>> Castillo that does the same, but also has support for kernel-space 
>> touchscreen filters? (I think [1] is his latest version)
>> I don't know how your patch performs, but according to [2] the filters 
>> should help a lot avoiding jitter etc.
>> 
>> I'm not sure whether Nelson has submitted his patches for mainline 
>> review yet and what the status is on the kernel filters, but IMHO doing 
>> some filtering in kernel space (see the "Why are we doing filtering in 
>> kernel space?" part of [2]) which results in a "cleaner" output is 
>> preferred over reporting possible "jittery" data.
>
> Yes, I do,  because part of my patch comes from source code of openmoko
> project. I've also noticed there's 4 touchscreen filters in the openmoko
> project used by s3c24xx touchscreen driver.
>
> I think there should be a s3c24xx touchscreen driver in the mailine, but
> I haven't found anyone submitted a s3c24xx touchscreen driver for the
> mainline. So I wrote the driver myself and refered the openmoko
> implementation.

Please have a look at the arm-kernel mailing list archive. There has
been some discussion about that. The idea was to compare between the
openmoko's version and Simtec's version and choose the best version. Both
are coming from the h1940 tree (fwiw written by me and it would have
been nice to tell in your header you've taken the code from openmoko's
tree) and both have been updated to take into account the adc support
given by arch/arm/plat-s3c24xx/adc.c. 

As regards why there's no driver yet, there's a reason. The adc
stuff didn't exist when the driver was written and it was not a good
idea to merge it because enabling it was meaning no hwmon support. Same
problem if the hwmon support was enabled, no touchscreen support. So, it
was decided to wait for a proper adc support. Now that Ben wrote it,
it's time to discuss about merging it.

So, please have a look at the driver which was posted by Ben some days
ago and comments/reply to his mail if you have problem with his
version. The driver has been in Simtec's patchset since quite a long
time so I tend to believe that this one is working nicely.

Thanks,
Arnaud

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Nelson Castillo]

On Mon, Oct 19, 2009 at 7:07 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Oct 19, 2009 at 01:28:13PM +0200, Maurus Cuelenaere wrote:
>> Do you know that there's another patch (at Openmoko) created by Nelson
>> Castillo that does the same, but also has support for kernel-space
>> touchscreen filters? (I think [1] is his latest version)
>> I don't know how your patch performs, but according to [2] the filters
>> should help a lot avoiding jitter etc.
>> I'm not sure whether Nelson has submitted his patches for mainline
>> review yet and what the status is on the kernel filters, but IMHO doing
>> some filtering in kernel space (see the "Why are we doing filtering in
>> kernel space?" part of [2]) which results in a "cleaner" output is
>> preferred over reporting possible "jittery" data.

I did submit the filter stuff once and I applied ALL the upstream
feedback we got from Andrew M.

> It doesn't really describe why doing the filtering in kernel space is
> apparantly soo much better than doing it in tslib - it's seems to just
> do some handwaving kind of argument:

Some drivers do some kind of filtering. Even the other one for this
chip submitted to linux-arm-kernel a few days ago.

>   Let's say we would like to deliver a TS event to user space each 10
>   milliseconds. In the GTA02 with the current configuration the
>   Analog/Digital conversion time of a sample is 0.4697 milliseconds, thus
>   can get 18 samples for each event that we send to user-space. Sending the
>   event (with 18 samples) takes us about 8.45 milliseconds.
>
> So far so good.  But, according to my maths, 8.45ms is less than 10ms.

In practice the event is generated each 10ms because of the jiffies
resolution and the use of a timer but this might not be true if
someone uses the filters with a different driver/configuration.

>                                                               Sometimes we
>   even decide that the event should not be sent to user-space (because the
>   hardware didn't provide reliable data), and our tests show that it's the
>   right thing to do. In previous versions of the driver light taps would
>   confuse the driver (that would report bad clicks) and this is no longer an
>   issue.
>
> Err, that doesn't seem to follow on from the previous point.
>
> The reason that we decided raw events should be passed to userspace and
> the processing done there is that it allows all of the _policy_ of
> deciding how to process touchscreen events to be configured.  There's
> lots of different parameters and ways to filter touchscreens, and some
> are specific to individual touchscreen setups.

Yes, ignoring the event might not be the best thing to do if this is a policy.

"Giving up" could be implemented as sending a point of bad quality
instead of ignoring the event. It's better to implement "I give up
trying to get a better point for you" instead of "I driver decide you
should never know someone touched the screen because it was all
noise".

> This is why tslib is entirely modular, and allows any combination of
> modules to be loaded to process the touchscreen samples - it's there
> to provide a totally flexible infrastructure to filter and scale
> the output from the touchscreen in whatever way is required for the
> hardware.

There is one case where "whatever way" fails here. If you get a set of
points and you notice they have noise you can schedule more
conversions _right_away_ without latency to get new points from the
ADC. That's what the "group filter" does and you can not do that from
tslib and I say it after learning how to write tslib filters.

Most people who hear about the filters blindly say "this should be
done in user-space" without considering this fact.

> I don't think there is any single filtering system which can properly
> filter the output of any one touchscreen, and I don't think that putting
> this filtering in the kernel is a sane approach.  It _may_ be a sane
> approach for one particular touchscreen setup, but it certainly isn't
> for all.

I've found the filters to be useful and I would like to see them
upstream but not if people don't find them to be useful.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Nelson Castillo]

On Mon, Oct 19, 2009 at 7:07 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Oct 19, 2009 at 01:28:13PM +0200, Maurus Cuelenaere wrote:
>> Do you know that there's another patch (at Openmoko) created by Nelson
>> Castillo that does the same, but also has support for kernel-space
>> touchscreen filters? (I think [1] is his latest version)
>> I don't know how your patch performs, but according to [2] the filters
>> should help a lot avoiding jitter etc.
>> I'm not sure whether Nelson has submitted his patches for mainline
>> review yet and what the status is on the kernel filters, but IMHO doing
>> some filtering in kernel space (see the "Why are we doing filtering in
>> kernel space?" part of [2]) which results in a "cleaner" output is
>> preferred over reporting possible "jittery" data.

I did submit the filter stuff once and I applied ALL the upstream
feedback we got from Andrew M.

> It doesn't really describe why doing the filtering in kernel space is
> apparantly soo much better than doing it in tslib - it's seems to just
> do some handwaving kind of argument:

Some drivers do some kind of filtering. Even the other one for this
chip submitted to linux-arm-kernel a few days ago.

> ? Let's say we would like to deliver a TS event to user space each 10
> ? milliseconds. In the GTA02 with the current configuration the
> ? Analog/Digital conversion time of a sample is 0.4697 milliseconds, thus
> ? can get 18 samples for each event that we send to user-space. Sending the
> ? event (with 18 samples) takes us about 8.45 milliseconds.
>
> So far so good. ?But, according to my maths, 8.45ms is less than 10ms.

In practice the event is generated each 10ms because of the jiffies
resolution and the use of a timer but this might not be true if
someone uses the filters with a different driver/configuration.

> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? Sometimes we
> ? even decide that the event should not be sent to user-space (because the
> ? hardware didn't provide reliable data), and our tests show that it's the
> ? right thing to do. In previous versions of the driver light taps would
> ? confuse the driver (that would report bad clicks) and this is no longer an
> ? issue.
>
> Err, that doesn't seem to follow on from the previous point.
>
> The reason that we decided raw events should be passed to userspace and
> the processing done there is that it allows all of the _policy_ of
> deciding how to process touchscreen events to be configured. ?There's
> lots of different parameters and ways to filter touchscreens, and some
> are specific to individual touchscreen setups.

Yes, ignoring the event might not be the best thing to do if this is a policy.

"Giving up" could be implemented as sending a point of bad quality
instead of ignoring the event. It's better to implement "I give up
trying to get a better point for you" instead of "I driver decide you
should never know someone touched the screen because it was all
noise".

> This is why tslib is entirely modular, and allows any combination of
> modules to be loaded to process the touchscreen samples - it's there
> to provide a totally flexible infrastructure to filter and scale
> the output from the touchscreen in whatever way is required for the
> hardware.

There is one case where "whatever way" fails here. If you get a set of
points and you notice they have noise you can schedule more
conversions _right_away_ without latency to get new points from the
ADC. That's what the "group filter" does and you can not do that from
tslib and I say it after learning how to write tslib filters.

Most people who hear about the filters blindly say "this should be
done in user-space" without considering this fact.

> I don't think there is any single filtering system which can properly
> filter the output of any one touchscreen, and I don't think that putting
> this filtering in the kernel is a sane approach. ?It _may_ be a sane
> approach for one particular touchscreen setup, but it certainly isn't
> for all.

I've found the filters to be useful and I would like to see them
upstream but not if people don't find them to be useful.

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Russell King - ARM Linux]

On Mon, Oct 19, 2009 at 11:21:40PM -0500, Nelson Castillo wrote:
> There is one case where "whatever way" fails here. If you get a set of
> points and you notice they have noise you can schedule more
> conversions _right_away_ without latency to get new points from the
> ADC. That's what the "group filter" does and you can not do that from
> tslib and I say it after learning how to write tslib filters.

In which case people aren't implementing their kernel drivers correctly.
The kernel is supposed to produce a stream of samples when the pen is
touched, and those samples should be delivered to tslib.  tslib's filters
then has the responsibility to read groups of samples, and filter them
using whatever method that the user wants.

I also don't buy your comparison on your web page since there's almost
zero details of what you're comparing.  You don't give details of what
filters you're using with the userspace method - for all the reader
knows, you're not using tslib at all.

> Most people who hear about the filters blindly say "this should be
> done in user-space" without considering this fact.

I'm the original author of tslib.  Don't think for a moment that I haven't
any experience in this area.

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Russell King - ARM Linux]

On Mon, Oct 19, 2009 at 11:21:40PM -0500, Nelson Castillo wrote:
> There is one case where "whatever way" fails here. If you get a set of
> points and you notice they have noise you can schedule more
> conversions _right_away_ without latency to get new points from the
> ADC. That's what the "group filter" does and you can not do that from
> tslib and I say it after learning how to write tslib filters.

In which case people aren't implementing their kernel drivers correctly.
The kernel is supposed to produce a stream of samples when the pen is
touched, and those samples should be delivered to tslib.  tslib's filters
then has the responsibility to read groups of samples, and filter them
using whatever method that the user wants.

I also don't buy your comparison on your web page since there's almost
zero details of what you're comparing.  You don't give details of what
filters you're using with the userspace method - for all the reader
knows, you're not using tslib at all.

> Most people who hear about the filters blindly say "this should be
> done in user-space" without considering this fact.

I'm the original author of tslib.  Don't think for a moment that I haven't
any experience in this area.

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Nelson Castillo]

On Tue, Oct 20, 2009 at 2:39 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Oct 19, 2009 at 11:21:40PM -0500, Nelson Castillo wrote:
>> There is one case where "whatever way" fails here. If you get a set of
>> points and you notice they have noise you can schedule more
>> conversions _right_away_ without latency to get new points from the
>> ADC. That's what the "group filter" does and you can not do that from
>> tslib and I say it after learning how to write tslib filters.
>
> In which case people aren't implementing their kernel drivers correctly.
> The kernel is supposed to produce a stream of samples when the pen is
> touched, and those samples should be delivered to tslib.  tslib's filters
> then has the responsibility to read groups of samples, and filter them
> using whatever method that the user wants.

The group filter (that mixes information from X and Y) could be ported
to user-space but we would need points at a much higher rate.

Can the s3c driver send the events at full speed? (Maximum possible ADC rate?).

Even if it's possible I think we would monopolize the adc driver
needlessly because we don't need all that conversions when there is
not much noise. Please read on.

> I also don't buy your comparison on your web page since there's almost
> zero details of what you're comparing.  You don't give details of what
> filters you're using with the userspace method - for all the reader
> knows, you're not using tslib at all.

Blame lack of time.

Most of the time tslib filtering worked well when we have good pressure.

We have problems when we don't have much pressure and in this case
there is a lot of noise and the events that are reported by the driver
are actually noise. This noise would be more common at pen-down before
the adc readings settle and at pen-up.

This is the noise we filtered in most cases.

I remember I wrote a tslib filter to skip the N first and the M last
points but it was not enough.

http://svn.arhuaco.org/svn/src/openmoko/touchscreen/tslib-skip_module.patch

I should report some data here so that you can see how bad the noise
problem is. It might be a hardware problem. I've heard of other boards
that also have a lot of noise.

I did read this message last month in tslib:

https://lists.berlios.de/pipermail/tslib-general/2009-September/000228.html

  I've taken the tack that we when we use the resistive
  sensors we have to take sloppy positioning and mis-clicks
  into the user interface design.

Really? We managed to fix a similar problem. Perhaps it can also be
fixed with tslib but we tried and we could not. This was the last
message in the thread so I took it as a conclusion.

>> Most people who hear about the filters blindly say "this should be
>> done in user-space" without considering this fact.
>
> I'm the original author of tslib.  Don't think for a moment that I haven't
> any experience in this area.

I already knew, and I know, I know :-)

Perhaps I need to replace talk with data but I don't really feel like
doing it now. I could do it later.

If everyone solves the noise problem in resistive touchscreens with
tslib then there is no need for in-kernel filtering. But then I see
the reply I quoted from the tslib ML where it is stated that one has
to learn to live with the "sloppy positioning and mis-clicks". I don't
think this is acceptable if there is a solution.

As you wrote before it will not work in all cases, but I've seen it's
working for a few s3c-based projects.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Nelson Castillo]

On Tue, Oct 20, 2009 at 2:39 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Oct 19, 2009 at 11:21:40PM -0500, Nelson Castillo wrote:
>> There is one case where "whatever way" fails here. If you get a set of
>> points and you notice they have noise you can schedule more
>> conversions _right_away_ without latency to get new points from the
>> ADC. That's what the "group filter" does and you can not do that from
>> tslib and I say it after learning how to write tslib filters.
>
> In which case people aren't implementing their kernel drivers correctly.
> The kernel is supposed to produce a stream of samples when the pen is
> touched, and those samples should be delivered to tslib. ?tslib's filters
> then has the responsibility to read groups of samples, and filter them
> using whatever method that the user wants.

The group filter (that mixes information from X and Y) could be ported
to user-space but we would need points at a much higher rate.

Can the s3c driver send the events at full speed? (Maximum possible ADC rate?).

Even if it's possible I think we would monopolize the adc driver
needlessly because we don't need all that conversions when there is
not much noise. Please read on.

> I also don't buy your comparison on your web page since there's almost
> zero details of what you're comparing. ?You don't give details of what
> filters you're using with the userspace method - for all the reader
> knows, you're not using tslib at all.

Blame lack of time.

Most of the time tslib filtering worked well when we have good pressure.

We have problems when we don't have much pressure and in this case
there is a lot of noise and the events that are reported by the driver
are actually noise. This noise would be more common at pen-down before
the adc readings settle and at pen-up.

This is the noise we filtered in most cases.

I remember I wrote a tslib filter to skip the N first and the M last
points but it was not enough.

http://svn.arhuaco.org/svn/src/openmoko/touchscreen/tslib-skip_module.patch

I should report some data here so that you can see how bad the noise
problem is. It might be a hardware problem. I've heard of other boards
that also have a lot of noise.

I did read this message last month in tslib:

https://lists.berlios.de/pipermail/tslib-general/2009-September/000228.html

  I've taken the tack that we when we use the resistive
  sensors we have to take sloppy positioning and mis-clicks
  into the user interface design.

Really? We managed to fix a similar problem. Perhaps it can also be
fixed with tslib but we tried and we could not. This was the last
message in the thread so I took it as a conclusion.

>> Most people who hear about the filters blindly say "this should be
>> done in user-space" without considering this fact.
>
> I'm the original author of tslib. ?Don't think for a moment that I haven't
> any experience in this area.

I already knew, and I know, I know :-)

Perhaps I need to replace talk with data but I don't really feel like
doing it now. I could do it later.

If everyone solves the noise problem in resistive touchscreens with
tslib then there is no need for in-kernel filtering. But then I see
the reply I quoted from the tslib ML where it is stated that one has
to learn to live with the "sloppy positioning and mis-clicks". I don't
think this is acceptable if there is a solution.

As you wrote before it will not work in all cases, but I've seen it's
working for a few s3c-based projects.

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Mark Brown]

On Tue, Oct 20, 2009 at 03:21:17AM -0500, Nelson Castillo wrote:

> Can the s3c driver send the events at full speed? (Maximum possible ADC rate?).

> Even if it's possible I think we would monopolize the adc driver
> needlessly because we don't need all that conversions when there is
> not much noise. Please read on.

There's currently no facility to push sample rate requests down into the
kernel from userspace.  However, this is a good idea in general - there
was some recent (brief) discussion of doing this on the linux-input list.
Since different use cases have different sample rate requirements (such
as the difference between handwriting recognition which can need 200
samples/second and a simple finger operated menu which may be happy with
10) there's value in being able to vary the sample rate at run time.  I
was intending to look at this shortly as part of some touchscreen driver
development I need to do soon or perhaps someone else will pick up the
idea, if it does get implemented then obviously tslib would also be able
to use it here.

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Mark Brown]

On Tue, Oct 20, 2009 at 03:21:17AM -0500, Nelson Castillo wrote:

> Can the s3c driver send the events at full speed? (Maximum possible ADC rate?).

> Even if it's possible I think we would monopolize the adc driver
> needlessly because we don't need all that conversions when there is
> not much noise. Please read on.

There's currently no facility to push sample rate requests down into the
kernel from userspace.  However, this is a good idea in general - there
was some recent (brief) discussion of doing this on the linux-input list.
Since different use cases have different sample rate requirements (such
as the difference between handwriting recognition which can need 200
samples/second and a simple finger operated menu which may be happy with
10) there's value in being able to vary the sample rate at run time.  I
was intending to look at this shortly as part of some touchscreen driver
development I need to do soon or perhaps someone else will pick up the
idea, if it does get implemented then obviously tslib would also be able
to use it here.

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Andy Green]

On 10/20/09 09:21, Somebody in the thread at some point said:

> We have problems when we don't have much pressure and in this case
> there is a lot of noise and the events that are reported by the driver
> are actually noise. This noise would be more common at pen-down before
> the adc readings settle and at pen-up.

Hi Nelson -

Just a FYI, on the board I have been working on there is a capacitive 
sensor, AD7174.  I wrote a driver for it which again uses the in-kernel 
median filter (the original Openmoko version I wrote chopped out at the 
moment) heavily and with really excellent results.  The median filters 
are pretty much like magic for certain common kinds of noise.

However, I just drop the median filter code into my tree in 
drivers/input/misc at the moment.  What would be really a good result I 
think is if the filters turned up in ./lib in an easily reusable way.

I think maybe it's a mistake to focus on the in-kernel filters being 
good for a particular touchscreen technology or even specifically 
touchscreens.  I am sure other drivers doing something completely 
different with noisy instantaneously sampled data, that won't fit into 
tslib worldview at all, can also benefit from being able to clean up 
data in a standardized way before passing it down as an input event or 
back to the device or whatever.  And once that's possible, touchscreen 
drivers of any sort have the standardized option to use it too if they 
have a reason.

Just a thought, good luck :-)

-Andy

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Andy Green]

On 10/20/09 09:21, Somebody in the thread at some point said:

> We have problems when we don't have much pressure and in this case
> there is a lot of noise and the events that are reported by the driver
> are actually noise. This noise would be more common at pen-down before
> the adc readings settle and at pen-up.

Hi Nelson -

Just a FYI, on the board I have been working on there is a capacitive 
sensor, AD7174.  I wrote a driver for it which again uses the in-kernel 
median filter (the original Openmoko version I wrote chopped out at the 
moment) heavily and with really excellent results.  The median filters 
are pretty much like magic for certain common kinds of noise.

However, I just drop the median filter code into my tree in 
drivers/input/misc at the moment.  What would be really a good result I 
think is if the filters turned up in ./lib in an easily reusable way.

I think maybe it's a mistake to focus on the in-kernel filters being 
good for a particular touchscreen technology or even specifically 
touchscreens.  I am sure other drivers doing something completely 
different with noisy instantaneously sampled data, that won't fit into 
tslib worldview at all, can also benefit from being able to clean up 
data in a standardized way before passing it down as an input event or 
back to the device or whatever.  And once that's possible, touchscreen 
drivers of any sort have the standardized option to use it too if they 
have a reason.

Just a thought, good luck :-)

-Andy

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Nelson Castillo]

On Tue, Oct 20, 2009 at 4:41 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Oct 20, 2009 at 03:21:17AM -0500, Nelson Castillo wrote:
>
>> Can the s3c driver send the events at full speed? (Maximum possible ADC rate?).

Yes, but in packets I think (The way I would know how to implement it).
We could try sending 2000 events per second to user-space and see what
happens :-) :-)

>> Even if it's possible I think we would monopolize the adc driver
>> needlessly because we don't need all that conversions when there is
>> not much noise. Please read on.
>
> There's currently no facility to push sample rate requests down into the
> kernel from userspace.  However, this is a good idea in general - there
> was some recent (brief) discussion of doing this on the linux-input list.

I see. It makes sense.

ATM one big difference with the kernel approach is that in the kernel
you can request the adc conversions on demand.

> Since different use cases have different sample rate requirements (such
> as the difference between handwriting recognition which can need 200
> samples/second and a simple finger operated menu which may be happy with
> 10) there's value in being able to vary the sample rate at run time.  I
> was intending to look at this shortly as part of some touchscreen driver
> development I need to do soon or perhaps someone else will pick up the
> idea, if it does get implemented then obviously tslib would also be able
> to use it here.

With a lot more points you can indeed do better filtering in users-pace.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Nelson Castillo]

On Tue, Oct 20, 2009 at 4:41 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Oct 20, 2009 at 03:21:17AM -0500, Nelson Castillo wrote:
>
>> Can the s3c driver send the events at full speed? (Maximum possible ADC rate?).

Yes, but in packets I think (The way I would know how to implement it).
We could try sending 2000 events per second to user-space and see what
happens :-) :-)

>> Even if it's possible I think we would monopolize the adc driver
>> needlessly because we don't need all that conversions when there is
>> not much noise. Please read on.
>
> There's currently no facility to push sample rate requests down into the
> kernel from userspace. ?However, this is a good idea in general - there
> was some recent (brief) discussion of doing this on the linux-input list.

I see. It makes sense.

ATM one big difference with the kernel approach is that in the kernel
you can request the adc conversions on demand.

> Since different use cases have different sample rate requirements (such
> as the difference between handwriting recognition which can need 200
> samples/second and a simple finger operated menu which may be happy with
> 10) there's value in being able to vary the sample rate at run time. ?I
> was intending to look at this shortly as part of some touchscreen driver
> development I need to do soon or perhaps someone else will pick up the
> idea, if it does get implemented then obviously tslib would also be able
> to use it here.

With a lot more points you can indeed do better filtering in users-pace.

Re: [PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Dmitry Torokhov]

Hi Shine,

On Mon, Oct 19, 2009 at 06:54:21PM +0800, Shine Liu wrote:
> 
> This touchscreen driver is for touchscreen controller on Samsung
> S3C2410/S3C2440 SoC chip. S3C2410/S3C2440 has the on chip touchscreen
> controller based on it's analog to digital converter(8-channel analog
> inputs, touchscreen uses 4 of them). This driver uses the exsiting S3C
> ADC driver to make the touchscreen controller of S3C2410/S3C2440 work
> well together with other ADC devices connected to the S3C24XX SoC chip.
> 
> The patch is created against kernel 2.6.32-rc4 and tested with S3C2440
> SoC and Samsung LTE430WQ-F0C touchscreen.

So I understand there are questions whether this verson of driver should
be merged, I'll let you guys workit it out. I think that filtering is
better be left off to tslib since there are many more opportunities to
do this kind of stuff in userspace.

Still, below some comments so that the next iteration of the driver does
not have the same faults ;)

> 
> Signed-off-by: Shine Liu <liuxian@redflag-linux.com>
> Signed-off-by: Shine Liu <shinel@foxmail.com>

Umm, is it the same person? Then one sign-off is enough ;)

> --------------------------------------------------------
> 
> --- a/drivers/input/touchscreen/Makefile	2009-10-12 05:43:56.000000000 +0800
> +++ b/drivers/input/touchscreen/Makefile	2009-10-13 20:35:02.000000000 +0800
> @@ -10,6 +10,7 @@
>  obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
>  obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
>  obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC)	+= atmel_tsadcc.o
> +obj-$(CONFIG_TOUCHSCREEN_S3C24XX_TSADCC)       += s3c24xx_tsadcc.o
>  obj-$(CONFIG_TOUCHSCREEN_BITSY)		+= h3600_ts_input.o
>  obj-$(CONFIG_TOUCHSCREEN_CORGI)		+= corgi_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_GUNZE)		+= gunze.o
> --- a/drivers/input/touchscreen/Kconfig	2009-10-12 05:43:56.000000000 +0800
> +++ b/drivers/input/touchscreen/Kconfig	2009-10-13 20:37:51.000000000 +0800
> @@ -307,6 +307,19 @@
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called atmel_tsadcc.
>  
> +config TOUCHSCREEN_S3C24XX_TSADCC
> +	tristate "Samsung S3C24XX touchscreen input driver"
> +	depends on ARCH_S3C2410 && S3C24XX_ADC && INPUT && INPUT_TOUCHSCREEN
> +	select SERIO

Why do you need SERIO? I don't see it being a serio driver.

> +	help
> +	  Say Y here if you have a touchscreen connected to the ADC Controller
> +	  on your s3c2410/s3c2440 SoC.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called s3c24xx_tsadcc.
> +
>  config TOUCHSCREEN_UCB1400
>  	tristate "Philips UCB1400 touchscreen"
>  	depends on AC97_BUS
> --- /dev/null	2009-10-09 10:57:12.360986601 +0800
> +++ b/drivers/input/touchscreen/s3c24xx_tsadcc.c	2009-10-19 17:16:57.000000000 +0800
> @@ -0,0 +1,432 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Part of the code come from openmoko project.
> + *
> + * Shine Liu <shinel@foxmail.com>
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/input.h>
> +#include <linux/init.h>
> +#include <linux/serio.h>

NO need to include this.

> +#include <linux/timer.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <asm/irq.h>
> +
> +#include <mach/gpio.h>
> +#include <mach/regs-gpio.h>
> +#include <mach/hardware.h>
> +#include <plat/regs-adc.h>
> +#include <plat/adc.h>
> +
> +
> +MODULE_AUTHOR("Shine Liu <shinel@foxmail.com>");
> +MODULE_DESCRIPTION("Samsung s3c24xx touchscreen driver");
> +MODULE_LICENSE("GPL");
> +
> +
> +#define S3C24XX_TS_VERSION	0x0101
> +
> +#define TSC_SLEEP (S3C2410_ADCTSC_PULL_UP_DISABLE | \
> +	S3C2410_ADCTSC_XY_PST(0))
> +
> +#define WAIT4INT(x)	(((x)<<8) | \
> +	S3C2410_ADCTSC_YM_SEN | \
> +	S3C2410_ADCTSC_YP_SEN | \
> +	S3C2410_ADCTSC_XP_SEN | \
> +	S3C2410_ADCTSC_XY_PST(3))
> +
> +#define AUTO_XY	(S3C2410_ADCTSC_PULL_UP_DISABLE | \
> +	S3C2410_ADCTSC_AUTO_PST | \
> +	S3C2410_ADCTSC_XY_PST(0))
> +
> +#define TS_STATE_STANDBY 0 /* initial state */
> +#define TS_STATE_PRESSED 1
> +#define TS_STATE_RELEASE_PENDING 2 /* wait to confirm the up event */
> +#define TS_STATE_RELEASE 3
> +
> +#define TIME_RELEASE_PENDING (HZ / 20) /* about 50ms */
> +#define TIME_NEXT_CONV ((HZ < 51)? 1 : HZ / 50) /* about 20ms */
> +
> +static char *s3c24xx_ts_name = "Samsung s3c24xx TouchScreen";
> +static void __iomem *base_addr;

No chance of a second device? I'd put it in s3c24xx_ts anyway.

> +
> +
> +/*
> + * Per-touchscreen data.
> + */
> +
> +struct s3c24xx_ts {
> +	struct input_dev *dev;
> +	struct s3c_adc_client *adc_client;
> +	unsigned char is_down;
> +	unsigned char state;
> +	unsigned adc_selected;
> +	unsigned int delay;	/* register value for delay */
> +	unsigned int presc;	/* register value for prescaler */
> +};
> +
> +static struct s3c24xx_ts ts;

Better use platform_set_drvdata and platform_get_drvdata instead of a
global.

> +
> +
> +/*
> + * Low level functions to config registers.
> + */
> +static inline void s3c24xx_ts_connect(void)
> +{
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(12), S3C2410_GPG12_XMON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(13), S3C2410_GPG13_nXPON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(14), S3C2410_GPG14_YMON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(15), S3C2410_GPG15_nYPON);
> +}
> +
> +static void s3c24xx_ts_start_adc_conversion(void)
> +{
> +	pr_debug("start_adc_conv ADCTSC: 0x%x", 
> +		readl(base_addr + S3C2410_ADCTSC));
> +	writel(AUTO_XY, base_addr + S3C2410_ADCTSC);
> +	pr_debug(" ---> 0x%x", 
> +		readl(base_addr + S3C2410_ADCTSC));
> +	s3c_adc_start(ts.adc_client, 0, 1);
> +	pr_debug(" ---> 0x%x\n", 
> +		readl(base_addr + S3C2410_ADCTSC));
> +}
> +
> +
> +/*
> + * Event handling
> + */
> +
> +enum ts_input_event {IE_UP = 0, IE_DOWN};
> +
> +static void ts_input_report(int event, int coords[])
> +{
> +
> +	if (event == IE_DOWN) {
> +		input_report_abs(ts.dev, ABS_X, coords[0]);
> +		input_report_abs(ts.dev, ABS_Y, coords[1]);
> +		input_report_key(ts.dev, BTN_TOUCH, 1);
> +		input_report_abs(ts.dev, ABS_PRESSURE, 1);

No fake pressure events please.

> +
> +		pr_debug("down (X:%03d, Y:%03d)\n", coords[0], coords[1]);

Maybe use dev_dbg()?

> +	} else {
> +		input_report_key(ts.dev, BTN_TOUCH, 0);
> +		input_report_abs(ts.dev, ABS_PRESSURE, 0);
> +		pr_debug("up\n");
> +	}
> +
> +	input_sync(ts.dev);
> +}
> +
> +
> +/*
> + * Timer to process the UP event or to report the postion of DRAG
> + */
> +
> +static void ts_drag_pull_timer_f(unsigned long data);
> +static struct timer_list ts_drag_pull_timer = 
> +	TIMER_INITIALIZER(ts_drag_pull_timer_f, 0, 0);

Move it to the device structure as well.

> +
> +static void ts_drag_pull_timer_f(unsigned long data)
> +{
> +	unsigned long flags;
> +	/* delay reporting the UP event to avoid jitter */
> +	static unsigned release_pending_counter = 0;
> +	pr_debug("Timer called: is_down[%d] status[%d]\n", ts.is_down, ts.state);
> +
> +	local_irq_save(flags);

Is it really needed?

> +	if(ts.state == TS_STATE_RELEASE_PENDING) {
> +		if(release_pending_counter++ < 1) {
> +			/* jitter avoidance window: delay TIME_RELEASE_PENDING jfs */
> +			mod_timer(&ts_drag_pull_timer, jiffies + TIME_RELEASE_PENDING);
> +		} else {
> +			/* no down event occurd during last delay */
> +			release_pending_counter = 0;
> +			ts_input_report(IE_UP, NULL);
> +			ts.state = TS_STATE_RELEASE;
> +			writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
> +		}
> +		local_irq_restore(flags);
> +		return;
> +	}
> +
> +	/* ts.is_down should be true here */
> +	s3c24xx_ts_start_adc_conversion();
> +	local_irq_restore(flags);
> +}
> +
> +
> +/*
> + * ISR for the IRQ_TS
> + */
> +
> +static irqreturn_t stylus_updown(int irq, void *dev_id)
> +{
> +	unsigned long data0;
> +	unsigned long data1;
> +	unsigned long adctsc;
> +	int event_type;		/* 1 for down, 0 for up */
> +
> +	/* 
> +	 * According to s3c2440 manual chap16: After Touch Screen 
> +	 * Controller generates INT_TC, XY_PST must be set to [00]
> +	 */
> +	adctsc = readl(base_addr + S3C2410_ADCTSC);
> +	writel(adctsc & ~(S3C2410_ADCTSC_XY_PST(3)), base_addr + S3C2410_ADCTSC);
> +	data0 = readl(base_addr + S3C2410_ADCDAT0);
> +	data1 = readl(base_addr + S3C2410_ADCDAT1);
> +	event_type = (!(data0 & S3C2410_ADCDAT0_UPDOWN)) &&
> +					    (!(data1 & S3C2410_ADCDAT0_UPDOWN));
> +	pr_debug("event_type: %d, DATA0 0x%lx, DATA1 0x%lx, ADCTSC 0x%lx, "
> +		"ADCUPDN 0x%x\n", event_type, data0, data1,
> +		adctsc, readl(base_addr + 0x14));
> +
> +	/* ignore sequential same event */
> +	if(ts.is_down == event_type) {
> +		pr_debug("###Ignore same event: %d\n", event_type);
> +		/* restore XY_PST */
> +		writel(adctsc, base_addr + S3C2410_ADCTSC);
> +		return IRQ_HANDLED;
> +	}
> +
> +	ts.is_down = event_type;
> +	if (ts.is_down) {
> +		s3c24xx_ts_start_adc_conversion();
> +	} else {
> +		ts.state = TS_STATE_RELEASE_PENDING;
> +		/*
> +		 * Delay to report the up event for a while to avoid jitter.
> +		 * This state will be checked after (0, TIME_NEXT_CONV)
> +		 * jiffies depended on when the interrupt occured.
> +		 */
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* 
> + * Called in ISR of IRQ_ADC
> + */
> +
> +static void stylus_adc_action(struct s3c_adc_client *client,
> +		unsigned p0, unsigned p1, unsigned *conv_left)
> +{
> +	int buf[2];
> +	unsigned long flags;
> +
> +	local_irq_save(flags);

Why?

> +	/* 
> +	 * IRQ_TS may rise just in the time window between AUTO_XY mode
> +	 * set and this pointer */
> +	if(ts.state == TS_STATE_RELEASE_PENDING)
> +	{
> +		local_irq_restore(flags);
> +		return;
> +	}
> +
> +	/* Grab the ADC results. */
> +	buf[0] = p0;
> +	buf[1] = p1;
> +
> +	ts_input_report(IE_DOWN, buf);
> +	pr_debug("[adc_selected: %d] ADCTSC: 0x%x", 
> +		ts.adc_selected, readl(base_addr + S3C2410_ADCTSC));
> +
> +	writel(WAIT4INT(1), base_addr + S3C2410_ADCTSC);
> +	printk(" ---> 0x%x\n", 
> +		readl(base_addr + S3C2410_ADCTSC));
> +
> +	ts.state = TS_STATE_PRESSED;
> +	mod_timer(&ts_drag_pull_timer, jiffies + TIME_NEXT_CONV);
> +	local_irq_restore(flags);
> +}
> +
> +
> +/* 
> + * callback function for s3c-adc
> + */
> +
> +void adc_selected_f(struct s3c_adc_client *client, unsigned selected)
> +{
> +	ts.adc_selected = selected;
> +}
> +
> +
> +/*
> + * The functions for inserting/removing us as a module.
> + */
> +
> +static int __init s3c24xx_ts_probe(struct platform_device *pdev)

__devinit.

> +{
> +	int rc;
> +	struct input_dev *input_dev;
> +	int ret = 0;
> +
> +	dev_info(&pdev->dev, "Starting\n");
> +	pr_debug("Entering s3c24xx_ts_probe\n");
> +
> +	base_addr = ioremap(S3C2410_PA_ADC, 0x20);
> +	if (base_addr == NULL) {
> +		dev_err(&pdev->dev, "Failed to remap register block\n");
> +		ret = -ENOMEM;
> +		goto fail0;
> +	}
> +
> +	/* Configure GPIOs */
> +	s3c24xx_ts_connect();
> +
> +	writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
> +
> +	/* Initialise input stuff */
> +	memset(&ts, 0, sizeof(struct s3c24xx_ts));
> +	ts.adc_client =
> +		s3c_adc_register(pdev, adc_selected_f, stylus_adc_action, 1);
> +	if (!ts.adc_client) {
> +			dev_err(&pdev->dev,
> +					"Unable to register s3c24xx_ts as s3c_adc client\n");
> +			ret = -EIO;
> +			goto fail1;

-EWIERDIDENTATION.

> +	}
> +
> +	/* save prescale EN and VAL in the S3C2410_ADCCON register */
> +	ts.presc = readl(base_addr + S3C2410_ADCCON);
> +	/* save value of S3C2410_ADCDLY register */
> +	ts.delay = readl(base_addr + S3C2410_ADCDLY);
> +	pr_debug("platform data: ADCCON=%08x ADCDLY=%08x\n", ts.presc, ts.delay);
> +
> +	input_dev = input_allocate_device();
> +	if (!input_dev) {
> +		dev_err(&pdev->dev, "Unable to allocate the input device\n");
> +		ret = -ENOMEM;
> +		goto fail2;
> +	}
> +
> +	ts.dev = input_dev;
> +	ts.dev->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	ts.dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +	input_set_abs_params(ts.dev, ABS_X, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.dev, ABS_Y, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.dev, ABS_PRESSURE, 0, 1, 0, 0);

Kill the above line.

> +
> +	ts.dev->name = s3c24xx_ts_name;
> +	ts.dev->id.bustype = BUS_RS232;

Really?

> +	ts.dev->id.vendor = 0xBAAD;
> +	ts.dev->id.product = 0xBEEF;
> +	ts.dev->id.version = S3C24XX_TS_VERSION;
> +	ts.state = TS_STATE_STANDBY;
> +
> +	/* Get IRQ */
> +	if (request_irq(IRQ_TC, stylus_updown, IRQF_SAMPLE_RANDOM,
> +			"s3c24xx_ts_action", ts.dev)) {
> +		dev_err(&pdev->dev, "Could not allocate ts IRQ_TC !\n");
> +		ret = -EIO;
> +		goto fail3;
> +	}
> +
> +	dev_info(&pdev->dev, "Successfully loaded\n");

No need.

> +
> +	/* All went ok, so register to the input system */
> +	rc = input_register_device(ts.dev);
> +	if (rc) {
> +		ret = -EIO;
> +		goto fail4;
> +	}
> +	return 0;
> +
> +fail4:
> +	disable_irq(IRQ_TC);

No need.

> +	free_irq(IRQ_TC, ts.dev);
> +fail3:
> +	input_free_device(ts.dev);
> +fail2:	
> +	s3c_adc_release(ts.adc_client);
> +fail1:
> +	iounmap(base_addr);
> +fail0:
> +	return ret;
> +}
> +
> +static int s3c24xx_ts_remove(struct platform_device *pdev)

__devexit.

> +{
> +	disable_irq(IRQ_TC);

No need.

> +	free_irq(IRQ_TC, ts.dev);
> +	input_unregister_device(ts.dev);
> +	input_free_device(ts.dev);

No free after unregister, input_dev is refcounted.

> +	s3c_adc_release(ts.adc_client);
> +	iounmap(base_addr);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int s3c24xx_ts_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	disable_irq(IRQ_TC);
> +	ts.presc = readl(base_addr + S3C2410_ADCCON);
> +	ts.delay = readl(base_addr + S3C2410_ADCDLY);
> +
> +	writel(TSC_SLEEP, base_addr + S3C2410_ADCTSC);
> +	writel(ts.presc | S3C2410_ADCCON_STDBM, base_addr + S3C2410_ADCCON);
> +	return 0;
> +}
> +
> +static int s3c24xx_ts_resume(struct platform_device *pdev)
> +{
> +	/* Restore registers */
> +	writel(ts.presc, base_addr + S3C2410_ADCCON);
> +	writel(ts.delay, base_addr + S3C2410_ADCDLY);
> +	writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
> +	enable_irq(IRQ_TC);
> +	return 0;
> +}
> +
> +#else
> +#define s3c24xx_ts_suspend NULL
> +#define s3c24xx_ts_resume  NULL
> +#endif
> +
> +static struct platform_driver s3c24xx_ts_driver = {
> +	.driver		= {
> +		.name	= "s3c24xx-ts",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= s3c24xx_ts_probe,
> +	.remove		= s3c24xx_ts_remove,

__devexit_p().

> +	.suspend 	= s3c24xx_ts_suspend,

This should be converted to dev_pm_ops.

> +	.resume		= s3c24xx_ts_resume,
> +};
> +
> +static int __init s3c24xx_ts_init(void)
> +{
> +	int rc;
> +
> +	rc = platform_driver_register(&s3c24xx_ts_driver);
> +	return rc;

Just do "return platform_driver_register();"

> +}
> +
> +static void __exit s3c24xx_ts_exit(void)
> +{
> +	platform_driver_unregister(&s3c24xx_ts_driver);
> +}
> +
> +module_init(s3c24xx_ts_init);
> +module_exit(s3c24xx_ts_exit);
> +

Thanks.

-- 
Dmitry

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Dmitry Torokhov]

Hi Shine,

On Mon, Oct 19, 2009 at 06:54:21PM +0800, Shine Liu wrote:
> 
> This touchscreen driver is for touchscreen controller on Samsung
> S3C2410/S3C2440 SoC chip. S3C2410/S3C2440 has the on chip touchscreen
> controller based on it's analog to digital converter(8-channel analog
> inputs, touchscreen uses 4 of them). This driver uses the exsiting S3C
> ADC driver to make the touchscreen controller of S3C2410/S3C2440 work
> well together with other ADC devices connected to the S3C24XX SoC chip.
> 
> The patch is created against kernel 2.6.32-rc4 and tested with S3C2440
> SoC and Samsung LTE430WQ-F0C touchscreen.

So I understand there are questions whether this verson of driver should
be merged, I'll let you guys workit it out. I think that filtering is
better be left off to tslib since there are many more opportunities to
do this kind of stuff in userspace.

Still, below some comments so that the next iteration of the driver does
not have the same faults ;)

> 
> Signed-off-by: Shine Liu <liuxian@redflag-linux.com>
> Signed-off-by: Shine Liu <shinel@foxmail.com>

Umm, is it the same person? Then one sign-off is enough ;)

> --------------------------------------------------------
> 
> --- a/drivers/input/touchscreen/Makefile	2009-10-12 05:43:56.000000000 +0800
> +++ b/drivers/input/touchscreen/Makefile	2009-10-13 20:35:02.000000000 +0800
> @@ -10,6 +10,7 @@
>  obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
>  obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
>  obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC)	+= atmel_tsadcc.o
> +obj-$(CONFIG_TOUCHSCREEN_S3C24XX_TSADCC)       += s3c24xx_tsadcc.o
>  obj-$(CONFIG_TOUCHSCREEN_BITSY)		+= h3600_ts_input.o
>  obj-$(CONFIG_TOUCHSCREEN_CORGI)		+= corgi_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_GUNZE)		+= gunze.o
> --- a/drivers/input/touchscreen/Kconfig	2009-10-12 05:43:56.000000000 +0800
> +++ b/drivers/input/touchscreen/Kconfig	2009-10-13 20:37:51.000000000 +0800
> @@ -307,6 +307,19 @@
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called atmel_tsadcc.
>  
> +config TOUCHSCREEN_S3C24XX_TSADCC
> +	tristate "Samsung S3C24XX touchscreen input driver"
> +	depends on ARCH_S3C2410 && S3C24XX_ADC && INPUT && INPUT_TOUCHSCREEN
> +	select SERIO

Why do you need SERIO? I don't see it being a serio driver.

> +	help
> +	  Say Y here if you have a touchscreen connected to the ADC Controller
> +	  on your s3c2410/s3c2440 SoC.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called s3c24xx_tsadcc.
> +
>  config TOUCHSCREEN_UCB1400
>  	tristate "Philips UCB1400 touchscreen"
>  	depends on AC97_BUS
> --- /dev/null	2009-10-09 10:57:12.360986601 +0800
> +++ b/drivers/input/touchscreen/s3c24xx_tsadcc.c	2009-10-19 17:16:57.000000000 +0800
> @@ -0,0 +1,432 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Part of the code come from openmoko project.
> + *
> + * Shine Liu <shinel@foxmail.com>
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/input.h>
> +#include <linux/init.h>
> +#include <linux/serio.h>

NO need to include this.

> +#include <linux/timer.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <asm/irq.h>
> +
> +#include <mach/gpio.h>
> +#include <mach/regs-gpio.h>
> +#include <mach/hardware.h>
> +#include <plat/regs-adc.h>
> +#include <plat/adc.h>
> +
> +
> +MODULE_AUTHOR("Shine Liu <shinel@foxmail.com>");
> +MODULE_DESCRIPTION("Samsung s3c24xx touchscreen driver");
> +MODULE_LICENSE("GPL");
> +
> +
> +#define S3C24XX_TS_VERSION	0x0101
> +
> +#define TSC_SLEEP (S3C2410_ADCTSC_PULL_UP_DISABLE | \
> +	S3C2410_ADCTSC_XY_PST(0))
> +
> +#define WAIT4INT(x)	(((x)<<8) | \
> +	S3C2410_ADCTSC_YM_SEN | \
> +	S3C2410_ADCTSC_YP_SEN | \
> +	S3C2410_ADCTSC_XP_SEN | \
> +	S3C2410_ADCTSC_XY_PST(3))
> +
> +#define AUTO_XY	(S3C2410_ADCTSC_PULL_UP_DISABLE | \
> +	S3C2410_ADCTSC_AUTO_PST | \
> +	S3C2410_ADCTSC_XY_PST(0))
> +
> +#define TS_STATE_STANDBY 0 /* initial state */
> +#define TS_STATE_PRESSED 1
> +#define TS_STATE_RELEASE_PENDING 2 /* wait to confirm the up event */
> +#define TS_STATE_RELEASE 3
> +
> +#define TIME_RELEASE_PENDING (HZ / 20) /* about 50ms */
> +#define TIME_NEXT_CONV ((HZ < 51)? 1 : HZ / 50) /* about 20ms */
> +
> +static char *s3c24xx_ts_name = "Samsung s3c24xx TouchScreen";
> +static void __iomem *base_addr;

No chance of a second device? I'd put it in s3c24xx_ts anyway.

> +
> +
> +/*
> + * Per-touchscreen data.
> + */
> +
> +struct s3c24xx_ts {
> +	struct input_dev *dev;
> +	struct s3c_adc_client *adc_client;
> +	unsigned char is_down;
> +	unsigned char state;
> +	unsigned adc_selected;
> +	unsigned int delay;	/* register value for delay */
> +	unsigned int presc;	/* register value for prescaler */
> +};
> +
> +static struct s3c24xx_ts ts;

Better use platform_set_drvdata and platform_get_drvdata instead of a
global.

> +
> +
> +/*
> + * Low level functions to config registers.
> + */
> +static inline void s3c24xx_ts_connect(void)
> +{
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(12), S3C2410_GPG12_XMON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(13), S3C2410_GPG13_nXPON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(14), S3C2410_GPG14_YMON);
> +	s3c2410_gpio_cfgpin(S3C2410_GPG(15), S3C2410_GPG15_nYPON);
> +}
> +
> +static void s3c24xx_ts_start_adc_conversion(void)
> +{
> +	pr_debug("start_adc_conv ADCTSC: 0x%x", 
> +		readl(base_addr + S3C2410_ADCTSC));
> +	writel(AUTO_XY, base_addr + S3C2410_ADCTSC);
> +	pr_debug(" ---> 0x%x", 
> +		readl(base_addr + S3C2410_ADCTSC));
> +	s3c_adc_start(ts.adc_client, 0, 1);
> +	pr_debug(" ---> 0x%x\n", 
> +		readl(base_addr + S3C2410_ADCTSC));
> +}
> +
> +
> +/*
> + * Event handling
> + */
> +
> +enum ts_input_event {IE_UP = 0, IE_DOWN};
> +
> +static void ts_input_report(int event, int coords[])
> +{
> +
> +	if (event == IE_DOWN) {
> +		input_report_abs(ts.dev, ABS_X, coords[0]);
> +		input_report_abs(ts.dev, ABS_Y, coords[1]);
> +		input_report_key(ts.dev, BTN_TOUCH, 1);
> +		input_report_abs(ts.dev, ABS_PRESSURE, 1);

No fake pressure events please.

> +
> +		pr_debug("down (X:%03d, Y:%03d)\n", coords[0], coords[1]);

Maybe use dev_dbg()?

> +	} else {
> +		input_report_key(ts.dev, BTN_TOUCH, 0);
> +		input_report_abs(ts.dev, ABS_PRESSURE, 0);
> +		pr_debug("up\n");
> +	}
> +
> +	input_sync(ts.dev);
> +}
> +
> +
> +/*
> + * Timer to process the UP event or to report the postion of DRAG
> + */
> +
> +static void ts_drag_pull_timer_f(unsigned long data);
> +static struct timer_list ts_drag_pull_timer = 
> +	TIMER_INITIALIZER(ts_drag_pull_timer_f, 0, 0);

Move it to the device structure as well.

> +
> +static void ts_drag_pull_timer_f(unsigned long data)
> +{
> +	unsigned long flags;
> +	/* delay reporting the UP event to avoid jitter */
> +	static unsigned release_pending_counter = 0;
> +	pr_debug("Timer called: is_down[%d] status[%d]\n", ts.is_down, ts.state);
> +
> +	local_irq_save(flags);

Is it really needed?

> +	if(ts.state == TS_STATE_RELEASE_PENDING) {
> +		if(release_pending_counter++ < 1) {
> +			/* jitter avoidance window: delay TIME_RELEASE_PENDING jfs */
> +			mod_timer(&ts_drag_pull_timer, jiffies + TIME_RELEASE_PENDING);
> +		} else {
> +			/* no down event occurd during last delay */
> +			release_pending_counter = 0;
> +			ts_input_report(IE_UP, NULL);
> +			ts.state = TS_STATE_RELEASE;
> +			writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
> +		}
> +		local_irq_restore(flags);
> +		return;
> +	}
> +
> +	/* ts.is_down should be true here */
> +	s3c24xx_ts_start_adc_conversion();
> +	local_irq_restore(flags);
> +}
> +
> +
> +/*
> + * ISR for the IRQ_TS
> + */
> +
> +static irqreturn_t stylus_updown(int irq, void *dev_id)
> +{
> +	unsigned long data0;
> +	unsigned long data1;
> +	unsigned long adctsc;
> +	int event_type;		/* 1 for down, 0 for up */
> +
> +	/* 
> +	 * According to s3c2440 manual chap16: After Touch Screen 
> +	 * Controller generates INT_TC, XY_PST must be set to [00]
> +	 */
> +	adctsc = readl(base_addr + S3C2410_ADCTSC);
> +	writel(adctsc & ~(S3C2410_ADCTSC_XY_PST(3)), base_addr + S3C2410_ADCTSC);
> +	data0 = readl(base_addr + S3C2410_ADCDAT0);
> +	data1 = readl(base_addr + S3C2410_ADCDAT1);
> +	event_type = (!(data0 & S3C2410_ADCDAT0_UPDOWN)) &&
> +					    (!(data1 & S3C2410_ADCDAT0_UPDOWN));
> +	pr_debug("event_type: %d, DATA0 0x%lx, DATA1 0x%lx, ADCTSC 0x%lx, "
> +		"ADCUPDN 0x%x\n", event_type, data0, data1,
> +		adctsc, readl(base_addr + 0x14));
> +
> +	/* ignore sequential same event */
> +	if(ts.is_down == event_type) {
> +		pr_debug("###Ignore same event: %d\n", event_type);
> +		/* restore XY_PST */
> +		writel(adctsc, base_addr + S3C2410_ADCTSC);
> +		return IRQ_HANDLED;
> +	}
> +
> +	ts.is_down = event_type;
> +	if (ts.is_down) {
> +		s3c24xx_ts_start_adc_conversion();
> +	} else {
> +		ts.state = TS_STATE_RELEASE_PENDING;
> +		/*
> +		 * Delay to report the up event for a while to avoid jitter.
> +		 * This state will be checked after (0, TIME_NEXT_CONV)
> +		 * jiffies depended on when the interrupt occured.
> +		 */
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* 
> + * Called in ISR of IRQ_ADC
> + */
> +
> +static void stylus_adc_action(struct s3c_adc_client *client,
> +		unsigned p0, unsigned p1, unsigned *conv_left)
> +{
> +	int buf[2];
> +	unsigned long flags;
> +
> +	local_irq_save(flags);

Why?

> +	/* 
> +	 * IRQ_TS may rise just in the time window between AUTO_XY mode
> +	 * set and this pointer */
> +	if(ts.state == TS_STATE_RELEASE_PENDING)
> +	{
> +		local_irq_restore(flags);
> +		return;
> +	}
> +
> +	/* Grab the ADC results. */
> +	buf[0] = p0;
> +	buf[1] = p1;
> +
> +	ts_input_report(IE_DOWN, buf);
> +	pr_debug("[adc_selected: %d] ADCTSC: 0x%x", 
> +		ts.adc_selected, readl(base_addr + S3C2410_ADCTSC));
> +
> +	writel(WAIT4INT(1), base_addr + S3C2410_ADCTSC);
> +	printk(" ---> 0x%x\n", 
> +		readl(base_addr + S3C2410_ADCTSC));
> +
> +	ts.state = TS_STATE_PRESSED;
> +	mod_timer(&ts_drag_pull_timer, jiffies + TIME_NEXT_CONV);
> +	local_irq_restore(flags);
> +}
> +
> +
> +/* 
> + * callback function for s3c-adc
> + */
> +
> +void adc_selected_f(struct s3c_adc_client *client, unsigned selected)
> +{
> +	ts.adc_selected = selected;
> +}
> +
> +
> +/*
> + * The functions for inserting/removing us as a module.
> + */
> +
> +static int __init s3c24xx_ts_probe(struct platform_device *pdev)

__devinit.

> +{
> +	int rc;
> +	struct input_dev *input_dev;
> +	int ret = 0;
> +
> +	dev_info(&pdev->dev, "Starting\n");
> +	pr_debug("Entering s3c24xx_ts_probe\n");
> +
> +	base_addr = ioremap(S3C2410_PA_ADC, 0x20);
> +	if (base_addr == NULL) {
> +		dev_err(&pdev->dev, "Failed to remap register block\n");
> +		ret = -ENOMEM;
> +		goto fail0;
> +	}
> +
> +	/* Configure GPIOs */
> +	s3c24xx_ts_connect();
> +
> +	writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
> +
> +	/* Initialise input stuff */
> +	memset(&ts, 0, sizeof(struct s3c24xx_ts));
> +	ts.adc_client =
> +		s3c_adc_register(pdev, adc_selected_f, stylus_adc_action, 1);
> +	if (!ts.adc_client) {
> +			dev_err(&pdev->dev,
> +					"Unable to register s3c24xx_ts as s3c_adc client\n");
> +			ret = -EIO;
> +			goto fail1;

-EWIERDIDENTATION.

> +	}
> +
> +	/* save prescale EN and VAL in the S3C2410_ADCCON register */
> +	ts.presc = readl(base_addr + S3C2410_ADCCON);
> +	/* save value of S3C2410_ADCDLY register */
> +	ts.delay = readl(base_addr + S3C2410_ADCDLY);
> +	pr_debug("platform data: ADCCON=%08x ADCDLY=%08x\n", ts.presc, ts.delay);
> +
> +	input_dev = input_allocate_device();
> +	if (!input_dev) {
> +		dev_err(&pdev->dev, "Unable to allocate the input device\n");
> +		ret = -ENOMEM;
> +		goto fail2;
> +	}
> +
> +	ts.dev = input_dev;
> +	ts.dev->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	ts.dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +	input_set_abs_params(ts.dev, ABS_X, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.dev, ABS_Y, 0, 0x3FF, 0, 0);
> +	input_set_abs_params(ts.dev, ABS_PRESSURE, 0, 1, 0, 0);

Kill the above line.

> +
> +	ts.dev->name = s3c24xx_ts_name;
> +	ts.dev->id.bustype = BUS_RS232;

Really?

> +	ts.dev->id.vendor = 0xBAAD;
> +	ts.dev->id.product = 0xBEEF;
> +	ts.dev->id.version = S3C24XX_TS_VERSION;
> +	ts.state = TS_STATE_STANDBY;
> +
> +	/* Get IRQ */
> +	if (request_irq(IRQ_TC, stylus_updown, IRQF_SAMPLE_RANDOM,
> +			"s3c24xx_ts_action", ts.dev)) {
> +		dev_err(&pdev->dev, "Could not allocate ts IRQ_TC !\n");
> +		ret = -EIO;
> +		goto fail3;
> +	}
> +
> +	dev_info(&pdev->dev, "Successfully loaded\n");

No need.

> +
> +	/* All went ok, so register to the input system */
> +	rc = input_register_device(ts.dev);
> +	if (rc) {
> +		ret = -EIO;
> +		goto fail4;
> +	}
> +	return 0;
> +
> +fail4:
> +	disable_irq(IRQ_TC);

No need.

> +	free_irq(IRQ_TC, ts.dev);
> +fail3:
> +	input_free_device(ts.dev);
> +fail2:	
> +	s3c_adc_release(ts.adc_client);
> +fail1:
> +	iounmap(base_addr);
> +fail0:
> +	return ret;
> +}
> +
> +static int s3c24xx_ts_remove(struct platform_device *pdev)

__devexit.

> +{
> +	disable_irq(IRQ_TC);

No need.

> +	free_irq(IRQ_TC, ts.dev);
> +	input_unregister_device(ts.dev);
> +	input_free_device(ts.dev);

No free after unregister, input_dev is refcounted.

> +	s3c_adc_release(ts.adc_client);
> +	iounmap(base_addr);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int s3c24xx_ts_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	disable_irq(IRQ_TC);
> +	ts.presc = readl(base_addr + S3C2410_ADCCON);
> +	ts.delay = readl(base_addr + S3C2410_ADCDLY);
> +
> +	writel(TSC_SLEEP, base_addr + S3C2410_ADCTSC);
> +	writel(ts.presc | S3C2410_ADCCON_STDBM, base_addr + S3C2410_ADCCON);
> +	return 0;
> +}
> +
> +static int s3c24xx_ts_resume(struct platform_device *pdev)
> +{
> +	/* Restore registers */
> +	writel(ts.presc, base_addr + S3C2410_ADCCON);
> +	writel(ts.delay, base_addr + S3C2410_ADCDLY);
> +	writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
> +	enable_irq(IRQ_TC);
> +	return 0;
> +}
> +
> +#else
> +#define s3c24xx_ts_suspend NULL
> +#define s3c24xx_ts_resume  NULL
> +#endif
> +
> +static struct platform_driver s3c24xx_ts_driver = {
> +	.driver		= {
> +		.name	= "s3c24xx-ts",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= s3c24xx_ts_probe,
> +	.remove		= s3c24xx_ts_remove,

__devexit_p().

> +	.suspend 	= s3c24xx_ts_suspend,

This should be converted to dev_pm_ops.

> +	.resume		= s3c24xx_ts_resume,
> +};
> +
> +static int __init s3c24xx_ts_init(void)
> +{
> +	int rc;
> +
> +	rc = platform_driver_register(&s3c24xx_ts_driver);
> +	return rc;

Just do "return platform_driver_register();"

> +}
> +
> +static void __exit s3c24xx_ts_exit(void)
> +{
> +	platform_driver_unregister(&s3c24xx_ts_driver);
> +}
> +
> +module_init(s3c24xx_ts_init);
> +module_exit(s3c24xx_ts_exit);
> +

Thanks.

-- 
Dmitry

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Shine Liu]

This touchscreen driver is for touchscreen controller on Samsung
S3C2410/S3C2440 SoC chip. S3C2410/S3C2440 has the on chip touchscreen
controller based on it's analog to digital converter(8-channel analog
inputs, touchscreen uses 4 of them). This driver uses the exsiting S3C
ADC driver to make the touchscreen controller of S3C2410/S3C2440 work
well together with other ADC devices connected to the S3C24XX SoC chip.

The patch is created against kernel 2.6.32-rc4 and tested with S3C2440
SoC and Samsung LTE430WQ-F0C touchscreen.


Signed-off-by: Shine Liu <liuxian@redflag-linux.com>
Signed-off-by: Shine Liu <shinel@foxmail.com>
--------------------------------------------------------

--- a/drivers/input/touchscreen/Makefile	2009-10-12 05:43:56.000000000 +0800
+++ b/drivers/input/touchscreen/Makefile	2009-10-13 20:35:02.000000000 +0800
@@ -10,6 +10,7 @@
 obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
 obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
 obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC)	+= atmel_tsadcc.o
+obj-$(CONFIG_TOUCHSCREEN_S3C24XX_TSADCC)       += s3c24xx_tsadcc.o
 obj-$(CONFIG_TOUCHSCREEN_BITSY)		+= h3600_ts_input.o
 obj-$(CONFIG_TOUCHSCREEN_CORGI)		+= corgi_ts.o
 obj-$(CONFIG_TOUCHSCREEN_GUNZE)		+= gunze.o
--- a/drivers/input/touchscreen/Kconfig	2009-10-12 05:43:56.000000000 +0800
+++ b/drivers/input/touchscreen/Kconfig	2009-10-13 20:37:51.000000000 +0800
@@ -307,6 +307,19 @@
 	  To compile this driver as a module, choose M here: the
 	  module will be called atmel_tsadcc.
 
+config TOUCHSCREEN_S3C24XX_TSADCC
+	tristate "Samsung S3C24XX touchscreen input driver"
+	depends on ARCH_S3C2410 && S3C24XX_ADC && INPUT && INPUT_TOUCHSCREEN
+	select SERIO
+	help
+	  Say Y here if you have a touchscreen connected to the ADC Controller
+	  on your s3c2410/s3c2440 SoC.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called s3c24xx_tsadcc.
+
 config TOUCHSCREEN_UCB1400
 	tristate "Philips UCB1400 touchscreen"
 	depends on AC97_BUS
--- /dev/null	2009-10-09 10:57:12.360986601 +0800
+++ b/drivers/input/touchscreen/s3c24xx_tsadcc.c	2009-10-19 17:16:57.000000000 +0800
@@ -0,0 +1,432 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Part of the code come from openmoko project.
+ *
+ * Shine Liu <shinel@foxmail.com>
+ */
+
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/input.h>
+#include <linux/init.h>
+#include <linux/serio.h>
+#include <linux/timer.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <asm/irq.h>
+
+#include <mach/gpio.h>
+#include <mach/regs-gpio.h>
+#include <mach/hardware.h>
+#include <plat/regs-adc.h>
+#include <plat/adc.h>
+
+
+MODULE_AUTHOR("Shine Liu <shinel@foxmail.com>");
+MODULE_DESCRIPTION("Samsung s3c24xx touchscreen driver");
+MODULE_LICENSE("GPL");
+
+
+#define S3C24XX_TS_VERSION	0x0101
+
+#define TSC_SLEEP (S3C2410_ADCTSC_PULL_UP_DISABLE | \
+	S3C2410_ADCTSC_XY_PST(0))
+
+#define WAIT4INT(x)	(((x)<<8) | \
+	S3C2410_ADCTSC_YM_SEN | \
+	S3C2410_ADCTSC_YP_SEN | \
+	S3C2410_ADCTSC_XP_SEN | \
+	S3C2410_ADCTSC_XY_PST(3))
+
+#define AUTO_XY	(S3C2410_ADCTSC_PULL_UP_DISABLE | \
+	S3C2410_ADCTSC_AUTO_PST | \
+	S3C2410_ADCTSC_XY_PST(0))
+
+#define TS_STATE_STANDBY 0 /* initial state */
+#define TS_STATE_PRESSED 1
+#define TS_STATE_RELEASE_PENDING 2 /* wait to confirm the up event */
+#define TS_STATE_RELEASE 3
+
+#define TIME_RELEASE_PENDING (HZ / 20) /* about 50ms */
+#define TIME_NEXT_CONV ((HZ < 51)? 1 : HZ / 50) /* about 20ms */
+
+static char *s3c24xx_ts_name = "Samsung s3c24xx TouchScreen";
+static void __iomem *base_addr;
+
+
+/*
+ * Per-touchscreen data.
+ */
+
+struct s3c24xx_ts {
+	struct input_dev *dev;
+	struct s3c_adc_client *adc_client;
+	unsigned char is_down;
+	unsigned char state;
+	unsigned adc_selected;
+	unsigned int delay;	/* register value for delay */
+	unsigned int presc;	/* register value for prescaler */
+};
+
+static struct s3c24xx_ts ts;
+
+
+/*
+ * Low level functions to config registers.
+ */
+static inline void s3c24xx_ts_connect(void)
+{
+	s3c2410_gpio_cfgpin(S3C2410_GPG(12), S3C2410_GPG12_XMON);
+	s3c2410_gpio_cfgpin(S3C2410_GPG(13), S3C2410_GPG13_nXPON);
+	s3c2410_gpio_cfgpin(S3C2410_GPG(14), S3C2410_GPG14_YMON);
+	s3c2410_gpio_cfgpin(S3C2410_GPG(15), S3C2410_GPG15_nYPON);
+}
+
+static void s3c24xx_ts_start_adc_conversion(void)
+{
+	pr_debug("start_adc_conv ADCTSC: 0x%x", 
+		readl(base_addr + S3C2410_ADCTSC));
+	writel(AUTO_XY, base_addr + S3C2410_ADCTSC);
+	pr_debug(" ---> 0x%x", 
+		readl(base_addr + S3C2410_ADCTSC));
+	s3c_adc_start(ts.adc_client, 0, 1);
+	pr_debug(" ---> 0x%x\n", 
+		readl(base_addr + S3C2410_ADCTSC));
+}
+
+
+/*
+ * Event handling
+ */
+
+enum ts_input_event {IE_UP = 0, IE_DOWN};
+
+static void ts_input_report(int event, int coords[])
+{
+
+	if (event == IE_DOWN) {
+		input_report_abs(ts.dev, ABS_X, coords[0]);
+		input_report_abs(ts.dev, ABS_Y, coords[1]);
+		input_report_key(ts.dev, BTN_TOUCH, 1);
+		input_report_abs(ts.dev, ABS_PRESSURE, 1);
+
+		pr_debug("down (X:%03d, Y:%03d)\n", coords[0], coords[1]);
+	} else {
+		input_report_key(ts.dev, BTN_TOUCH, 0);
+		input_report_abs(ts.dev, ABS_PRESSURE, 0);
+		pr_debug("up\n");
+	}
+
+	input_sync(ts.dev);
+}
+
+
+/*
+ * Timer to process the UP event or to report the postion of DRAG
+ */
+
+static void ts_drag_pull_timer_f(unsigned long data);
+static struct timer_list ts_drag_pull_timer = 
+	TIMER_INITIALIZER(ts_drag_pull_timer_f, 0, 0);
+
+static void ts_drag_pull_timer_f(unsigned long data)
+{
+	unsigned long flags;
+	/* delay reporting the UP event to avoid jitter */
+	static unsigned release_pending_counter = 0;
+	pr_debug("Timer called: is_down[%d] status[%d]\n", ts.is_down, ts.state);
+
+	local_irq_save(flags);
+	if(ts.state == TS_STATE_RELEASE_PENDING) {
+		if(release_pending_counter++ < 1) {
+			/* jitter avoidance window: delay TIME_RELEASE_PENDING jfs */
+			mod_timer(&ts_drag_pull_timer, jiffies + TIME_RELEASE_PENDING);
+		} else {
+			/* no down event occurd during last delay */
+			release_pending_counter = 0;
+			ts_input_report(IE_UP, NULL);
+			ts.state = TS_STATE_RELEASE;
+			writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
+		}
+		local_irq_restore(flags);
+		return;
+	}
+
+	/* ts.is_down should be true here */
+	s3c24xx_ts_start_adc_conversion();
+	local_irq_restore(flags);
+}
+
+
+/*
+ * ISR for the IRQ_TS
+ */
+
+static irqreturn_t stylus_updown(int irq, void *dev_id)
+{
+	unsigned long data0;
+	unsigned long data1;
+	unsigned long adctsc;
+	int event_type;		/* 1 for down, 0 for up */
+
+	/* 
+	 * According to s3c2440 manual chap16: After Touch Screen 
+	 * Controller generates INT_TC, XY_PST must be set to [00]
+	 */
+	adctsc = readl(base_addr + S3C2410_ADCTSC);
+	writel(adctsc & ~(S3C2410_ADCTSC_XY_PST(3)), base_addr + S3C2410_ADCTSC);
+	data0 = readl(base_addr + S3C2410_ADCDAT0);
+	data1 = readl(base_addr + S3C2410_ADCDAT1);
+	event_type = (!(data0 & S3C2410_ADCDAT0_UPDOWN)) &&
+					    (!(data1 & S3C2410_ADCDAT0_UPDOWN));
+	pr_debug("event_type: %d, DATA0 0x%lx, DATA1 0x%lx, ADCTSC 0x%lx, "
+		"ADCUPDN 0x%x\n", event_type, data0, data1,
+		adctsc, readl(base_addr + 0x14));
+
+	/* ignore sequential same event */
+	if(ts.is_down == event_type) {
+		pr_debug("###Ignore same event: %d\n", event_type);
+		/* restore XY_PST */
+		writel(adctsc, base_addr + S3C2410_ADCTSC);
+		return IRQ_HANDLED;
+	}
+
+	ts.is_down = event_type;
+	if (ts.is_down) {
+		s3c24xx_ts_start_adc_conversion();
+	} else {
+		ts.state = TS_STATE_RELEASE_PENDING;
+		/*
+		 * Delay to report the up event for a while to avoid jitter.
+		 * This state will be checked after (0, TIME_NEXT_CONV)
+		 * jiffies depended on when the interrupt occured.
+		 */
+	}
+
+	return IRQ_HANDLED;
+}
+
+/* 
+ * Called in ISR of IRQ_ADC
+ */
+
+static void stylus_adc_action(struct s3c_adc_client *client,
+		unsigned p0, unsigned p1, unsigned *conv_left)
+{
+	int buf[2];
+	unsigned long flags;
+
+	local_irq_save(flags);
+	/* 
+	 * IRQ_TS may rise just in the time window between AUTO_XY mode
+	 * set and this pointer */
+	if(ts.state == TS_STATE_RELEASE_PENDING)
+	{
+		local_irq_restore(flags);
+		return;
+	}
+
+	/* Grab the ADC results. */
+	buf[0] = p0;
+	buf[1] = p1;
+
+	ts_input_report(IE_DOWN, buf);
+	pr_debug("[adc_selected: %d] ADCTSC: 0x%x", 
+		ts.adc_selected, readl(base_addr + S3C2410_ADCTSC));
+
+	writel(WAIT4INT(1), base_addr + S3C2410_ADCTSC);
+	printk(" ---> 0x%x\n", 
+		readl(base_addr + S3C2410_ADCTSC));
+
+	ts.state = TS_STATE_PRESSED;
+	mod_timer(&ts_drag_pull_timer, jiffies + TIME_NEXT_CONV);
+	local_irq_restore(flags);
+}
+
+
+/* 
+ * callback function for s3c-adc
+ */
+
+void adc_selected_f(struct s3c_adc_client *client, unsigned selected)
+{
+	ts.adc_selected = selected;
+}
+
+
+/*
+ * The functions for inserting/removing us as a module.
+ */
+
+static int __init s3c24xx_ts_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct input_dev *input_dev;
+	int ret = 0;
+
+	dev_info(&pdev->dev, "Starting\n");
+	pr_debug("Entering s3c24xx_ts_probe\n");
+
+	base_addr = ioremap(S3C2410_PA_ADC, 0x20);
+	if (base_addr == NULL) {
+		dev_err(&pdev->dev, "Failed to remap register block\n");
+		ret = -ENOMEM;
+		goto fail0;
+	}
+
+	/* Configure GPIOs */
+	s3c24xx_ts_connect();
+
+	writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
+
+	/* Initialise input stuff */
+	memset(&ts, 0, sizeof(struct s3c24xx_ts));
+	ts.adc_client =
+		s3c_adc_register(pdev, adc_selected_f, stylus_adc_action, 1);
+	if (!ts.adc_client) {
+			dev_err(&pdev->dev,
+					"Unable to register s3c24xx_ts as s3c_adc client\n");
+			ret = -EIO;
+			goto fail1;
+	}
+
+	/* save prescale EN and VAL in the S3C2410_ADCCON register */
+	ts.presc = readl(base_addr + S3C2410_ADCCON);
+	/* save value of S3C2410_ADCDLY register */
+	ts.delay = readl(base_addr + S3C2410_ADCDLY);
+	pr_debug("platform data: ADCCON=%08x ADCDLY=%08x\n", ts.presc, ts.delay);
+
+	input_dev = input_allocate_device();
+	if (!input_dev) {
+		dev_err(&pdev->dev, "Unable to allocate the input device\n");
+		ret = -ENOMEM;
+		goto fail2;
+	}
+
+	ts.dev = input_dev;
+	ts.dev->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	ts.dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+	input_set_abs_params(ts.dev, ABS_X, 0, 0x3FF, 0, 0);
+	input_set_abs_params(ts.dev, ABS_Y, 0, 0x3FF, 0, 0);
+	input_set_abs_params(ts.dev, ABS_PRESSURE, 0, 1, 0, 0);
+
+	ts.dev->name = s3c24xx_ts_name;
+	ts.dev->id.bustype = BUS_RS232;
+	ts.dev->id.vendor = 0xBAAD;
+	ts.dev->id.product = 0xBEEF;
+	ts.dev->id.version = S3C24XX_TS_VERSION;
+	ts.state = TS_STATE_STANDBY;
+
+	/* Get IRQ */
+	if (request_irq(IRQ_TC, stylus_updown, IRQF_SAMPLE_RANDOM,
+			"s3c24xx_ts_action", ts.dev)) {
+		dev_err(&pdev->dev, "Could not allocate ts IRQ_TC !\n");
+		ret = -EIO;
+		goto fail3;
+	}
+
+	dev_info(&pdev->dev, "Successfully loaded\n");
+
+	/* All went ok, so register to the input system */
+	rc = input_register_device(ts.dev);
+	if (rc) {
+		ret = -EIO;
+		goto fail4;
+	}
+	return 0;
+
+fail4:
+	disable_irq(IRQ_TC);
+	free_irq(IRQ_TC, ts.dev);
+fail3:
+	input_free_device(ts.dev);
+fail2:	
+	s3c_adc_release(ts.adc_client);
+fail1:
+	iounmap(base_addr);
+fail0:
+	return ret;
+}
+
+static int s3c24xx_ts_remove(struct platform_device *pdev)
+{
+	disable_irq(IRQ_TC);
+	free_irq(IRQ_TC, ts.dev);
+	input_unregister_device(ts.dev);
+	input_free_device(ts.dev);
+	s3c_adc_release(ts.adc_client);
+	iounmap(base_addr);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int s3c24xx_ts_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	disable_irq(IRQ_TC);
+	ts.presc = readl(base_addr + S3C2410_ADCCON);
+	ts.delay = readl(base_addr + S3C2410_ADCDLY);
+
+	writel(TSC_SLEEP, base_addr + S3C2410_ADCTSC);
+	writel(ts.presc | S3C2410_ADCCON_STDBM, base_addr + S3C2410_ADCCON);
+	return 0;
+}
+
+static int s3c24xx_ts_resume(struct platform_device *pdev)
+{
+	/* Restore registers */
+	writel(ts.presc, base_addr + S3C2410_ADCCON);
+	writel(ts.delay, base_addr + S3C2410_ADCDLY);
+	writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
+	enable_irq(IRQ_TC);
+	return 0;
+}
+
+#else
+#define s3c24xx_ts_suspend NULL
+#define s3c24xx_ts_resume  NULL
+#endif
+
+static struct platform_driver s3c24xx_ts_driver = {
+	.driver		= {
+		.name	= "s3c24xx-ts",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= s3c24xx_ts_probe,
+	.remove		= s3c24xx_ts_remove,
+	.suspend 	= s3c24xx_ts_suspend,
+	.resume		= s3c24xx_ts_resume,
+};
+
+static int __init s3c24xx_ts_init(void)
+{
+	int rc;
+
+	rc = platform_driver_register(&s3c24xx_ts_driver);
+	return rc;
+}
+
+static void __exit s3c24xx_ts_exit(void)
+{
+	platform_driver_unregister(&s3c24xx_ts_driver);
+}
+
+module_init(s3c24xx_ts_init);
+module_exit(s3c24xx_ts_exit);
+

[PATCH] input/touchscreen: add S3C24XX SoC touchscreen input driver

[Shine Liu]

This touchscreen driver is for touchscreen controller on Samsung
S3C2410/S3C2440 SoC chip. S3C2410/S3C2440 has the on chip touchscreen
controller based on it's analog to digital converter(8-channel analog
inputs, touchscreen uses 4 of them). This driver uses the exsiting S3C
ADC driver to make the touchscreen controller of S3C2410/S3C2440 work
well together with other ADC devices connected to the S3C24XX SoC chip.

The patch is created against kernel 2.6.32-rc4 and tested with S3C2440
SoC and Samsung LTE430WQ-F0C touchscreen.


Signed-off-by: Shine Liu <liuxian@redflag-linux.com>
Signed-off-by: Shine Liu <shinel@foxmail.com>
--------------------------------------------------------

--- a/drivers/input/touchscreen/Makefile	2009-10-12 05:43:56.000000000 +0800
+++ b/drivers/input/touchscreen/Makefile	2009-10-13 20:35:02.000000000 +0800
@@ -10,6 +10,7 @@
 obj-$(CONFIG_TOUCHSCREEN_AD7879)	+= ad7879.o
 obj-$(CONFIG_TOUCHSCREEN_ADS7846)	+= ads7846.o
 obj-$(CONFIG_TOUCHSCREEN_ATMEL_TSADCC)	+= atmel_tsadcc.o
+obj-$(CONFIG_TOUCHSCREEN_S3C24XX_TSADCC)       += s3c24xx_tsadcc.o
 obj-$(CONFIG_TOUCHSCREEN_BITSY)		+= h3600_ts_input.o
 obj-$(CONFIG_TOUCHSCREEN_CORGI)		+= corgi_ts.o
 obj-$(CONFIG_TOUCHSCREEN_GUNZE)		+= gunze.o
--- a/drivers/input/touchscreen/Kconfig	2009-10-12 05:43:56.000000000 +0800
+++ b/drivers/input/touchscreen/Kconfig	2009-10-13 20:37:51.000000000 +0800
@@ -307,6 +307,19 @@
 	  To compile this driver as a module, choose M here: the
 	  module will be called atmel_tsadcc.
 
+config TOUCHSCREEN_S3C24XX_TSADCC
+	tristate "Samsung S3C24XX touchscreen input driver"
+	depends on ARCH_S3C2410 && S3C24XX_ADC && INPUT && INPUT_TOUCHSCREEN
+	select SERIO
+	help
+	  Say Y here if you have a touchscreen connected to the ADC Controller
+	  on your s3c2410/s3c2440 SoC.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called s3c24xx_tsadcc.
+
 config TOUCHSCREEN_UCB1400
 	tristate "Philips UCB1400 touchscreen"
 	depends on AC97_BUS
--- /dev/null	2009-10-09 10:57:12.360986601 +0800
+++ b/drivers/input/touchscreen/s3c24xx_tsadcc.c	2009-10-19 17:16:57.000000000 +0800
@@ -0,0 +1,432 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ * Part of the code come from openmoko project.
+ *
+ * Shine Liu <shinel@foxmail.com>
+ */
+
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/input.h>
+#include <linux/init.h>
+#include <linux/serio.h>
+#include <linux/timer.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <asm/irq.h>
+
+#include <mach/gpio.h>
+#include <mach/regs-gpio.h>
+#include <mach/hardware.h>
+#include <plat/regs-adc.h>
+#include <plat/adc.h>
+
+
+MODULE_AUTHOR("Shine Liu <shinel@foxmail.com>");
+MODULE_DESCRIPTION("Samsung s3c24xx touchscreen driver");
+MODULE_LICENSE("GPL");
+
+
+#define S3C24XX_TS_VERSION	0x0101
+
+#define TSC_SLEEP (S3C2410_ADCTSC_PULL_UP_DISABLE | \
+	S3C2410_ADCTSC_XY_PST(0))
+
+#define WAIT4INT(x)	(((x)<<8) | \
+	S3C2410_ADCTSC_YM_SEN | \
+	S3C2410_ADCTSC_YP_SEN | \
+	S3C2410_ADCTSC_XP_SEN | \
+	S3C2410_ADCTSC_XY_PST(3))
+
+#define AUTO_XY	(S3C2410_ADCTSC_PULL_UP_DISABLE | \
+	S3C2410_ADCTSC_AUTO_PST | \
+	S3C2410_ADCTSC_XY_PST(0))
+
+#define TS_STATE_STANDBY 0 /* initial state */
+#define TS_STATE_PRESSED 1
+#define TS_STATE_RELEASE_PENDING 2 /* wait to confirm the up event */
+#define TS_STATE_RELEASE 3
+
+#define TIME_RELEASE_PENDING (HZ / 20) /* about 50ms */
+#define TIME_NEXT_CONV ((HZ < 51)? 1 : HZ / 50) /* about 20ms */
+
+static char *s3c24xx_ts_name = "Samsung s3c24xx TouchScreen";
+static void __iomem *base_addr;
+
+
+/*
+ * Per-touchscreen data.
+ */
+
+struct s3c24xx_ts {
+	struct input_dev *dev;
+	struct s3c_adc_client *adc_client;
+	unsigned char is_down;
+	unsigned char state;
+	unsigned adc_selected;
+	unsigned int delay;	/* register value for delay */
+	unsigned int presc;	/* register value for prescaler */
+};
+
+static struct s3c24xx_ts ts;
+
+
+/*
+ * Low level functions to config registers.
+ */
+static inline void s3c24xx_ts_connect(void)
+{
+	s3c2410_gpio_cfgpin(S3C2410_GPG(12), S3C2410_GPG12_XMON);
+	s3c2410_gpio_cfgpin(S3C2410_GPG(13), S3C2410_GPG13_nXPON);
+	s3c2410_gpio_cfgpin(S3C2410_GPG(14), S3C2410_GPG14_YMON);
+	s3c2410_gpio_cfgpin(S3C2410_GPG(15), S3C2410_GPG15_nYPON);
+}
+
+static void s3c24xx_ts_start_adc_conversion(void)
+{
+	pr_debug("start_adc_conv ADCTSC: 0x%x", 
+		readl(base_addr + S3C2410_ADCTSC));
+	writel(AUTO_XY, base_addr + S3C2410_ADCTSC);
+	pr_debug(" ---> 0x%x", 
+		readl(base_addr + S3C2410_ADCTSC));
+	s3c_adc_start(ts.adc_client, 0, 1);
+	pr_debug(" ---> 0x%x\n", 
+		readl(base_addr + S3C2410_ADCTSC));
+}
+
+
+/*
+ * Event handling
+ */
+
+enum ts_input_event {IE_UP = 0, IE_DOWN};
+
+static void ts_input_report(int event, int coords[])
+{
+
+	if (event == IE_DOWN) {
+		input_report_abs(ts.dev, ABS_X, coords[0]);
+		input_report_abs(ts.dev, ABS_Y, coords[1]);
+		input_report_key(ts.dev, BTN_TOUCH, 1);
+		input_report_abs(ts.dev, ABS_PRESSURE, 1);
+
+		pr_debug("down (X:%03d, Y:%03d)\n", coords[0], coords[1]);
+	} else {
+		input_report_key(ts.dev, BTN_TOUCH, 0);
+		input_report_abs(ts.dev, ABS_PRESSURE, 0);
+		pr_debug("up\n");
+	}
+
+	input_sync(ts.dev);
+}
+
+
+/*
+ * Timer to process the UP event or to report the postion of DRAG
+ */
+
+static void ts_drag_pull_timer_f(unsigned long data);
+static struct timer_list ts_drag_pull_timer = 
+	TIMER_INITIALIZER(ts_drag_pull_timer_f, 0, 0);
+
+static void ts_drag_pull_timer_f(unsigned long data)
+{
+	unsigned long flags;
+	/* delay reporting the UP event to avoid jitter */
+	static unsigned release_pending_counter = 0;
+	pr_debug("Timer called: is_down[%d] status[%d]\n", ts.is_down, ts.state);
+
+	local_irq_save(flags);
+	if(ts.state == TS_STATE_RELEASE_PENDING) {
+		if(release_pending_counter++ < 1) {
+			/* jitter avoidance window: delay TIME_RELEASE_PENDING jfs */
+			mod_timer(&ts_drag_pull_timer, jiffies + TIME_RELEASE_PENDING);
+		} else {
+			/* no down event occurd during last delay */
+			release_pending_counter = 0;
+			ts_input_report(IE_UP, NULL);
+			ts.state = TS_STATE_RELEASE;
+			writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
+		}
+		local_irq_restore(flags);
+		return;
+	}
+
+	/* ts.is_down should be true here */
+	s3c24xx_ts_start_adc_conversion();
+	local_irq_restore(flags);
+}
+
+
+/*
+ * ISR for the IRQ_TS
+ */
+
+static irqreturn_t stylus_updown(int irq, void *dev_id)
+{
+	unsigned long data0;
+	unsigned long data1;
+	unsigned long adctsc;
+	int event_type;		/* 1 for down, 0 for up */
+
+	/* 
+	 * According to s3c2440 manual chap16: After Touch Screen 
+	 * Controller generates INT_TC, XY_PST must be set to [00]
+	 */
+	adctsc = readl(base_addr + S3C2410_ADCTSC);
+	writel(adctsc & ~(S3C2410_ADCTSC_XY_PST(3)), base_addr + S3C2410_ADCTSC);
+	data0 = readl(base_addr + S3C2410_ADCDAT0);
+	data1 = readl(base_addr + S3C2410_ADCDAT1);
+	event_type = (!(data0 & S3C2410_ADCDAT0_UPDOWN)) &&
+					    (!(data1 & S3C2410_ADCDAT0_UPDOWN));
+	pr_debug("event_type: %d, DATA0 0x%lx, DATA1 0x%lx, ADCTSC 0x%lx, "
+		"ADCUPDN 0x%x\n", event_type, data0, data1,
+		adctsc, readl(base_addr + 0x14));
+
+	/* ignore sequential same event */
+	if(ts.is_down == event_type) {
+		pr_debug("###Ignore same event: %d\n", event_type);
+		/* restore XY_PST */
+		writel(adctsc, base_addr + S3C2410_ADCTSC);
+		return IRQ_HANDLED;
+	}
+
+	ts.is_down = event_type;
+	if (ts.is_down) {
+		s3c24xx_ts_start_adc_conversion();
+	} else {
+		ts.state = TS_STATE_RELEASE_PENDING;
+		/*
+		 * Delay to report the up event for a while to avoid jitter.
+		 * This state will be checked after (0, TIME_NEXT_CONV)
+		 * jiffies depended on when the interrupt occured.
+		 */
+	}
+
+	return IRQ_HANDLED;
+}
+
+/* 
+ * Called in ISR of IRQ_ADC
+ */
+
+static void stylus_adc_action(struct s3c_adc_client *client,
+		unsigned p0, unsigned p1, unsigned *conv_left)
+{
+	int buf[2];
+	unsigned long flags;
+
+	local_irq_save(flags);
+	/* 
+	 * IRQ_TS may rise just in the time window between AUTO_XY mode
+	 * set and this pointer */
+	if(ts.state == TS_STATE_RELEASE_PENDING)
+	{
+		local_irq_restore(flags);
+		return;
+	}
+
+	/* Grab the ADC results. */
+	buf[0] = p0;
+	buf[1] = p1;
+
+	ts_input_report(IE_DOWN, buf);
+	pr_debug("[adc_selected: %d] ADCTSC: 0x%x", 
+		ts.adc_selected, readl(base_addr + S3C2410_ADCTSC));
+
+	writel(WAIT4INT(1), base_addr + S3C2410_ADCTSC);
+	printk(" ---> 0x%x\n", 
+		readl(base_addr + S3C2410_ADCTSC));
+
+	ts.state = TS_STATE_PRESSED;
+	mod_timer(&ts_drag_pull_timer, jiffies + TIME_NEXT_CONV);
+	local_irq_restore(flags);
+}
+
+
+/* 
+ * callback function for s3c-adc
+ */
+
+void adc_selected_f(struct s3c_adc_client *client, unsigned selected)
+{
+	ts.adc_selected = selected;
+}
+
+
+/*
+ * The functions for inserting/removing us as a module.
+ */
+
+static int __init s3c24xx_ts_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct input_dev *input_dev;
+	int ret = 0;
+
+	dev_info(&pdev->dev, "Starting\n");
+	pr_debug("Entering s3c24xx_ts_probe\n");
+
+	base_addr = ioremap(S3C2410_PA_ADC, 0x20);
+	if (base_addr == NULL) {
+		dev_err(&pdev->dev, "Failed to remap register block\n");
+		ret = -ENOMEM;
+		goto fail0;
+	}
+
+	/* Configure GPIOs */
+	s3c24xx_ts_connect();
+
+	writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
+
+	/* Initialise input stuff */
+	memset(&ts, 0, sizeof(struct s3c24xx_ts));
+	ts.adc_client =
+		s3c_adc_register(pdev, adc_selected_f, stylus_adc_action, 1);
+	if (!ts.adc_client) {
+			dev_err(&pdev->dev,
+					"Unable to register s3c24xx_ts as s3c_adc client\n");
+			ret = -EIO;
+			goto fail1;
+	}
+
+	/* save prescale EN and VAL in the S3C2410_ADCCON register */
+	ts.presc = readl(base_addr + S3C2410_ADCCON);
+	/* save value of S3C2410_ADCDLY register */
+	ts.delay = readl(base_addr + S3C2410_ADCDLY);
+	pr_debug("platform data: ADCCON=%08x ADCDLY=%08x\n", ts.presc, ts.delay);
+
+	input_dev = input_allocate_device();
+	if (!input_dev) {
+		dev_err(&pdev->dev, "Unable to allocate the input device\n");
+		ret = -ENOMEM;
+		goto fail2;
+	}
+
+	ts.dev = input_dev;
+	ts.dev->evbit[0] = BIT_MASK(EV_SYN) | BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	ts.dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+	input_set_abs_params(ts.dev, ABS_X, 0, 0x3FF, 0, 0);
+	input_set_abs_params(ts.dev, ABS_Y, 0, 0x3FF, 0, 0);
+	input_set_abs_params(ts.dev, ABS_PRESSURE, 0, 1, 0, 0);
+
+	ts.dev->name = s3c24xx_ts_name;
+	ts.dev->id.bustype = BUS_RS232;
+	ts.dev->id.vendor = 0xBAAD;
+	ts.dev->id.product = 0xBEEF;
+	ts.dev->id.version = S3C24XX_TS_VERSION;
+	ts.state = TS_STATE_STANDBY;
+
+	/* Get IRQ */
+	if (request_irq(IRQ_TC, stylus_updown, IRQF_SAMPLE_RANDOM,
+			"s3c24xx_ts_action", ts.dev)) {
+		dev_err(&pdev->dev, "Could not allocate ts IRQ_TC !\n");
+		ret = -EIO;
+		goto fail3;
+	}
+
+	dev_info(&pdev->dev, "Successfully loaded\n");
+
+	/* All went ok, so register to the input system */
+	rc = input_register_device(ts.dev);
+	if (rc) {
+		ret = -EIO;
+		goto fail4;
+	}
+	return 0;
+
+fail4:
+	disable_irq(IRQ_TC);
+	free_irq(IRQ_TC, ts.dev);
+fail3:
+	input_free_device(ts.dev);
+fail2:	
+	s3c_adc_release(ts.adc_client);
+fail1:
+	iounmap(base_addr);
+fail0:
+	return ret;
+}
+
+static int s3c24xx_ts_remove(struct platform_device *pdev)
+{
+	disable_irq(IRQ_TC);
+	free_irq(IRQ_TC, ts.dev);
+	input_unregister_device(ts.dev);
+	input_free_device(ts.dev);
+	s3c_adc_release(ts.adc_client);
+	iounmap(base_addr);
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int s3c24xx_ts_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	disable_irq(IRQ_TC);
+	ts.presc = readl(base_addr + S3C2410_ADCCON);
+	ts.delay = readl(base_addr + S3C2410_ADCDLY);
+
+	writel(TSC_SLEEP, base_addr + S3C2410_ADCTSC);
+	writel(ts.presc | S3C2410_ADCCON_STDBM, base_addr + S3C2410_ADCCON);
+	return 0;
+}
+
+static int s3c24xx_ts_resume(struct platform_device *pdev)
+{
+	/* Restore registers */
+	writel(ts.presc, base_addr + S3C2410_ADCCON);
+	writel(ts.delay, base_addr + S3C2410_ADCDLY);
+	writel(WAIT4INT(0), base_addr + S3C2410_ADCTSC);
+	enable_irq(IRQ_TC);
+	return 0;
+}
+
+#else
+#define s3c24xx_ts_suspend NULL
+#define s3c24xx_ts_resume  NULL
+#endif
+
+static struct platform_driver s3c24xx_ts_driver = {
+	.driver		= {
+		.name	= "s3c24xx-ts",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= s3c24xx_ts_probe,
+	.remove		= s3c24xx_ts_remove,
+	.suspend 	= s3c24xx_ts_suspend,
+	.resume		= s3c24xx_ts_resume,
+};
+
+static int __init s3c24xx_ts_init(void)
+{
+	int rc;
+
+	rc = platform_driver_register(&s3c24xx_ts_driver);
+	return rc;
+}
+
+static void __exit s3c24xx_ts_exit(void)
+{
+	platform_driver_unregister(&s3c24xx_ts_driver);
+}
+
+module_init(s3c24xx_ts_init);
+module_exit(s3c24xx_ts_exit);
+