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:
> > 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.
> ^ typo deprecated
>
> > >
> > > 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.
> >
> > Let's let Heiko take a look as well.
> >
> > > Signed-off-by: Dirk Behme <dirk.behme@xxxxxxxxxxxx>
> > > ---
> > >
> > > .../bindings/input/touchscreen/zforce_ts.txt | 8 ++--
> > > drivers/input/touchscreen/zforce_ts.c | 49
> > > +++++++++++++++++++++- 2 files changed, 52 insertions(+), 5 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> > > b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt index
> > > 80c37df..c6be925 100644
> > > --- a/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
> > >
> > > @@ -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:
> > > @@ -23,8 +23,8 @@ Example:
> > > interrupts = <2 0>;
> > > vdd-supply = <®_zforce_vdd>;
> > >
> > > - gpios = <&gpio5 6 0>, /* INT */
> > > - <&gpio5 9 0>; /* RST */
> > > + rst-gpio = <&gpio5 9 0>; /* RST */
> > > + int-gpio = <&gpio5 6 0>; /* INT, optional */
>
> 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 =
Agree.
>
>
> > >
> > > x-size = <800>;
> > > y-size = <600>;
> > >
> > > diff --git a/drivers/input/touchscreen/zforce_ts.c
> > > b/drivers/input/touchscreen/zforce_ts.c index edf01c3..bf1e944 100644
> > > --- a/drivers/input/touchscreen/zforce_ts.c
> > > +++ b/drivers/input/touchscreen/zforce_ts.c
> > > @@ -494,6 +494,7 @@ static irqreturn_t zforce_irq_thread(int irq, void
> > > *dev_id)>
> > > int ret;
> > > u8 payload_buffer[FRAME_MAXSIZE];
> > > u8 *payload;
> > >
> > > + int run = 1;
> > >
> > > /*
> > >
> > > * When still suspended, return.
> > >
> > > @@ -510,7 +511,18 @@ static irqreturn_t zforce_irq_thread(int irq, 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)
> > > + * 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.
>
>
> > >
> > > 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.
>
>
> > > + 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;
> > > + }
> > > +
> > > + 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;
> > > + }
> > > +
> > > + /* 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
