| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <YwAx38N0ICE37Vu9@google.com>
Date: Fri, 19 Aug 2022 17:59:11 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Marco Felsch <m.felsch@...gutronix.de>
Cc: Gireesh.Hiremath@...bosch.com, linux-omap@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-input@...r.kernel.org, bcousson@...libre.com,
tony@...mide.com, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, mkorpershoek@...libre.com,
davidgow@...gle.com, swboyd@...omium.org, fengping.yu@...iatek.com,
y.oudjana@...tonmail.com, rdunlap@...radead.org,
colin.king@...el.com, sjoerd.simons@...labora.co.uk,
VinayKumar.Shettar@...bosch.com,
Govindaraji.Sivanantham@...bosch.com, anaclaudia.dias@...bosch.com
Subject: Re: [PATCH v3 1/3] driver: input: matric-keypad: switch to gpiod
On Fri, Aug 19, 2022 at 03:12:31PM +0200, Marco Felsch wrote:
> Hi Gireesh,
>
> On 22-08-19, Gireesh.Hiremath@...bosch.com wrote:
> > From: Gireesh Hiremath <Gireesh.Hiremath@...bosch.com>
> >
> > Switch from gpio API to gpiod API
>
> Nice change.
>
> Did you checked the users of this driver? This change changes the
> behaviour for actice_low GPIOs. A quick check showed that at least on
> upstream board: arch/arm/mach-pxa/palmtc.c uses active low GPIOs.
>
> > Signed-off-by: Gireesh Hiremath <Gireesh.Hiremath@...bosch.com>
> >
> > Gbp-Pq: Topic apertis/guardian
> > Gbp-Pq: Name driver-input-matric-keypad-switch-gpio-to-gpiod.patch
>
> Please drop this internal tags.
>
> > ---
> > drivers/input/keyboard/matrix_keypad.c | 84 ++++++++++----------------
> > include/linux/input/matrix_keypad.h | 4 +-
> > 2 files changed, 33 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
> > index 30924b57058f..b99574edad9c 100644
> > --- a/drivers/input/keyboard/matrix_keypad.c
> > +++ b/drivers/input/keyboard/matrix_keypad.c
> > @@ -15,11 +15,10 @@
> > #include <linux/interrupt.h>
> > #include <linux/jiffies.h>
> > #include <linux/module.h>
> > -#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> > #include <linux/input/matrix_keypad.h>
> > #include <linux/slab.h>
> > #include <linux/of.h>
> > -#include <linux/of_gpio.h>
> > #include <linux/of_platform.h>
> >
> > struct matrix_keypad {
> > @@ -49,11 +48,11 @@ static void __activate_col(const struct matrix_keypad_platform_data *pdata,
> > bool level_on = !pdata->active_low;
> >
> > if (on) {
> > - gpio_direction_output(pdata->col_gpios[col], level_on);
> > + gpiod_direction_output(pdata->col_gpios[col], level_on);
>
> Now strange things can happen, if active_low is set and you specified
> GPIO_ACTIVE_LOW within the device-tree. We need a way to move to gpiod
> and keep the current behaviour.
>
> > } else {
> > - gpio_set_value_cansleep(pdata->col_gpios[col], !level_on);
> > + gpiod_set_value_cansleep(pdata->col_gpios[col], !level_on);
> > if (!pdata->drive_inactive_cols)
> > - gpio_direction_input(pdata->col_gpios[col]);
> > + gpiod_direction_input(pdata->col_gpios[col]);
> > }
> > }
> >
> > @@ -78,7 +77,7 @@ static void activate_all_cols(const struct matrix_keypad_platform_data *pdata,
> > static bool row_asserted(const struct matrix_keypad_platform_data *pdata,
> > int row)
> > {
> > - return gpio_get_value_cansleep(pdata->row_gpios[row]) ?
> > + return gpiod_get_value_cansleep(pdata->row_gpios[row]) ?
> > !pdata->active_low : pdata->active_low;
> > }
> >
> > @@ -91,7 +90,7 @@ static void enable_row_irqs(struct matrix_keypad *keypad)
> > enable_irq(pdata->clustered_irq);
> > else {
> > for (i = 0; i < pdata->num_row_gpios; i++)
> > - enable_irq(gpio_to_irq(pdata->row_gpios[i]));
> > + enable_irq(gpiod_to_irq(pdata->row_gpios[i]));
> > }
> > }
> >
> > @@ -104,7 +103,7 @@ static void disable_row_irqs(struct matrix_keypad *keypad)
> > disable_irq_nosync(pdata->clustered_irq);
> > else {
> > for (i = 0; i < pdata->num_row_gpios; i++)
> > - disable_irq_nosync(gpio_to_irq(pdata->row_gpios[i]));
> > + disable_irq_nosync(gpiod_to_irq(pdata->row_gpios[i]));
> > }
> > }
> >
> > @@ -230,7 +229,7 @@ static void matrix_keypad_stop(struct input_dev *dev)
> > static void matrix_keypad_enable_wakeup(struct matrix_keypad *keypad)
> > {
> > const struct matrix_keypad_platform_data *pdata = keypad->pdata;
> > - unsigned int gpio;
> > + struct gpio_desc *gpio;
> > int i;
> >
> > if (pdata->clustered_irq > 0) {
> > @@ -242,7 +241,7 @@ static void matrix_keypad_enable_wakeup(struct matrix_keypad *keypad)
> > if (!test_bit(i, keypad->disabled_gpios)) {
> > gpio = pdata->row_gpios[i];
> >
> > - if (enable_irq_wake(gpio_to_irq(gpio)) == 0)
> > + if (enable_irq_wake(gpiod_to_irq(gpio)) == 0)
> > __set_bit(i, keypad->disabled_gpios);
> > }
> > }
> > @@ -252,7 +251,7 @@ static void matrix_keypad_enable_wakeup(struct matrix_keypad *keypad)
> > static void matrix_keypad_disable_wakeup(struct matrix_keypad *keypad)
> > {
> > const struct matrix_keypad_platform_data *pdata = keypad->pdata;
> > - unsigned int gpio;
> > + struct gpio_desc *gpio;
> > int i;
> >
> > if (pdata->clustered_irq > 0) {
> > @@ -264,7 +263,7 @@ static void matrix_keypad_disable_wakeup(struct matrix_keypad *keypad)
> > for (i = 0; i < pdata->num_row_gpios; i++) {
> > if (test_and_clear_bit(i, keypad->disabled_gpios)) {
> > gpio = pdata->row_gpios[i];
> > - disable_irq_wake(gpio_to_irq(gpio));
> > + disable_irq_wake(gpiod_to_irq(gpio));
> > }
> > }
> > }
> > @@ -308,27 +307,11 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev,
> >
> > /* initialized strobe lines as outputs, activated */
> > for (i = 0; i < pdata->num_col_gpios; i++) {
> > - err = gpio_request(pdata->col_gpios[i], "matrix_kbd_col");
> > - if (err) {
> > - dev_err(&pdev->dev,
> > - "failed to request GPIO%d for COL%d\n",
> > - pdata->col_gpios[i], i);
> > - goto err_free_cols;
> > - }
> > -
> > - gpio_direction_output(pdata->col_gpios[i], !pdata->active_low);
> > + gpiod_direction_output(pdata->col_gpios[i], !pdata->active_low);
> > }
> >
> > for (i = 0; i < pdata->num_row_gpios; i++) {
> > - err = gpio_request(pdata->row_gpios[i], "matrix_kbd_row");
> > - if (err) {
> > - dev_err(&pdev->dev,
> > - "failed to request GPIO%d for ROW%d\n",
> > - pdata->row_gpios[i], i);
> > - goto err_free_rows;
> > - }
> > -
> > - gpio_direction_input(pdata->row_gpios[i]);
> > + gpiod_direction_input(pdata->row_gpios[i]);
> > }
> >
> > if (pdata->clustered_irq > 0) {
> > @@ -344,7 +327,7 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev,
> > } else {
> > for (i = 0; i < pdata->num_row_gpios; i++) {
> > err = request_any_context_irq(
> > - gpio_to_irq(pdata->row_gpios[i]),
> > + gpiod_to_irq(pdata->row_gpios[i]),
> > matrix_keypad_interrupt,
> > IRQF_TRIGGER_RISING |
> > IRQF_TRIGGER_FALLING,
> > @@ -352,7 +335,7 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev,
> > if (err < 0) {
> > dev_err(&pdev->dev,
> > "Unable to acquire interrupt for GPIO line %i\n",
> > - pdata->row_gpios[i]);
> > + i);
> > goto err_free_irqs;
> > }
> > }
> > @@ -364,15 +347,12 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev,
> >
> > err_free_irqs:
> > while (--i >= 0)
> > - free_irq(gpio_to_irq(pdata->row_gpios[i]), keypad);
> > + free_irq(gpiod_to_irq(pdata->row_gpios[i]), keypad);
> > i = pdata->num_row_gpios;
> > err_free_rows:
> > while (--i >= 0)
> > - gpio_free(pdata->row_gpios[i]);
> > + gpiod_put(pdata->row_gpios[i]);
> > i = pdata->num_col_gpios;
> > -err_free_cols:
> > - while (--i >= 0)
> > - gpio_free(pdata->col_gpios[i]);
> >
> > return err;
> > }
> > @@ -386,14 +366,14 @@ static void matrix_keypad_free_gpio(struct matrix_keypad *keypad)
> > free_irq(pdata->clustered_irq, keypad);
> > } else {
> > for (i = 0; i < pdata->num_row_gpios; i++)
> > - free_irq(gpio_to_irq(pdata->row_gpios[i]), keypad);
> > + free_irq(gpiod_to_irq(pdata->row_gpios[i]), keypad);
> > }
> >
> > for (i = 0; i < pdata->num_row_gpios; i++)
> > - gpio_free(pdata->row_gpios[i]);
> > + gpiod_put(pdata->row_gpios[i]);
> >
> > for (i = 0; i < pdata->num_col_gpios; i++)
> > - gpio_free(pdata->col_gpios[i]);
> > + gpiod_put(pdata->col_gpios[i]);
> > }
> >
> > #ifdef CONFIG_OF
> > @@ -402,7 +382,8 @@ matrix_keypad_parse_dt(struct device *dev)
> > {
> > struct matrix_keypad_platform_data *pdata;
> > struct device_node *np = dev->of_node;
> > - unsigned int *gpios;
> > + struct gpio_desc **gpios;
> > + struct gpio_desc *desc;
> > int ret, i, nrow, ncol;
> >
> > if (!np) {
> > @@ -416,8 +397,8 @@ matrix_keypad_parse_dt(struct device *dev)
> > return ERR_PTR(-ENOMEM);
> > }
> >
> > - pdata->num_row_gpios = nrow = of_gpio_named_count(np, "row-gpios");
> > - pdata->num_col_gpios = ncol = of_gpio_named_count(np, "col-gpios");
> > + pdata->num_row_gpios = nrow = gpiod_count(dev, "row");
> > + pdata->num_col_gpios = ncol = gpiod_count(dev, "col");
> > if (nrow <= 0 || ncol <= 0) {
> > dev_err(dev, "number of keypad rows/columns not specified\n");
> > return ERR_PTR(-EINVAL);
> > @@ -429,9 +410,6 @@ matrix_keypad_parse_dt(struct device *dev)
> > pdata->wakeup = of_property_read_bool(np, "wakeup-source") ||
> > of_property_read_bool(np, "linux,wakeup"); /* legacy */
> >
> > - if (of_get_property(np, "gpio-activelow", NULL))
> > - pdata->active_low = true;
> > -
>
> This removes backward compatibility, please don't do that.
I do not think there is a reasonable way of keeping the compatibility
while using gpiod API (sans abandoning polarity handling and using
*_raw() versions of API).
I would adjust the DTSes in the kernel and move on, especially given
that the DTSes in the kernel are inconsistent - they specify
gpio-activelow attribute while specifying "action high" in gpio
properties).
Thanks.
--
Dmitry
Powered by blists - more mailing lists