Re: [PATCH v1 1/2] Input: tsc2007 - convert to GPIO descriptors

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Mon, Mar 08, 2021 at 11:10:38AM +0200, Andy Shevchenko wrote:
> On Mon, Mar 8, 2021 at 12:57 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > On Mon, Mar 08, 2021 at 12:05:48AM +0200, Andy Shevchenko wrote:
> 
> ...
> 
> > > -     return !gpio_get_value(ts->gpio);
> > > +     return !gpiod_get_value(ts->gpiod);
> >
> > This is not correct. gpio_get_value() is raw polarity vs
> > gpiod_get_value() using logical active/inactive, and tsc2007 GPIO lines
> > are active low. The negation must be dropped after switching to GPIOD
> > API.
> 
> Ah, indeed, I missed that, thanks!
> 
> ...
> 
> > > -     ts->gpio = of_get_gpio(np, 0);
> > > -     if (gpio_is_valid(ts->gpio))
> > > -             ts->get_pendown_state = tsc2007_get_pendown_state_gpio;
> > > -     else
> > > -             dev_warn(&client->dev,
> > > -                      "GPIO not specified in DT (of_get_gpio returned %d)\n",
> > > -                      ts->gpio);
> > > +     ts->gpiod = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
> >
> > GPIO is definitely not optional in DT case, at least in the way the
> > driver written right now.
> 
> Can you elaborate this, please? I don't see from the dev_warn() w/o
> any error code returned that it's mandatory.
> In the bindings one may read:
> 
>   Optional properties:
>   - gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
>     The penirq pin goes to low when the panel is touched.
>     (see GPIO binding[1] for more details).
> 
> Nothing suggested it's mandatory. What have I missed?

Ah, indeed, I misread the code and thought we'd abort if there is no
pendown GPIO. I wonder if we should remove the warning since we seem to
support operations without it.

Thanks.

-- 
Dmitry