Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
On Sunday 20 July 2014 23:37:18 Hartmut Knaack wrote:
Jonathan Cameron schrieb:
On 18/07/14 20:29, Arnd Bergmann wrote:
- I simply register the input device from the adc driver
itself, as the at91 code does. The driver also supports
sub-nodes, but I don't understand how they are meant
to be used, so using those might be better.
So, the alternative you are (I think) referring to is to use
the buffered in kernel client interface. That way a separate
touch screen driver can use the output channels provided by IIO
in much the same way you might use a regulator or similar.
Note that whilst this is similar to the simple polled interface
used for things like the iio_hwmon driver, the data flow is
quite different (clearly the polled interfce would be
inappropriate here).
Whilst we've discussed it in the past for touch screen drivers
like this, usually the hardware isn't generic enough to be
of any real use if not being used as a touch screen. As such
it's often simpler to just have the support directly in the
driver (as you've observed the at91 driver does this).
Ok, I see. That's exactly the information I was looking for.
Whilst the interface has been there a while, it's not really had
all that much use. The original target was the simpler case
of 3D accelerometer where we have a generic iio to input
bridge driver. Time constraints meant that I haven't yet actually
formally submitted the input side of this. Whilst there are lots
of other things that can use this interface, right now nothing
actually does so.
Ok.
- The new exynos_read_s3c64xx_ts() function is intentionally
very similar to the existing exynos_read_raw() functions.
It should probably be changed, either by merging the two
into one, or by simplifying the exynos_read_s3c64xx_ts()
function. This depends a bit on the answers to the questions
above.
I'd be tempted to not bother keeping them that similar. It's
not a generic IIO channel so simplify it where possible.
Ok.
index 5f95638513d2..cf1d9f3e2492 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -34,6 +34,7 @@
#include linux/regulator/consumer.h
#include linux/of_platform.h
#include linux/err.h
+#include linux/input.h
Might want to make the input side optional at compile time...
I supose the existing parts are unlikely to be used much in headless
devices, but you never know. Maybe we just leave this until someone
shouts they want to be able to avoid compiling it in.
I expected the input stuff to just be left out by the compiler
if CONFIG_INPUT is not set. I'll try it out and change it if necessary.
struct completion completion;
+ boolread_ts;
u32 value;
+ u32 value2;
As I state further down, I'd rather keep a little
clear of the naming used in IIO for bits that aren't
going through IIO (less confusing!). Maybe just
have
u32 x, y;
Ok.
unsigned intversion;
};
@@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
return ret;
}
+static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val,
+ int *val2,
+ long mask)
+{
+ struct exynos_adc *info = iio_priv(indio_dev);
+ unsigned long timeout;
+ int ret;
+
+ if (mask != IIO_CHAN_INFO_RAW)
+ return -EINVAL;
+
+ mutex_lock(indio_dev-mlock);
+ info-read_ts = 1;
Since info-read_ts is of type bool, use true/false.
Ok
+
+ reinit_completion(info-completion);
+
+ writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
+ ADC_V1_TSC(info-regs));
+
+ /* Select the ts channel to be used and Trigger conversion */
+ info-data-start_conv(info, 0);
0 is a rather magic value. A define perhaps?
I'm not entirely sure about why we pass 0 here, it's either channel zero
being used for touchscreen, or the channel number being ignore after
we write to the TSC register above. I copied it from the original driver,
but it might be helpful if someone with access to the specs could comment
here.
I'll add a macro for now.
+
+ timeout = wait_for_completion_timeout
+ (info-completion, EXYNOS_ADC_TIMEOUT);
+ if (timeout == 0) {
+ dev_warn(indio_dev-dev, Conversion timed out! Resetting\n);
+ if (info-data-init_hw)
+ info-data-init_hw(info);
+ ret = -ETIMEDOUT;
+ } else {
+ *val = info-value;
+ *val2 = info-value2;
This is definitely abuse as those two values are not intended for
different values. If you want to do this please use different naming
and don't try to fiddle it into the IIO read raw framework.
As you've suggested above,
Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
On Sunday 20 July 2014 13:28:42 Dmitry Torokhov wrote:
On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
+
+do {
+ret =exynos_read_s3c64xx_ts(dev, NULL, x, y,
IIO_CHAN_INFO_RAW);
= exynos
+if (ret == -ETIMEDOUT)
+break;
+
+pressed = x y ADC_DATX_PRESSED;
+if (!pressed)
+break;
+
+input_report_abs(info-input, ABS_X, x ADC_DATX_MASK);
+input_report_abs(info-input, ABS_Y, y ADC_DATX_MASK);
+input_report_key(info-input, BTN_TOUCH, 1);
+input_sync(info-input);
+
+msleep(1);
+} while (1);
It would be nice to actually close the device even if someone is
touching screen. Please implement open/close methods and have them set a
flag that you would check here.
Ok. I think it's even better to move the request_irq() into the open function,
which will avoid the flag and defer the error handling into the actual opening,
as well as syncing the running irq with the close function.
+/* data from s3c2410_ts driver */
+info-input-name = S3C24xx TouchScreen;
+info-input-id.bustype = BUS_HOST;
+info-input-id.vendor = 0xDEAD;
+info-input-id.product = 0xBEEF;
You do not need to fill these entries with fake data.
Ok, I wondered about this, but didn't want to change too much from
the old driver (I changed the version number).
+info-input-id.version = 0x0200;
Do I need this?
@@ -576,6 +728,11 @@ static int exynos_adc_probe(struct platform_device
*pdev)
err_of_populate:
device_for_each_child(indio_dev-dev, NULL,
exynos_adc_remove_devices);
+if (has_ts) {
+input_unregister_device(info-input);
+free_irq(info-tsirq, info);
Are we sure that device is quiesced here and interrupt won't arrive
between input_unregister_device() and free_irq()? I guess if you
properly implement open/close it won't matter.
Should be fixed now.
+err_iio:
iio_device_unregister(indio_dev);
err_irq:
free_irq(info-irq, info);
@@ -595,9 +752,12 @@ static int exynos_adc_remove(struct platform_device
*pdev)
struct iio_dev *indio_dev = platform_get_drvdata(pdev);
struct exynos_adc *info = iio_priv(indio_dev);
+input_free_device(info-input);
Should be unregister, not free.
Ok.
Thanks a lot for the review! I'll send out the new version after some build
testing.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
On Monday 21 July 2014 12:23:58 Arnd Bergmann wrote:
Thanks a lot for the review! I'll send out the new version after some build
testing.
Here are the changes I did to the adc driver based on the review comments.
I'll start a new thread for the updated version that includes the changes.
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 3d2caea05977..823d7ebc7f52 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -67,6 +67,9 @@
#define ADC_S3C2410_CON_SELMUX(x) (((x)0x7)3)
+/* touch screen always uses channel 0 */
+#define ADC_S3C2410_MUX_TS 0
+
/* ADCTSC Register Bits */
#define ADC_S3C2443_TSC_UD_SEN (1u 8)
#define ADC_S3C2410_TSC_YM_SEN (1u 7)
@@ -106,6 +109,7 @@
#define ADC_CON_EN_START (1u 0)
#define ADC_DATX_PRESSED (1u 15)
#define ADC_DATX_MASK 0xFFF
+#define ADC_DATY_MASK 0xFFF
#define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
@@ -123,10 +127,12 @@ struct exynos_adc {
struct completion completion;
- boolread_ts;
u32 value;
- u32 value2;
unsigned intversion;
+
+ boolread_ts;
+ u32 ts_x;
+ u32 ts_y;
};
struct exynos_adc_data {
@@ -396,21 +402,14 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
return ret;
}
-static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan,
- int *val,
- int *val2,
- long mask)
+static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev, int *x, int *y)
{
struct exynos_adc *info = iio_priv(indio_dev);
unsigned long timeout;
int ret;
- if (mask != IIO_CHAN_INFO_RAW)
- return -EINVAL;
-
mutex_lock(indio_dev-mlock);
- info-read_ts = 1;
+ info-read_ts = true;
reinit_completion(info-completion);
@@ -418,7 +417,7 @@ static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
ADC_V1_TSC(info-regs));
/* Select the ts channel to be used and Trigger conversion */
- info-data-start_conv(info, 0);
+ info-data-start_conv(info, ADC_S3C2410_MUX_TS);
timeout = wait_for_completion_timeout
(info-completion, EXYNOS_ADC_TIMEOUT);
@@ -428,12 +427,12 @@ static int exynos_read_s3c64xx_ts(struct iio_dev
*indio_dev,
info-data-init_hw(info);
ret = -ETIMEDOUT;
} else {
- *val = info-value;
- *val2 = info-value2;
- ret = IIO_VAL_INT;
+ *x = info-ts_x;
+ *y = info-ts_y;
+ ret = 0;
}
- info-read_ts = 0;
+ info-read_ts = false;
mutex_unlock(indio_dev-mlock);
return ret;
@@ -445,8 +444,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
/* Read value */
if (info-read_ts) {
- info-value = readl(ADC_V1_DATX(info-regs)) ADC_DATX_MASK;
- info-value2 = readl(ADC_V1_DATY(info-regs)) ADC_DATX_MASK;
+ info-ts_x = readl(ADC_V1_DATX(info-regs));
+ info-ts_y = readl(ADC_V1_DATY(info-regs));
writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN,
ADC_V1_TSC(info-regs));
} else {
info-value = readl(ADC_V1_DATX(info-regs)) ADC_DATX_MASK;
@@ -477,7 +476,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
int ret;
do {
- ret =exynos_read_s3c64xx_ts(dev, NULL, x, y,
IIO_CHAN_INFO_RAW);
+ ret = exynos_read_s3c64xx_ts(dev, x, y);
if (ret == -ETIMEDOUT)
break;
@@ -486,11 +485,15 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
break;
input_report_abs(info-input, ABS_X, x ADC_DATX_MASK);
- input_report_abs(info-input, ABS_Y, y ADC_DATX_MASK);
+ input_report_abs(info-input, ABS_Y, y ADC_DATY_MASK);
input_report_key(info-input, BTN_TOUCH, 1);
input_sync(info-input);
msleep(1);
+
+ /* device may have been closed while touched */
+ if (!info-input-users)
+ return IRQ_HANDLED;
} while (1);
input_report_key(info-input, BTN_TOUCH, 0);
@@ -552,10 +555,28 @@ static int exynos_adc_remove_devices(struct device *dev,
void *c)
return 0;
}
+static int exynos_adc_ts_open(struct input_dev *dev)
+{
+ struct exynos_adc *info = input_get_drvdata(dev);
+
+ return request_threaded_irq(info-tsirq, NULL, exynos_ts_isr,
+ 0, touchscreen, info);
+}
+
Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
On Mon, Jul 21, 2014 at 12:23:58PM +0200, Arnd Bergmann wrote:
On Sunday 20 July 2014 13:28:42 Dmitry Torokhov wrote:
On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
+
+do {
+ret =exynos_read_s3c64xx_ts(dev, NULL, x, y,
IIO_CHAN_INFO_RAW);
= exynos
+if (ret == -ETIMEDOUT)
+break;
+
+pressed = x y ADC_DATX_PRESSED;
+if (!pressed)
+break;
+
+input_report_abs(info-input, ABS_X, x ADC_DATX_MASK);
+input_report_abs(info-input, ABS_Y, y ADC_DATX_MASK);
+input_report_key(info-input, BTN_TOUCH, 1);
+input_sync(info-input);
+
+msleep(1);
+} while (1);
It would be nice to actually close the device even if someone is
touching screen. Please implement open/close methods and have them set a
flag that you would check here.
Ok. I think it's even better to move the request_irq() into the open function,
which will avoid the flag and defer the error handling into the actual
opening,
as well as syncing the running irq with the close function.
I do not quite like acquiring resources needed in open. I think drivers should
do all resource acquisition in probe() and leave open()/close() to
activate/quiesce devices.
+/* data from s3c2410_ts driver */
+info-input-name = S3C24xx TouchScreen;
+info-input-id.bustype = BUS_HOST;
+info-input-id.vendor = 0xDEAD;
+info-input-id.product = 0xBEEF;
You do not need to fill these entries with fake data.
Ok, I wondered about this, but didn't want to change too much from
the old driver (I changed the version number).
+info-input-id.version = 0x0200;
Do I need this?
Not really.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
On Mon, Jul 21, 2014 at 05:11:27PM +0200, Arnd Bergmann wrote:
On Monday 21 July 2014 07:44:42 Dmitry Torokhov wrote:
It would be nice to actually close the device even if someone is
touching screen. Please implement open/close methods and have them set a
flag that you would check here.
Ok. I think it's even better to move the request_irq() into the open
function,
which will avoid the flag and defer the error handling into the actual
opening,
as well as syncing the running irq with the close function.
I do not quite like acquiring resources needed in open. I think drivers
should
do all resource acquisition in probe() and leave open()/close() to
activate/quiesce devices.
Ok, I'll move it back then. I'm not sure what I'm supposed to do
in open/close then. Isn't it enough to check info-input-users
like this?
static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
{
struct exynos_adc *info = dev_id;
struct iio_dev *dev = dev_get_drvdata(info-dev);
u32 x, y;
bool pressed;
int ret;
while (info-input-users) {
ret = exynos_read_s3c64xx_ts(dev, x, y);
if (ret == -ETIMEDOUT)
break;
pressed = x y ADC_DATX_PRESSED;
if (!pressed) {
input_report_key(info-input, BTN_TOUCH, 0);
input_sync(info-input);
break;
}
input_report_abs(info-input, ABS_X, x ADC_DATX_MASK);
input_report_abs(info-input, ABS_Y, y ADC_DATY_MASK);
input_report_key(info-input, BTN_TOUCH, 1);
input_sync(info-input);
msleep(1);
};
writel(0, ADC_V1_CLRINTPNDNUP(info-regs));
return IRQ_HANDLED;
}
I could do enable_irq()/disable_irq(), but that leaves a small
race at startup where we request the irq line and then immediately
disable it again.
I think the above (or a separate flag in driver structure) coupled with
enable/disable IRQ is fine - input core can deal with calls to input_report_*
on devices that have been properly allocated but have not been registered yet.
drivers/input/keyboard/samsung-keypad.c does similar handling.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
On 18/07/14 20:29, Arnd Bergmann wrote:
This adds support for the touchscreen on Samsung s3c64xx.
The driver is completely untested but shows roughly how
it could be done, following the example of the at91 driver.
Hi Arnd,
Open questions include:
- compared to the old plat-samsung/adc driver, there is
no support for prioritizing ts over other clients, nor
for oversampling. From my reading of the code, the
priorities didn't actually have any effect at all, but
the oversampling might be needed. Maybe the original
authors have some insight.
- I simply register the input device from the adc driver
itself, as the at91 code does. The driver also supports
sub-nodes, but I don't understand how they are meant
to be used, so using those might be better.
So, the alternative you are (I think) referring to is to use
the buffered in kernel client interface. That way a separate
touch screen driver can use the output channels provided by IIO
in much the same way you might use a regulator or similar.
Note that whilst this is similar to the simple polled interface
used for things like the iio_hwmon driver, the data flow is
quite different (clearly the polled interfce would be
inappropriate here).
Whilst we've discussed it in the past for touch screen drivers
like this, usually the hardware isn't generic enough to be
of any real use if not being used as a touch screen. As such
it's often simpler to just have the support directly in the
driver (as you've observed the at91 driver does this).
Whilst the interface has been there a while, it's not really had
all that much use. The original target was the simpler case
of 3D accelerometer where we have a generic iio to input
bridge driver. Time constraints meant that I haven't yet actually
formally submitted the input side of this. Whilst there are lots
of other things that can use this interface, right now nothing
actually does so.
- The new exynos_read_s3c64xx_ts() function is intentionally
very similar to the existing exynos_read_raw() functions.
It should probably be changed, either by merging the two
into one, or by simplifying the exynos_read_s3c64xx_ts()
function. This depends a bit on the answers to the questions
above.
I'd be tempted to not bother keeping them that similar. It's
not a generic IIO channel so simplify it where possible.
- We probably need to add support for platform_data as well,
I've skipped this so far.
- Is anybody able to debug this driver on real hardware?
While it's possible that it actually works, it's more
likely that I made a few subtle mistakes.
Signed-off-by: Arnd Bergmann a...@arndb.de
Looks pretty good to me. A few symantic bits and pieces and
one bug spotted. Short and sweet.
diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
index e1b74828f413..4329bf3c3326 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
@@ -41,6 +41,10 @@ Required properties:
and compatible ADC block)
- vdd-supply VDD input supply.
+Optional properties:
+- has-touchscreen: If present, indicates that a touchscreen is
+ connected an usable.
+
Note: child nodes can be added for auto probing from device tree.
Example: adding device info in dtsi file
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 5f95638513d2..cf1d9f3e2492 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -34,6 +34,7 @@
#include linux/regulator/consumer.h
#include linux/of_platform.h
#include linux/err.h
+#include linux/input.h
Might want to make the input side optional at compile time...
I supose the existing parts are unlikely to be used much in headless
devices, but you never know. Maybe we just leave this until someone
shouts they want to be able to avoid compiling it in.
#include linux/iio/iio.h
#include linux/iio/machine.h
@@ -103,6 +104,7 @@
/* Bit definitions common for ADC_V1 and ADC_V2 */
#define ADC_CON_EN_START (1u 0)
+#define ADC_DATX_PRESSED (1u 15)
#define ADC_DATX_MASK 0xFFF
#define EXYNOS_ADC_TIMEOUT(msecs_to_jiffies(100))
@@ -110,16 +112,20 @@
struct exynos_adc {
struct exynos_adc_data *data;
struct device *dev;
+ struct input_dev*input;
void __iomem*regs;
void __iomem*enable_reg;
struct clk *clk;
struct clk *sclk;
unsigned intirq;
+ unsigned inttsirq;
struct regulator*vdd;
struct completion completion;
+ boolread_ts;
u32 value;
+ u32 value2;
As I state further down, I'd rather keep a
Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
On 20/07/14 14:49, Jonathan Cameron wrote:
On 18/07/14 20:29, Arnd Bergmann wrote:
This adds support for the touchscreen on Samsung s3c64xx.
The driver is completely untested but shows roughly how
it could be done, following the example of the at91 driver.
Hi Arnd,
Cc'd linux-input and Dmitry.
Open questions include:
- compared to the old plat-samsung/adc driver, there is
no support for prioritizing ts over other clients, nor
for oversampling. From my reading of the code, the
priorities didn't actually have any effect at all, but
the oversampling might be needed. Maybe the original
authors have some insight.
- I simply register the input device from the adc driver
itself, as the at91 code does. The driver also supports
sub-nodes, but I don't understand how they are meant
to be used, so using those might be better.
So, the alternative you are (I think) referring to is to use
the buffered in kernel client interface. That way a separate
touch screen driver can use the output channels provided by IIO
in much the same way you might use a regulator or similar.
Note that whilst this is similar to the simple polled interface
used for things like the iio_hwmon driver, the data flow is
quite different (clearly the polled interfce would be
inappropriate here).
Whilst we've discussed it in the past for touch screen drivers
like this, usually the hardware isn't generic enough to be
of any real use if not being used as a touch screen. As such
it's often simpler to just have the support directly in the
driver (as you've observed the at91 driver does this).
Whilst the interface has been there a while, it's not really had
all that much use. The original target was the simpler case
of 3D accelerometer where we have a generic iio to input
bridge driver. Time constraints meant that I haven't yet actually
formally submitted the input side of this. Whilst there are lots
of other things that can use this interface, right now nothing
actually does so.
- The new exynos_read_s3c64xx_ts() function is intentionally
very similar to the existing exynos_read_raw() functions.
It should probably be changed, either by merging the two
into one, or by simplifying the exynos_read_s3c64xx_ts()
function. This depends a bit on the answers to the questions
above.
I'd be tempted to not bother keeping them that similar. It's
not a generic IIO channel so simplify it where possible.
- We probably need to add support for platform_data as well,
I've skipped this so far.
- Is anybody able to debug this driver on real hardware?
While it's possible that it actually works, it's more
likely that I made a few subtle mistakes.
Signed-off-by: Arnd Bergmann a...@arndb.de
Looks pretty good to me. A few symantic bits and pieces and
one bug spotted. Short and sweet.
diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
index e1b74828f413..4329bf3c3326 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
@@ -41,6 +41,10 @@ Required properties:
and compatible ADC block)
- vdd-supplyVDD input supply.
+Optional properties:
+- has-touchscreen:If present, indicates that a touchscreen is
+connected an usable.
+
Note: child nodes can be added for auto probing from device tree.
Example: adding device info in dtsi file
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 5f95638513d2..cf1d9f3e2492 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -34,6 +34,7 @@
#include linux/regulator/consumer.h
#include linux/of_platform.h
#include linux/err.h
+#include linux/input.h
Might want to make the input side optional at compile time...
I supose the existing parts are unlikely to be used much in headless
devices, but you never know. Maybe we just leave this until someone
shouts they want to be able to avoid compiling it in.
#include linux/iio/iio.h
#include linux/iio/machine.h
@@ -103,6 +104,7 @@
/* Bit definitions common for ADC_V1 and ADC_V2 */
#define ADC_CON_EN_START(1u 0)
+#define ADC_DATX_PRESSED(1u 15)
#define ADC_DATX_MASK0xFFF
#define EXYNOS_ADC_TIMEOUT(msecs_to_jiffies(100))
@@ -110,16 +112,20 @@
struct exynos_adc {
struct exynos_adc_data*data;
struct device*dev;
+struct input_dev*input;
void __iomem*regs;
void __iomem*enable_reg;
struct clk*clk;
struct clk*sclk;
unsigned intirq;
+unsigned inttsirq;
struct regulator*vdd;
struct completioncompletion;
+boolread_ts;
u32value;
+u32value2;
As I state further down, I'd rather keep a little
clear of the naming used in IIO for bits that
Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
On 20/07/14 14:49, Jonathan Cameron wrote:
On 18/07/14 20:29, Arnd Bergmann wrote:
This adds support for the touchscreen on Samsung s3c64xx.
The driver is completely untested but shows roughly how
it could be done, following the example of the at91 driver.
Hi Arnd,
Cc'd linux-input and Dmitry.
Open questions include:
- compared to the old plat-samsung/adc driver, there is
no support for prioritizing ts over other clients, nor
for oversampling. From my reading of the code, the
priorities didn't actually have any effect at all, but
the oversampling might be needed. Maybe the original
authors have some insight.
- I simply register the input device from the adc driver
itself, as the at91 code does. The driver also supports
sub-nodes, but I don't understand how they are meant
to be used, so using those might be better.
So, the alternative you are (I think) referring to is to use
the buffered in kernel client interface. That way a separate
touch screen driver can use the output channels provided by IIO
in much the same way you might use a regulator or similar.
Note that whilst this is similar to the simple polled interface
used for things like the iio_hwmon driver, the data flow is
quite different (clearly the polled interfce would be
inappropriate here).
Whilst we've discussed it in the past for touch screen drivers
like this, usually the hardware isn't generic enough to be
of any real use if not being used as a touch screen. As such
it's often simpler to just have the support directly in the
driver (as you've observed the at91 driver does this).
Whilst the interface has been there a while, it's not really had
all that much use. The original target was the simpler case
of 3D accelerometer where we have a generic iio to input
bridge driver. Time constraints meant that I haven't yet actually
formally submitted the input side of this. Whilst there are lots
of other things that can use this interface, right now nothing
actually does so.
- The new exynos_read_s3c64xx_ts() function is intentionally
very similar to the existing exynos_read_raw() functions.
It should probably be changed, either by merging the two
into one, or by simplifying the exynos_read_s3c64xx_ts()
function. This depends a bit on the answers to the questions
above.
I'd be tempted to not bother keeping them that similar. It's
not a generic IIO channel so simplify it where possible.
- We probably need to add support for platform_data as well,
I've skipped this so far.
- Is anybody able to debug this driver on real hardware?
While it's possible that it actually works, it's more
likely that I made a few subtle mistakes.
Signed-off-by: Arnd Bergmann a...@arndb.de
Looks pretty good to me. A few symantic bits and pieces and
one bug spotted. Short and sweet.
diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
index e1b74828f413..4329bf3c3326 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
@@ -41,6 +41,10 @@ Required properties:
and compatible ADC block)
- vdd-supplyVDD input supply.
+Optional properties:
+- has-touchscreen:If present, indicates that a touchscreen is
+connected an usable.
+
Note: child nodes can be added for auto probing from device tree.
Example: adding device info in dtsi file
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 5f95638513d2..cf1d9f3e2492 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -34,6 +34,7 @@
#include linux/regulator/consumer.h
#include linux/of_platform.h
#include linux/err.h
+#include linux/input.h
Might want to make the input side optional at compile time...
I supose the existing parts are unlikely to be used much in headless
devices, but you never know. Maybe we just leave this until someone
shouts they want to be able to avoid compiling it in.
#include linux/iio/iio.h
#include linux/iio/machine.h
@@ -103,6 +104,7 @@
/* Bit definitions common for ADC_V1 and ADC_V2 */
#define ADC_CON_EN_START(1u 0)
+#define ADC_DATX_PRESSED(1u 15)
#define ADC_DATX_MASK0xFFF
#define EXYNOS_ADC_TIMEOUT(msecs_to_jiffies(100))
@@ -110,16 +112,20 @@
struct exynos_adc {
struct exynos_adc_data*data;
struct device*dev;
+struct input_dev*input;
void __iomem*regs;
void __iomem*enable_reg;
struct clk*clk;
struct clk*sclk;
unsigned intirq;
+unsigned inttsirq;
struct regulator*vdd;
struct completioncompletion;
+bool
Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support
Jonathan Cameron schrieb:
On 18/07/14 20:29, Arnd Bergmann wrote:
This adds support for the touchscreen on Samsung s3c64xx.
The driver is completely untested but shows roughly how
it could be done, following the example of the at91 driver.
Hi Arnd,
Open questions include:
- compared to the old plat-samsung/adc driver, there is
no support for prioritizing ts over other clients, nor
for oversampling. From my reading of the code, the
priorities didn't actually have any effect at all, but
the oversampling might be needed. Maybe the original
authors have some insight.
- I simply register the input device from the adc driver
itself, as the at91 code does. The driver also supports
sub-nodes, but I don't understand how they are meant
to be used, so using those might be better.
So, the alternative you are (I think) referring to is to use
the buffered in kernel client interface. That way a separate
touch screen driver can use the output channels provided by IIO
in much the same way you might use a regulator or similar.
Note that whilst this is similar to the simple polled interface
used for things like the iio_hwmon driver, the data flow is
quite different (clearly the polled interfce would be
inappropriate here).
Whilst we've discussed it in the past for touch screen drivers
like this, usually the hardware isn't generic enough to be
of any real use if not being used as a touch screen. As such
it's often simpler to just have the support directly in the
driver (as you've observed the at91 driver does this).
Whilst the interface has been there a while, it's not really had
all that much use. The original target was the simpler case
of 3D accelerometer where we have a generic iio to input
bridge driver. Time constraints meant that I haven't yet actually
formally submitted the input side of this. Whilst there are lots
of other things that can use this interface, right now nothing
actually does so.
- The new exynos_read_s3c64xx_ts() function is intentionally
very similar to the existing exynos_read_raw() functions.
It should probably be changed, either by merging the two
into one, or by simplifying the exynos_read_s3c64xx_ts()
function. This depends a bit on the answers to the questions
above.
I'd be tempted to not bother keeping them that similar. It's
not a generic IIO channel so simplify it where possible.
- We probably need to add support for platform_data as well,
I've skipped this so far.
- Is anybody able to debug this driver on real hardware?
While it's possible that it actually works, it's more
likely that I made a few subtle mistakes.
Signed-off-by: Arnd Bergmann a...@arndb.de
Looks pretty good to me. A few symantic bits and pieces and
one bug spotted. Short and sweet.
diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
index e1b74828f413..4329bf3c3326 100644
--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
@@ -41,6 +41,10 @@ Required properties:
and compatible ADC block)
- vdd-supply VDD input supply.
+Optional properties:
+- has-touchscreen: If present, indicates that a touchscreen is
+connected an usable.
+
Note: child nodes can be added for auto probing from device tree.
Example: adding device info in dtsi file
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 5f95638513d2..cf1d9f3e2492 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -34,6 +34,7 @@
#include linux/regulator/consumer.h
#include linux/of_platform.h
#include linux/err.h
+#include linux/input.h
Might want to make the input side optional at compile time...
I supose the existing parts are unlikely to be used much in headless
devices, but you never know. Maybe we just leave this until someone
shouts they want to be able to avoid compiling it in.
#include linux/iio/iio.h
#include linux/iio/machine.h
@@ -103,6 +104,7 @@
/* Bit definitions common for ADC_V1 and ADC_V2 */
#define ADC_CON_EN_START (1u 0)
+#define ADC_DATX_PRESSED(1u 15)
#define ADC_DATX_MASK 0xFFF
#define EXYNOS_ADC_TIMEOUT (msecs_to_jiffies(100))
@@ -110,16 +112,20 @@
struct exynos_adc {
struct exynos_adc_data *data;
struct device *dev;
+struct input_dev*input;
void __iomem*regs;
void __iomem*enable_reg;
struct clk *clk;
struct clk *sclk;
unsigned intirq;
+unsigned inttsirq;
struct regulator*vdd;
struct completion completion;
+boolread_ts;
u32 value;
+u32
