Add support for hardware which uses an I2C Serializer / Deserializer
(SerDes) to communicate with the zFroce touch driver. In this case the
SerDes will be configured as an interrupt controller and the zForce driver
will have no access to poll the GPIO line.
To support this, we add two dedicated new GPIOs in the device tree:
rst-gpio and int-gpio. With the int-gpio being optional, then.
To not break the existing device trees, the index based 'gpios' entries
are still supported, but marked as depreciated.
With this, if the interrupt GPIO is available, either via the old or new
device tree style, the while loop will read and handle the packets as long
as the GPIO indicates that the interrupt is asserted (existing, unchanged
driver behavior).
If the interrupt GPIO isn't available, i.e. not configured via the new
device tree style, we are falling back to one read per ISR invocation
(new behavior to support the SerDes).
Note that the gpiod functions help to handle the optional GPIO:
devm_gpiod_get_index_optional() will return NULL in case the interrupt
GPIO isn't available. And gpiod_get_value_cansleep() does cover this, too,
by returning 0 in this case.
Signed-off-by: Dirk Behme <redacted>
---
.../bindings/input/touchscreen/zforce_ts.txt | 8 ++--
drivers/input/touchscreen/zforce_ts.c | 49 +++++++++++++++++++++-
2 files changed, 52 insertions(+), 5 deletions(-)
@@ -4,12 +4,12 @@ Required properties: - compatible: must be "neonode,zforce" - reg: I2C address of the chip - interrupts: interrupt to which the chip is connected-- gpios: gpios the chip is connected to- first one is the interrupt gpio and second one the reset gpio+- rst-gpio: reset gpio the chip is connected to - x-size: horizontal resolution of touchscreen - y-size: vertical resolution of touchscreen Optional properties:+- int-gpio : interrupt gpio the chip is connected to - vdd-supply: Regulator controlling the controller supply Example:
@@ -754,6 +766,40 @@ static int zforce_probe(struct i2c_client *client,if(!ts)return-ENOMEM;+/*+*TheresetGPIOisn'toptional,butwemightgetit+*viatheoldstyleDTentriesbelow,too.Soit's+*notanerrorifwedon'tgetithere.Thereforeuse+*devm_gpiod_get_optional()here.+*/+ts->gpio_rst=devm_gpiod_get_optional(&client->dev,"rst",+GPIOD_OUT_HIGH);+if(IS_ERR(ts->gpio_rst)){+ret=PTR_ERR(ts->gpio_rst);+dev_err(&client->dev,+"failed to request reset GPIO: %d\n",ret);+returnret;+}++ts->gpio_int=devm_gpiod_get_optional(&client->dev,"int",GPIOD_IN);+if(IS_ERR(ts->gpio_int)){+ret=PTR_ERR(ts->gpio_int);+dev_err(&client->dev,+"failed to request interrupt GPIO: %d\n",ret);+returnret;+}++/* Skip the old style GPIO if we have the new one */+if(ts->gpio_rst)+gotoskip;++/*+*DepreciatedGPIOdevicetreehandlingforcompatibility+*withexistingdevicetrees.Fornewdevicetrees,don't+*useitanymore.Insteaduserst-gpioandtheoptional+*int-gpioabove.+*/+/* INT GPIO */ts->gpio_int=devm_gpiod_get_index(&client->dev,NULL,0,GPIOD_IN);if(IS_ERR(ts->gpio_int)){
@@ -773,6 +819,7 @@ static int zforce_probe(struct i2c_client *client,returnret;}+skip:ts->reg_vdd=devm_regulator_get_optional(&client->dev,"vdd");if(IS_ERR(ts->reg_vdd)){ret=PTR_ERR(ts->reg_vdd);
On Tue, Jul 28, 2015 at 10:26:48AM +0200, Dirk Behme wrote:
Add support for hardware which uses an I2C Serializer / Deserializer
(SerDes) to communicate with the zFroce touch driver. In this case the
SerDes will be configured as an interrupt controller and the zForce driver
will have no access to poll the GPIO line.
To support this, we add two dedicated new GPIOs in the device tree:
rst-gpio and int-gpio. With the int-gpio being optional, then.
To not break the existing device trees, the index based 'gpios' entries
are still supported, but marked as depreciated.
With this, if the interrupt GPIO is available, either via the old or new
device tree style, the while loop will read and handle the packets as long
as the GPIO indicates that the interrupt is asserted (existing, unchanged
driver behavior).
If the interrupt GPIO isn't available, i.e. not configured via the new
device tree style, we are falling back to one read per ISR invocation
(new behavior to support the SerDes).
Note that the gpiod functions help to handle the optional GPIO:
devm_gpiod_get_index_optional() will return NULL in case the interrupt
GPIO isn't available. And gpiod_get_value_cansleep() does cover this, too,
by returning 0 in this case.
@@ -4,12 +4,12 @@ Required properties: - compatible: must be "neonode,zforce" - reg: I2C address of the chip - interrupts: interrupt to which the chip is connected-- gpios: gpios the chip is connected to- first one is the interrupt gpio and second one the reset gpio+- rst-gpio: reset gpio the chip is connected to - x-size: horizontal resolution of touchscreen - y-size: vertical resolution of touchscreen Optional properties:+- int-gpio : interrupt gpio the chip is connected to - vdd-supply: Regulator controlling the controller supply Example:
@@ -754,6 +766,40 @@ static int zforce_probe(struct i2c_client *client,if(!ts)return-ENOMEM;+/*+*TheresetGPIOisn'toptional,butwemightgetit+*viatheoldstyleDTentriesbelow,too.Soit's+*notanerrorifwedon'tgetithere.Thereforeuse+*devm_gpiod_get_optional()here.+*/+ts->gpio_rst=devm_gpiod_get_optional(&client->dev,"rst",+GPIOD_OUT_HIGH);+if(IS_ERR(ts->gpio_rst)){+ret=PTR_ERR(ts->gpio_rst);+dev_err(&client->dev,+"failed to request reset GPIO: %d\n",ret);+returnret;+}++ts->gpio_int=devm_gpiod_get_optional(&client->dev,"int",GPIOD_IN);+if(IS_ERR(ts->gpio_int)){+ret=PTR_ERR(ts->gpio_int);+dev_err(&client->dev,+"failed to request interrupt GPIO: %d\n",ret);+returnret;+}++/* Skip the old style GPIO if we have the new one */+if(ts->gpio_rst)+gotoskip;++/*+*DepreciatedGPIOdevicetreehandlingforcompatibility+*withexistingdevicetrees.Fornewdevicetrees,don't+*useitanymore.Insteaduserst-gpioandtheoptional+*int-gpioabove.+*/+/* INT GPIO */ts->gpio_int=devm_gpiod_get_index(&client->dev,NULL,0,GPIOD_IN);if(IS_ERR(ts->gpio_int)){
@@ -773,6 +819,7 @@ static int zforce_probe(struct i2c_client *client,returnret;}+skip:ts->reg_vdd=devm_regulator_get_optional(&client->dev,"vdd");if(IS_ERR(ts->reg_vdd)){ret=PTR_ERR(ts->reg_vdd);
Hi,
it's nice to see that my driver seems to be used somewhere :-)
Am Dienstag, 28. Juli 2015, 14:06:21 schrieb Dmitry Torokhov:
On Tue, Jul 28, 2015 at 10:26:48AM +0200, Dirk Behme wrote:
quoted
Add support for hardware which uses an I2C Serializer / Deserializer
(SerDes) to communicate with the zFroce touch driver. In this case the
SerDes will be configured as an interrupt controller and the zForce driver
will have no access to poll the GPIO line.
To support this, we add two dedicated new GPIOs in the device tree:
rst-gpio and int-gpio. With the int-gpio being optional, then.
To not break the existing device trees, the index based 'gpios' entries
are still supported, but marked as depreciated.
^ typo deprecated
quoted
With this, if the interrupt GPIO is available, either via the old or new
device tree style, the while loop will read and handle the packets as long
as the GPIO indicates that the interrupt is asserted (existing, unchanged
driver behavior).
If the interrupt GPIO isn't available, i.e. not configured via the new
device tree style, we are falling back to one read per ISR invocation
(new behavior to support the SerDes).
Note that the gpiod functions help to handle the optional GPIO:
devm_gpiod_get_index_optional() will return NULL in case the interrupt
GPIO isn't available. And gpiod_get_value_cansleep() does cover this, too,
by returning 0 in this case.
@@ -4,12 +4,12 @@ Required properties: - compatible: must be "neonode,zforce" - reg: I2C address of the chip - interrupts: interrupt to which the chip is connected-- gpios: gpios the chip is connected to- first one is the interrupt gpio and second one the reset gpio+- rst-gpio: reset gpio the chip is connected to - x-size: horizontal resolution of touchscreen - y-size: vertical resolution of touchscreen Optional properties:+- int-gpio : interrupt gpio the chip is connected to - vdd-supply: Regulator controlling the controller supply Example:
just today in #armlinux I read that it's always supposed to be "x-gpios" even
if it's only one gpio being defined. The gpio core handles both but only the
variant with "s" at the end is actually specified, see
Documentation/devicetree/bindings/gpio/gpio.txt
Also I think it's common to not use abbreviation in the devicetree. so it
probably should be
reset-gpios =
irq-gpios =
*dev_id)>
if (!ts->suspending && device_may_wakeup(&client->dev))
pm_stay_awake(&client->dev);
- while (gpiod_get_value_cansleep(ts->gpio_int)) {
+ while (run) {
+ /*
+ * Exit the loop if either
+ * - the optional interrupt GPIO isn't specified
+ * (there is only one packet read per ISR invocation, then)
+ * or
+ * - the GPIO isn't active any more
+ * (packet read until the level GPIO indicates that there is
+ * no IRQ any more)
+ */
+ run = gpiod_get_value_cansleep(ts->gpio_int);
+
alteratively you could simply convert to a
/* Run at least once, or as long as the interrupt gpio is active. */
do {
...
} while(gpiod_get_value_cansleep(ts->gpio_int));
saving the additional variable run and a lot of lines, while still running at
least once.
quoted
ret = zforce_read_packet(ts, payload_buffer);
if (ret < 0) {
dev_err(&client->dev,
@@ -754,6 +766,40 @@ static int zforce_probe(struct i2c_client *client, if (!ts) return -ENOMEM;+ /*+ * The reset GPIO isn't optional, but we might get it+ * via the old style DT entries below, too. So it's+ * not an error if we don't get it here. Therefore use+ * devm_gpiod_get_optional() here.+ */
hmm, you shouldn't mix styles. I.e. the new binding is using reset- and irq-
gpios exclusively. And old devicetrees will use the old binding exclusively.
So if you don't find the reset-gpio here, you can jump to the legacy binding
directly.
+ if (IS_ERR(ts->gpio_int)) {
+ ret = PTR_ERR(ts->gpio_int);
+ dev_err(&client->dev,
+ "failed to request interrupt GPIO: %d\n", ret);
+ return ret;
+ }
+
+ /* Skip the old style GPIO if we have the new one */
+ if (ts->gpio_rst)
+ goto skip;
personally I would prefer to not introduce non-error-handling gotos. After you
tried to get the reset gpio you can already decide which paradigm to use, so
could do something like:
ts->gpio_rst = devm_gpiod_get_optional(&client->dev, "rst",
GPIOD_OUT_HIGH);
if (IS_ERR(ts->gpio_rst)) {
ret = PTR_ERR(ts->gpio_rst);
dev_err(&client->dev,
"failed to request reset GPIO: %d\n", ret);
return ret;
}
if (ts->gpio_rst) {
ts->gpio_int = devm_gpiod_get_optional(&client->dev, "int",
GPIOD_IN);
if (IS_ERR(ts->gpio_int)) {
ret = PTR_ERR(ts->gpio_int);
dev_err(&client->dev,
"failed to request interrupt GPIO: %d\n", ret);
return ret;
}
} else {
/* Deprecated gpio handling for legacy binding */
... old code ...
}
But I guess this is a matter of style and up to what Dmitry prefers.
quoted
+
+ /*
+ * Depreciated GPIO device tree handling for compatibility
^ Deprecated
quoted
+ * with existing device trees. For new device trees, don't
+ * use it any more. Instead use rst-gpio and the optional
+ * int-gpio above.
+ */
+
you can skip the second sentence in the comment. People are supposed to read
the binding docs and will thus only find the new binding there ;-)
quoted
/* INT GPIO */
ts->gpio_int = devm_gpiod_get_index(&client->dev, NULL, 0,
GPIOD_IN);
quoted
if (IS_ERR(ts->gpio_int)) {
@@ -773,6 +819,7 @@ static int zforce_probe(struct i2c_client *client, return ret; }+skip: ts->reg_vdd = devm_regulator_get_optional(&client->dev, "vdd"); if (IS_ERR(ts->reg_vdd)) { ret = PTR_ERR(ts->reg_vdd);
On Tue, Jul 28, 2015 at 11:43:20PM +0200, Heiko Stübner wrote:
Hi,
it's nice to see that my driver seems to be used somewhere :-)
Am Dienstag, 28. Juli 2015, 14:06:21 schrieb Dmitry Torokhov:
quoted
On Tue, Jul 28, 2015 at 10:26:48AM +0200, Dirk Behme wrote:
quoted
Add support for hardware which uses an I2C Serializer / Deserializer
(SerDes) to communicate with the zFroce touch driver. In this case the
SerDes will be configured as an interrupt controller and the zForce driver
will have no access to poll the GPIO line.
To support this, we add two dedicated new GPIOs in the device tree:
rst-gpio and int-gpio. With the int-gpio being optional, then.
To not break the existing device trees, the index based 'gpios' entries
are still supported, but marked as depreciated.
^ typo deprecated
quoted
quoted
With this, if the interrupt GPIO is available, either via the old or new
device tree style, the while loop will read and handle the packets as long
as the GPIO indicates that the interrupt is asserted (existing, unchanged
driver behavior).
If the interrupt GPIO isn't available, i.e. not configured via the new
device tree style, we are falling back to one read per ISR invocation
(new behavior to support the SerDes).
Note that the gpiod functions help to handle the optional GPIO:
devm_gpiod_get_index_optional() will return NULL in case the interrupt
GPIO isn't available. And gpiod_get_value_cansleep() does cover this, too,
by returning 0 in this case.
@@ -4,12 +4,12 @@ Required properties: - compatible: must be "neonode,zforce" - reg: I2C address of the chip - interrupts: interrupt to which the chip is connected-- gpios: gpios the chip is connected to- first one is the interrupt gpio and second one the reset gpio+- rst-gpio: reset gpio the chip is connected to - x-size: horizontal resolution of touchscreen - y-size: vertical resolution of touchscreen Optional properties:+- int-gpio : interrupt gpio the chip is connected to - vdd-supply: Regulator controlling the controller supply Example:
just today in #armlinux I read that it's always supposed to be "x-gpios" even
if it's only one gpio being defined. The gpio core handles both but only the
variant with "s" at the end is actually specified, see
Documentation/devicetree/bindings/gpio/gpio.txt
Also I think it's common to not use abbreviation in the devicetree. so it
probably should be
reset-gpios =
irq-gpios =
*dev_id)>
if (!ts->suspending && device_may_wakeup(&client->dev))
pm_stay_awake(&client->dev);
- while (gpiod_get_value_cansleep(ts->gpio_int)) {
+ while (run) {
+ /*
+ * Exit the loop if either
+ * - the optional interrupt GPIO isn't specified
+ * (there is only one packet read per ISR invocation, then)
+ * or
+ * - the GPIO isn't active any more
+ * (packet read until the level GPIO indicates that there is
+ * no IRQ any more)
+ */
+ run = gpiod_get_value_cansleep(ts->gpio_int);
+
alteratively you could simply convert to a
/* Run at least once, or as long as the interrupt gpio is active. */
do {
...
} while(gpiod_get_value_cansleep(ts->gpio_int));
saving the additional variable run and a lot of lines, while still running at
least once.
But I think that means that we'll try to read even if gpio is not active
anymore on the first iteration. So maybe we need to factor out the body
and do:
if (!ts->gpio_int)
zforce_report_data(...)
else
while (gpiod_get_value_cansleep(ts->gpio_int))
zforce_report_data(...);
Or we could install 2 different interrupt handlers, depending on the
presence of interrupt gpio.
quoted
quoted
ret = zforce_read_packet(ts, payload_buffer);
if (ret < 0) {
dev_err(&client->dev,
@@ -754,6 +766,40 @@ static int zforce_probe(struct i2c_client *client, if (!ts) return -ENOMEM;+ /*+ * The reset GPIO isn't optional, but we might get it+ * via the old style DT entries below, too. So it's+ * not an error if we don't get it here. Therefore use+ * devm_gpiod_get_optional() here.+ */
hmm, you shouldn't mix styles. I.e. the new binding is using reset- and irq-
gpios exclusively. And old devicetrees will use the old binding exclusively.
So if you don't find the reset-gpio here, you can jump to the legacy binding
directly.
+ if (IS_ERR(ts->gpio_int)) {
+ ret = PTR_ERR(ts->gpio_int);
+ dev_err(&client->dev,
+ "failed to request interrupt GPIO: %d\n", ret);
+ return ret;
+ }
+
+ /* Skip the old style GPIO if we have the new one */
+ if (ts->gpio_rst)
+ goto skip;
personally I would prefer to not introduce non-error-handling gotos. After you
tried to get the reset gpio you can already decide which paradigm to use, so
could do something like:
ts->gpio_rst = devm_gpiod_get_optional(&client->dev, "rst",
GPIOD_OUT_HIGH);
if (IS_ERR(ts->gpio_rst)) {
ret = PTR_ERR(ts->gpio_rst);
dev_err(&client->dev,
"failed to request reset GPIO: %d\n", ret);
return ret;
}
if (ts->gpio_rst) {
ts->gpio_int = devm_gpiod_get_optional(&client->dev, "int",
GPIOD_IN);
if (IS_ERR(ts->gpio_int)) {
ret = PTR_ERR(ts->gpio_int);
dev_err(&client->dev,
"failed to request interrupt GPIO: %d\n", ret);
return ret;
}
} else {
/* Deprecated gpio handling for legacy binding */
... old code ...
}
But I guess this is a matter of style and up to what Dmitry prefers.
Yes, I'd prefer this too, although I can see why Dirk used label as it
reduces the size of the patch considerably.
Thanks.
--
Dmitry
--
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
void
*dev_id)>
if (!ts->suspending && device_may_wakeup(&client->dev))
pm_stay_awake(&client->dev);
- while (gpiod_get_value_cansleep(ts->gpio_int)) {
+ while (run) {
+ /*
+ * Exit the loop if either
+ * - the optional interrupt GPIO isn't specified
+ * (there is only one packet read per ISR invocation,
then)
quoted
quoted
quoted
+ * or
+ * - the GPIO isn't active any more
+ * (packet read until the level GPIO indicates that there
is
quoted
quoted
quoted
+ * no IRQ any more)
+ */
+ run = gpiod_get_value_cansleep(ts->gpio_int);
+
alteratively you could simply convert to a
/* Run at least once, or as long as the interrupt gpio is active. */
do {
...
} while(gpiod_get_value_cansleep(ts->gpio_int));
saving the additional variable run and a lot of lines, while still running
at least once.
But I think that means that we'll try to read even if gpio is not active
anymore on the first iteration.
I don't think this is a possible scenario :-)
The interrupt is only used for transmission of touch-data and is pulled low as
long as the host hasn't fetched all touch-packages. So if nothing else except
the zforce-driver talks to the chip, the gpio will stay on.
And when we're entering the interrupt handler, there is at least one touch-
data package waiting.
So maybe we need to factor out the body
and do:
if (!ts->gpio_int)
zforce_report_data(...)
else
while (gpiod_get_value_cansleep(ts->gpio_int))
zforce_report_data(...);
Or we could install 2 different interrupt handlers, depending on the
presence of interrupt gpio.
Heiko
--
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
void
*dev_id)>
if (!ts->suspending && device_may_wakeup(&client->dev))
pm_stay_awake(&client->dev);
- while (gpiod_get_value_cansleep(ts->gpio_int)) {
+ while (run) {
+ /*
+ * Exit the loop if either
+ * - the optional interrupt GPIO isn't specified
+ * (there is only one packet read per ISR invocation,
then)
quoted
quoted
quoted
quoted
+ * or
+ * - the GPIO isn't active any more
+ * (packet read until the level GPIO indicates that there
is
quoted
quoted
quoted
quoted
+ * no IRQ any more)
+ */
+ run = gpiod_get_value_cansleep(ts->gpio_int);
+
alteratively you could simply convert to a
/* Run at least once, or as long as the interrupt gpio is active. */
do {
...
} while(gpiod_get_value_cansleep(ts->gpio_int));
saving the additional variable run and a lot of lines, while still running
at least once.
But I think that means that we'll try to read even if gpio is not active
anymore on the first iteration.
I don't think this is a possible scenario :-)
The interrupt is only used for transmission of touch-data and is pulled low as
long as the host hasn't fetched all touch-packages. So if nothing else except
the zforce-driver talks to the chip, the gpio will stay on.
And when we're entering the interrupt handler, there is at least one touch-
data package waiting.
I see. Then please ignore me ;)
--
Dmitry
--
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