| Message ID | 20210603043726.3793876-1-dmitry.torokhov@gmail.com |
|---|---|
| State | Accepted |
| Commit | 6abee582034c123d995cd454a1ccdcf0b8699da0 |
| Headers | show |
| Series | [1/7] Input: cy8ctmg110_ts - rely on platform code to supply interrupt | expand |
Hi Dmitry, I see you noticed that there is no upstream board defining any cy8ctmg110_pdata, so I don't see any problem with fixing this. Outoftree users can adopt. On Thu, Jun 3, 2021 at 6:37 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > Instead of using platform data to specify GPIO that is used as interrupt > source, rely on the platform and I2C core to set it up properly. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > - client->irq = gpio_to_irq(ts->irq_pin); This looks like a violation of the struct anyway.... Yours, Linus Walleij
On Thu, Jun 3, 2021 at 6:37 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Rely on the platform to set up interrupt polarity/type properly instead > of hard-coding falling edge. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Jun 3, 2021 at 6:37 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > I2C core already configures interrupt as wakeup source when device is > registered using I2C_CLIENT_WAKE flag, so let's rely on it instead of > configuring it ourselves. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> I wonder how many bugs of this deep semantic type we have in the kernel :/ Yours, Linus Walleij
On Thu, Jun 3, 2021 at 6:37 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > This simplifies error handling paths and allows to get rid of > cy8ctmg110_remove() method. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Neat! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Jun 3, 2021 at 6:37 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Instead of legacy gpio API let's use newer gpiod API. This also allows us > to get rid of platform data. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > static void cy8ctmg110_power(struct cy8ctmg110 *ts, bool poweron) > { > - if (ts->reset_pin) > - gpio_direction_output(ts->reset_pin, 1 - poweron); > + if (ts->reset_gpio) > + gpiod_set_value_cansleep(ts->reset_gpio, !poweron); I would perhaps drop in a comment that this de-asserts the RESET line when we power on and asserts it when we power off. > + ts->reset_gpio = devm_gpiod_get_optional(&client->dev, NULL, > + GPIOD_OUT_HIGH); And here that this will assert RESET. But that is just nitpicks, the code is fine. Yours, Linus Walleij
On Fri, Jun 04, 2021 at 09:38:04AM +0200, Linus Walleij wrote: > On Thu, Jun 3, 2021 at 6:37 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > Instead of legacy gpio API let's use newer gpiod API. This also allows us > > to get rid of platform data. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > static void cy8ctmg110_power(struct cy8ctmg110 *ts, bool poweron) > > { > > - if (ts->reset_pin) > > - gpio_direction_output(ts->reset_pin, 1 - poweron); > > + if (ts->reset_gpio) > > + gpiod_set_value_cansleep(ts->reset_gpio, !poweron); > > I would perhaps drop in a comment that this de-asserts the RESET > line when we power on and asserts it when we power off. > > > + ts->reset_gpio = devm_gpiod_get_optional(&client->dev, NULL, > > + GPIOD_OUT_HIGH); > > And here that this will assert RESET. Good idea, added and applied. Thank you Linus. -- Dmitry
On Fri, Jun 04, 2021 at 09:32:53AM +0200, Linus Walleij wrote: > On Thu, Jun 3, 2021 at 6:37 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > I2C core already configures interrupt as wakeup source when device is > > registered using I2C_CLIENT_WAKE flag, so let's rely on it instead of > > configuring it ourselves. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > I wonder how many bugs of this deep semantic type we have in the kernel :/ I do not think this is necessarily a bug, it just shows age of the driver that was written before I2C core was doing this set up for us. Thanks. -- Dmitry
diff --git a/drivers/input/touchscreen/cy8ctmg110_ts.c b/drivers/input/touchscreen/cy8ctmg110_ts.c index f465bae618fe..691f35f1bdd7 100644 --- a/drivers/input/touchscreen/cy8ctmg110_ts.c +++ b/drivers/input/touchscreen/cy8ctmg110_ts.c @@ -46,7 +46,6 @@ struct cy8ctmg110 { char phys[32]; struct i2c_client *client; int reset_pin; - int irq_pin; }; /* @@ -191,7 +190,6 @@ static int cy8ctmg110_probe(struct i2c_client *client, ts->client = client; ts->input = input_dev; ts->reset_pin = pdata->reset_pin; - ts->irq_pin = pdata->irq_pin; snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&client->dev)); @@ -222,38 +220,13 @@ static int cy8ctmg110_probe(struct i2c_client *client, cy8ctmg110_power(ts, true); cy8ctmg110_set_sleepmode(ts, false); - err = gpio_request(ts->irq_pin, "touch_irq_key"); - if (err < 0) { - dev_err(&client->dev, - "Failed to request GPIO %d, error %d\n", - ts->irq_pin, err); - goto err_shutoff_device; - } - - err = gpio_direction_input(ts->irq_pin); - if (err < 0) { - dev_err(&client->dev, - "Failed to configure input direction for GPIO %d, error %d\n", - ts->irq_pin, err); - goto err_free_irq_gpio; - } - - client->irq = gpio_to_irq(ts->irq_pin); - if (client->irq < 0) { - err = client->irq; - dev_err(&client->dev, - "Unable to get irq number for GPIO %d, error %d\n", - ts->irq_pin, err); - goto err_free_irq_gpio; - } - err = request_threaded_irq(client->irq, NULL, cy8ctmg110_irq_thread, IRQF_TRIGGER_RISING | IRQF_ONESHOT, "touch_reset_key", ts); if (err < 0) { dev_err(&client->dev, "irq %d busy? error %d\n", client->irq, err); - goto err_free_irq_gpio; + goto err_shutoff_device; } err = input_register_device(input_dev); @@ -266,8 +239,6 @@ static int cy8ctmg110_probe(struct i2c_client *client, err_free_irq: free_irq(client->irq, ts); -err_free_irq_gpio: - gpio_free(ts->irq_pin); err_shutoff_device: cy8ctmg110_set_sleepmode(ts, true); cy8ctmg110_power(ts, false); @@ -318,7 +289,6 @@ static int cy8ctmg110_remove(struct i2c_client *client) free_irq(client->irq, ts); input_unregister_device(ts->input); - gpio_free(ts->irq_pin); if (ts->reset_pin) gpio_free(ts->reset_pin); kfree(ts); diff --git a/include/linux/input/cy8ctmg110_pdata.h b/include/linux/input/cy8ctmg110_pdata.h index 77582ae1745a..ee1d44545f30 100644 --- a/include/linux/input/cy8ctmg110_pdata.h +++ b/include/linux/input/cy8ctmg110_pdata.h @@ -5,7 +5,6 @@ struct cy8ctmg110_pdata { int reset_pin; /* Reset pin is wired to this GPIO (optional) */ - int irq_pin; /* IRQ pin is wired to this GPIO */ }; #endif
Instead of using platform data to specify GPIO that is used as interrupt source, rely on the platform and I2C core to set it up properly. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/touchscreen/cy8ctmg110_ts.c | 32 +---------------------- include/linux/input/cy8ctmg110_pdata.h | 1 - 2 files changed, 1 insertion(+), 32 deletions(-)