* [PATCH v3 0/2] [PATCH v3 0/2] gpio: mmio: report the line direction on chips without direction registers
@ 2026-09-02 15:45 Mehmet Fide
2026-09-02 15:45 ` [PATCH v3 1/2] pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register Mehmet Fide
2026-09-02 15:45 ` [PATCH v3 2/2] gpio: mmio: track the direction of chips without direction registers Mehmet Fide
0 siblings, 2 replies; 6+ messages in thread
From: Mehmet Fide @ 2026-09-02 15:45 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij
Cc: Dong Aisheng, Fabio Estevam, Frank Li, Jacky Bai, Sascha Hauer,
Pengutronix Kernel Team, imx, linux-gpio, linux-arm-kernel,
linux-kernel, Mehmet Fide
From: Mehmet Fide <mehmet.fide@screeningeagle.com>
Hi Bartosz, Linus,
this replaces the gpiolib guard patch [1], along the lines Bartosz
suggested there: instead of teaching gpiod_get_direction() to stay quiet
when a chip has no get_direction(), give gpio-mmio one and let the pin
controller tell it what the pad does.
The user is the Vybrid GPIO block (gpio-vf610, a generic mmio chip with
GPIO_GENERIC_PINCTRL_BACKEND and no direction registers, the direction
lives in the iomuxc pad as the OBE bit). Today every
gpiod_get_direction() there trips the WARN in gpiolib, 21 backtraces per
boot on a Colibri VF61/VF50.
Patch 1 is the pinctrl-imx side. Bartosz asked whether the raw register
coming back from pin_config_get() is a bug in pinctrl-imx: it is, the
callback never looked at which parameter was requested. It now answers
PIN_CONFIG_OUTPUT_ENABLE and PIN_CONFIG_INPUT_ENABLE on SoCs that say
where those bits live (Vybrid: OBE bit 1, IBE bit 0) and -ENOTSUPP for
everything else, the SCU based SoCs included; the debugfs dump, the only
raw-register user, reads the register through its own helper. The set
callback stays raw, as the fsl,pins binding requires. Converting the
driver fully to generic pinconf is a bigger job than this fix needs.
Patch 2 keeps the direction in gpio-mmio's existing shadow and installs
the shadow-reading get_direction() for the "pinctrl backend, no
direction registers" combination. The pad is asked once, from request(),
in process context, so it is safe for the gpiochip_lock_as_irq() path
that calls get_direction() under the irq descriptor lock.
Tested on a Colibri VF61 (Iris carrier) on top of gpio/for-next, with
DEBUG_ATOMIC_SLEEP and PROVE_LOCKING enabled: no backtraces, and
/sys/kernel/debug/gpio shows the right direction for every requested
line (the hogs, the SD card detect input, the USB VBUS regulator
output). Lines the pin controller cannot answer for keep the input
default gpiolib assumed before, so nothing that worked before is
affected. The initial direction scan in gpiochip_add_data_with_key()
runs before the pin ranges exist and still guesses; only requested lines
get the real answer.
Patch 2 needs patch 1 to give correct answers; taking both through one
tree, with an ack from the other side, avoids the window.
[1] https://lore.kernel.org/linux-gpio/20260813193715.2346477-1-mehmet.fide@gmail.com/
Changes in v3:
- patch 1: return -ENOTSUPP for the SCU based SoCs instead of letting
the firmware call hand back the raw pad value (Sashiko review)
- patch 1: say in the commit message that the set callback stays raw
- patch 1: -EINVAL for a pin the device tree never configured, -ENOTSUPP
only for unsupported parameters and SoCs; reject a pin index beyond
npins, gpio-mmio is the first caller reaching this through a gpio range
(local Sashiko run)
- patch 1, 2: drop two comments that only restated the code
Changes in v2:
- patch 1: decode the requested parameter instead of returning the raw
register; debugfs group dump reads the register through its own helper
- patch 2: keep the direction in the gpio-mmio shadow, ask pinctrl once
from request() instead of from get_direction()
Mehmet Fide (2):
pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad
register
gpio: mmio: track the direction of chips without direction registers
drivers/gpio/gpio-mmio.c | 59 +++++++++++++++++++++--
drivers/pinctrl/freescale/pinctrl-imx.c | 56 +++++++++++++++++++--
drivers/pinctrl/freescale/pinctrl-imx.h | 4 ++
drivers/pinctrl/freescale/pinctrl-vf610.c | 2 +
4 files changed, 113 insertions(+), 8 deletions(-)
base-commit: 1900b5a41493e7050c68c1e62780d6fb9a209457
--
2.54.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 1/2] pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register
2026-09-02 15:45 [PATCH v3 0/2] [PATCH v3 0/2] gpio: mmio: report the line direction on chips without direction registers Mehmet Fide
@ 2026-09-02 15:45 ` Mehmet Fide
[not found] ` <20260902155931.38FCC1F000E9@smtp.kernel.org>
2026-09-02 15:45 ` [PATCH v3 2/2] gpio: mmio: track the direction of chips without direction registers Mehmet Fide
1 sibling, 1 reply; 6+ messages in thread
From: Mehmet Fide @ 2026-09-02 15:45 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij
Cc: Dong Aisheng, Fabio Estevam, Frank Li, Jacky Bai, Sascha Hauer,
Pengutronix Kernel Team, imx, linux-gpio, linux-arm-kernel,
linux-kernel, Mehmet Fide
From: Mehmet Fide <mehmet.fide@screeningeagle.com>
The mmio pinconf get callback ignores which parameter was requested and
returns the raw conf register, so a generic query through
pinctrl_gpio_get_config() gets register bits back instead of the packed
parameter it asked for.
Decode the requested parameter and answer PIN_CONFIG_OUTPUT_ENABLE and
PIN_CONFIG_INPUT_ENABLE on SoCs that declare where those bits live in
the pad register; Vybrid has OBE at bit 1 and IBE at bit 0. The answer
is 0 with the bit value as the argument, which is what the
pinctrl_gpio_get_config() users (gpio-by-pinctrl, and gpio-mmio in the
next patch) expect. Other parameters and the SCU based SoCs, whose
firmware call returns the raw pad value as well, get -ENOTSUPP; a pin
the device tree never configured gets -EINVAL, as the raw helper already
does, so a caller can tell "no answer for this pin" from "this
controller never answers".
The pin index comes from the gpio range unchecked and the driver indexes
flat arrays with it, so an out of range gpio-ranges entry would read
past pin_regs[]. Nothing called pin_config_get through a gpio range on
these SoCs before; now something will, so reject an index beyond npins.
The only in-tree user of the raw register was the debugfs group dump,
which called the callback with an uninitialized config; it now reads
the register through its own helper, like the single pin dump already
did.
The set callback is not touched: the fsl,pins binding hands it the raw
pad register value and that stays the only thing it accepts. Nothing
in-tree sends generic parameters to it on these SoCs; making it
understand them is a separate change.
This gives gpio-mmio a way to read back the line direction on chips
whose direction lives in the pin controller.
Suggested-by: Bartosz Golaszewski <brgl@kernel.org>
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
drivers/pinctrl/freescale/pinctrl-imx.c | 56 +++++++++++++++++++++--
drivers/pinctrl/freescale/pinctrl-imx.h | 4 ++
drivers/pinctrl/freescale/pinctrl-vf610.c | 2 +
3 files changed, 58 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 9a45b376d36f..1bcb2f772d38 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -21,6 +21,7 @@
#include <linux/pinctrl/machine.h>
#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
#include <linux/pinctrl/pinctrl.h>
#include <linux/pinctrl/pinmux.h>
@@ -291,8 +292,8 @@ struct pinmux_ops imx_pmx_ops = {
.set_mux = imx_pmx_set,
};
-static int imx_pinconf_get_mmio(struct pinctrl_dev *pctldev, unsigned pin_id,
- unsigned long *config)
+static int imx_pinconf_get_raw_mmio(struct pinctrl_dev *pctldev,
+ unsigned int pin_id, unsigned long *config)
{
struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
const struct imx_pinctrl_soc_info *info = ipctl->info;
@@ -312,16 +313,63 @@ static int imx_pinconf_get_mmio(struct pinctrl_dev *pctldev, unsigned pin_id,
return 0;
}
+static int imx_pinconf_get_mmio(struct pinctrl_dev *pctldev,
+ unsigned int pin_id, unsigned long *config)
+{
+ struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+ const struct imx_pinctrl_soc_info *info = ipctl->info;
+ const struct imx_pin_reg *pin_reg = &ipctl->pin_regs[pin_id];
+ enum pin_config_param param = pinconf_to_config_param(*config);
+ unsigned int mask;
+ u32 raw;
+
+ switch (param) {
+ case PIN_CONFIG_OUTPUT_ENABLE:
+ mask = info->obe_mask;
+ break;
+ case PIN_CONFIG_INPUT_ENABLE:
+ mask = info->ibe_mask;
+ break;
+ default:
+ mask = 0;
+ break;
+ }
+
+ if (!mask)
+ return -ENOTSUPP;
+ if (pin_reg->conf_reg == -1)
+ return -EINVAL;
+
+ raw = readl(ipctl->base + pin_reg->conf_reg);
+ *config = pinconf_to_config_packed(param, !!(raw & mask));
+
+ return 0;
+}
+
static int imx_pinconf_get(struct pinctrl_dev *pctldev,
unsigned pin_id, unsigned long *config)
{
struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
const struct imx_pinctrl_soc_info *info = ipctl->info;
+ if (info->flags & IMX_USE_SCU)
+ return -ENOTSUPP;
+ if (pin_id >= info->npins)
+ return -EINVAL;
+
+ return imx_pinconf_get_mmio(pctldev, pin_id, config);
+}
+
+static int imx_pinconf_get_raw(struct pinctrl_dev *pctldev,
+ unsigned int pin_id, unsigned long *config)
+{
+ struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+ const struct imx_pinctrl_soc_info *info = ipctl->info;
+
if (info->flags & IMX_USE_SCU)
return info->imx_pinconf_get(pctldev, pin_id, config);
else
- return imx_pinconf_get_mmio(pctldev, pin_id, config);
+ return imx_pinconf_get_raw_mmio(pctldev, pin_id, config);
}
static int imx_pinconf_set_mmio(struct pinctrl_dev *pctldev,
@@ -426,7 +474,7 @@ static void imx_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
struct imx_pin *pin = &((struct imx_pin *)(grp->data))[i];
name = pin_get_name(pctldev, pin->pin);
- ret = imx_pinconf_get(pctldev, pin->pin, &config);
+ ret = imx_pinconf_get_raw(pctldev, pin->pin, &config);
if (ret)
return;
seq_printf(s, " %s: 0x%lx\n", name, config);
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h
index f65ff45b4003..8fa7e1e2521d 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -91,6 +91,10 @@ struct imx_pinctrl_soc_info {
unsigned int mux_mask;
u8 mux_shift;
+ /* OBE/IBE bits in the conf register, 0 if the pad does not have them */
+ unsigned int obe_mask;
+ unsigned int ibe_mask;
+
int (*gpio_set_direction)(struct pinctrl_dev *pctldev,
struct pinctrl_gpio_range *range,
unsigned offset,
diff --git a/drivers/pinctrl/freescale/pinctrl-vf610.c b/drivers/pinctrl/freescale/pinctrl-vf610.c
index 76a4bc0181a0..77d077618782 100644
--- a/drivers/pinctrl/freescale/pinctrl-vf610.c
+++ b/drivers/pinctrl/freescale/pinctrl-vf610.c
@@ -319,6 +319,8 @@ static const struct imx_pinctrl_soc_info vf610_pinctrl_info = {
.gpio_set_direction = vf610_pmx_gpio_set_direction,
.mux_mask = 0x700000,
.mux_shift = 20,
+ .obe_mask = 0x2,
+ .ibe_mask = 0x1,
};
static const struct of_device_id vf610_pinctrl_of_match[] = {
--
2.54.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] gpio: mmio: track the direction of chips without direction registers
2026-09-02 15:45 [PATCH v3 0/2] [PATCH v3 0/2] gpio: mmio: report the line direction on chips without direction registers Mehmet Fide
2026-09-02 15:45 ` [PATCH v3 1/2] pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register Mehmet Fide
@ 2026-09-02 15:45 ` Mehmet Fide
2026-09-02 17:31 ` Linus Walleij
1 sibling, 1 reply; 6+ messages in thread
From: Mehmet Fide @ 2026-09-02 15:45 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij
Cc: Dong Aisheng, Fabio Estevam, Frank Li, Jacky Bai, Sascha Hauer,
Pengutronix Kernel Team, imx, linux-gpio, linux-arm-kernel,
linux-kernel, Mehmet Fide
From: Mehmet Fide <mehmet.fide@screeningeagle.com>
A generic chip with GPIO_GENERIC_PINCTRL_BACKEND and no direction
registers sets the direction through pinctrl but has no get_direction
callback, so every gpiod_get_direction() call trips the WARN in gpiolib
and the direction gpiolib reports is whatever it assumed. On a Vybrid
Colibri module that is 21 backtraces per boot.
Keep the direction of such a chip in the existing shadow: the direction
setters update sdir under the chip lock, and get_direction() is the
shadow-reading path already used for unreadable direction registers.
That keeps the callback usable in atomic context, which it has to be:
gpiochip_lock_as_irq() calls it for !can_sleep chips from
gpiochip_irq_domain_activate(), under the irq descriptor lock.
The pad's actual state is read once, in process context, when a line is
requested: gpiolib calls request() right before get_direction() for a
new line, so the shadow is seeded from PIN_CONFIG_OUTPUT_ENABLE there and
the line reports what the pin controller says. Lines pinctrl cannot
answer for keep the input default, which is what gpiolib assumed before.
Suggested-by: Bartosz Golaszewski <brgl@kernel.org>
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
drivers/gpio/gpio-mmio.c | 59 +++++++++++++++++++++++++++++++++++++---
1 file changed, 55 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index 7e4b3e8d609f..987285c33f45 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -49,6 +49,7 @@ o ` ~~~~\___/~~~~ ` controller in FPGA is ,.`
#include <linux/log2.h>
#include <linux/module.h>
#include <linux/pinctrl/consumer.h>
+#include <linux/pinctrl/pinconf-generic.h>
#include <linux/platform_device.h>
#include <linux/property.h>
#include <linux/spinlock.h>
@@ -372,7 +373,17 @@ static int gpio_mmio_dir_in_err(struct gpio_chip *gc, unsigned int gpio)
static int gpio_mmio_simple_dir_in(struct gpio_chip *gc, unsigned int gpio)
{
- return gpio_mmio_dir_return(gc, gpio, false);
+ struct gpio_generic_chip *chip = to_gpio_generic_chip(gc);
+ int ret;
+
+ ret = gpio_mmio_dir_return(gc, gpio, false);
+ if (ret)
+ return ret;
+
+ guard(raw_spinlock_irqsave)(&chip->lock);
+ chip->sdir &= ~gpio_mmio_line2mask(gc, gpio);
+
+ return 0;
}
static int gpio_mmio_dir_out_err(struct gpio_chip *gc, unsigned int gpio,
@@ -384,9 +395,19 @@ static int gpio_mmio_dir_out_err(struct gpio_chip *gc, unsigned int gpio,
static int gpio_mmio_simple_dir_out(struct gpio_chip *gc, unsigned int gpio,
int val)
{
+ struct gpio_generic_chip *chip = to_gpio_generic_chip(gc);
+ int ret;
+
gc->set(gc, gpio, val);
- return gpio_mmio_dir_return(gc, gpio, true);
+ ret = gpio_mmio_dir_return(gc, gpio, true);
+ if (ret)
+ return ret;
+
+ guard(raw_spinlock_irqsave)(&chip->lock);
+ chip->sdir |= gpio_mmio_line2mask(gc, gpio);
+
+ return 0;
}
static int gpio_mmio_dir_in(struct gpio_chip *gc, unsigned int gpio)
@@ -601,20 +622,50 @@ static int gpio_mmio_setup_direction(struct gpio_generic_chip *chip,
gc->direction_input = gpio_mmio_dir_in_err;
else
gc->direction_input = gpio_mmio_simple_dir_in;
+
+ if (cfg->flags & GPIO_GENERIC_PINCTRL_BACKEND) {
+ chip->dir_unreadable = true;
+ gc->get_direction = gpio_mmio_get_dir;
+ }
}
return 0;
}
+static void gpio_mmio_seed_dir_from_pinctrl(struct gpio_chip *gc,
+ unsigned int gpio)
+{
+ struct gpio_generic_chip *chip = to_gpio_generic_chip(gc);
+ unsigned long config;
+
+ if (!IS_ENABLED(CONFIG_PINCTRL) || chip->reg_dir_out || chip->reg_dir_in)
+ return;
+
+ config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
+ if (pinctrl_gpio_get_config(gc, gpio, &config))
+ return;
+
+ guard(raw_spinlock_irqsave)(&chip->lock);
+ if (config)
+ chip->sdir |= gpio_mmio_line2mask(gc, gpio);
+ else
+ chip->sdir &= ~gpio_mmio_line2mask(gc, gpio);
+}
+
static int gpio_mmio_request(struct gpio_chip *gc, unsigned int gpio_pin)
{
struct gpio_generic_chip *chip = to_gpio_generic_chip(gc);
+ int ret;
if (gpio_pin >= gc->ngpio)
return -EINVAL;
- if (chip->pinctrl)
- return gpiochip_generic_request(gc, gpio_pin);
+ if (chip->pinctrl) {
+ ret = gpiochip_generic_request(gc, gpio_pin);
+ if (ret)
+ return ret;
+ gpio_mmio_seed_dir_from_pinctrl(gc, gpio_pin);
+ }
return 0;
}
--
2.54.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register
[not found] ` <20260902155931.38FCC1F000E9@smtp.kernel.org>
@ 2026-09-02 17:18 ` Mehmet Fide
0 siblings, 0 replies; 6+ messages in thread
From: Mehmet Fide @ 2026-09-02 17:18 UTC (permalink / raw)
To: Bartosz Golaszewski, Linus Walleij
Cc: Dong Aisheng, Fabio Estevam, Frank Li, Jacky Bai, Sascha Hauer,
Pengutronix Kernel Team, imx, linux-gpio, linux-arm-kernel,
linux-kernel, Mehmet Fide, sashiko-bot
From: Mehmet Fide <mehmet.fide@screeningeagle.com>
> [Severity: High]
> This isn't a bug introduced by this patch, but since you are adding bounds
> checking here, does imx_pinconf_set() also need this verification?
>
> Without validation, a malformed gpio-ranges property in the device tree could
> translate to an invalid pin_id. Since imx_pinconf_set() lacks the
> if (pin_id >= info->npins) check, this unvalidated pin_id could cause an
> out-of-bounds array access in imx_pinconf_set_mmio():
>
> const struct imx_pin_reg *pin_reg = &ipctl->pin_regs[pin_id];
>
> Could the same missing bounds check also cause out-of-bounds array accesses to
> ipctl->pin_regs in the GPIO direction callbacks like imx_pmx_gpio_set_direction()
> and vf610_pmx_gpio_set_direction()?
No. Everything that reaches the driver through a gpio range goes through
pinctrl_gpio_request() first, and pin_request() looks the pin up with
pin_desc_get(), which fails for a pin the controller does not have. A bad
gpio-ranges entry therefore fails the GPIO request, and the direction and
config callbacks never see that index.
Which also means the npins check I added in this version is redundant
and its commit message overstates the problem. I can drop it in a v4 or
keep it as a cheap guard, whichever Bartosz and Linus prefer.
> [Severity: High]
> This is another pre-existing issue, but does imx_pinctrl_parse_pin_mmio() also
> lack bounds checking for *pin_id during device tree parsing?
>
> The pin_id is derived algebraically from raw device tree properties:
>
> *pin_id = (mux_reg != -1) ? mux_reg / 4 : conf_reg / 4;
> pin_reg = &ipctl->pin_regs[*pin_id];
> pin->pin = *pin_id;
> pin_reg->mux_reg = mux_reg;
> pin_reg->conf_reg = conf_reg;
>
> If the parsed *pin_id exceeds info->npins, this uses the unverified index
> directly on the ipctl->pin_regs array, leading to an immediate out-of-bounds
> memory write.
>
> Does imx_pinctrl_parse_pin_scu() have a similar vulnerability when indexing the
> info->pins array with an unverified pin->pin?
Yes, that is the one real hole, and it is where the fsl,pins value is
turned into an index. As said on v2, I will send a separate bounds check
for the parsers after this series.
> [Severity: High]
> This is a pre-existing issue, but can concurrent driver probes cause
> hardware state corruption of the shared input_reg in imx_pmx_set_one_pin_mmio()?
>
> There is an unprotected read-modify-write on shared general-purpose IOMUXC
> registers:
>
> val = readl(ipctl->base + pin_mmio->input_reg);
> val &= ~mask;
> val |= select << shift;
> writel(val, ipctl->base + pin_mmio->input_reg);
>
> If two drivers concurrently request pinctrl states that share the same
> general-purpose register, and the pinctrl core calls pinmux_enable_setting()
> locklessly, will these RMW operations race and overwrite each other's
> configurations?
The core does call set_mux without a lock and the driver has none around
that read-modify-write, so in principle yes. This series does not touch
the mux path and I have not seen it on Vybrid, so I leave that one to
the i.MX maintainers.
Mehmet
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] gpio: mmio: track the direction of chips without direction registers
2026-09-02 15:45 ` [PATCH v3 2/2] gpio: mmio: track the direction of chips without direction registers Mehmet Fide
@ 2026-09-02 17:31 ` Linus Walleij
2026-09-02 22:25 ` Mehmet Fide
0 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2026-09-02 17:31 UTC (permalink / raw)
To: Mehmet Fide
Cc: Bartosz Golaszewski, Dong Aisheng, Fabio Estevam, Frank Li,
Jacky Bai, Sascha Hauer, Pengutronix Kernel Team, imx, linux-gpio,
linux-arm-kernel, linux-kernel, Mehmet Fide
Hi Mehmet,
thanks for your patch!
On Wed, Sep 2, 2026 at 5:45 PM Mehmet Fide <mehmet.fide@gmail.com> wrote:
> From: Mehmet Fide <mehmet.fide@screeningeagle.com>
>
> A generic chip with GPIO_GENERIC_PINCTRL_BACKEND and no direction
> registers sets the direction through pinctrl but has no get_direction
> callback, so every gpiod_get_direction() call trips the WARN in gpiolib
> and the direction gpiolib reports is whatever it assumed. On a Vybrid
> Colibri module that is 21 backtraces per boot.
>
> Keep the direction of such a chip in the existing shadow: the direction
> setters update sdir under the chip lock, and get_direction() is the
> shadow-reading path already used for unreadable direction registers.
> That keeps the callback usable in atomic context, which it has to be:
> gpiochip_lock_as_irq() calls it for !can_sleep chips from
> gpiochip_irq_domain_activate(), under the irq descriptor lock.
>
> The pad's actual state is read once, in process context, when a line is
> requested: gpiolib calls request() right before get_direction() for a
> new line, so the shadow is seeded from PIN_CONFIG_OUTPUT_ENABLE there and
> the line reports what the pin controller says. Lines pinctrl cannot
> answer for keep the input default, which is what gpiolib assumed before.
>
> Suggested-by: Bartosz Golaszewski <brgl@kernel.org>
> Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
For one I really like the approach to ask the pin controller
back-end about the direction state when the GPIO registers
doesn't know this!
I have more of an implementation question here:
gpiolib already supports this:
struct gpio_chip {
(...)
int (*set_config)(struct gpio_chip *gc,
unsigned int offset,
unsigned long config);
this sets one specific config at a time. With a generic pin control
back-end it is simply populated with gpiochip_generic_config()
from gpiolib.c which will call pinctrl_gpio_set_config() for
the corresponding pin.
What about just implementing generic optional get_config()
in struct gpio_chip, implement a likewise generic
gpiochip_generic_get_config() in gpiolib and use that as
the fallback?
int (*get_config)(struct gpio_chip *gc,
unsigned int offset,
unsigned long *config);
Then the implementation becomes pretty straight-forward
from that point, and this:
+static void gpio_mmio_seed_dir_from_pinctrl(struct gpio_chip *gc,
+ unsigned int gpio)
+{
+ struct gpio_generic_chip *chip = to_gpio_generic_chip(gc);
+ unsigned long config;
+
+ if (!IS_ENABLED(CONFIG_PINCTRL) || chip->reg_dir_out ||
chip->reg_dir_in)
+ return;
+
+ config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
+ if (pinctrl_gpio_get_config(gc, gpio, &config))
+ return;
Can drop all the checks for !IS_ENABLED(CONFIG_PINCTRL) as this is done
generically in in gpiolib and:
+ config = pinconf_to_config_packed(PIN_CONFIG_OUTPUT_ENABLE, 0);
+ if (gc->get_config(gc, gpio, &config))
+ return;
I don't think it is necessary to provide any consumer API for this
such as gpiod_get_config(gpiod); as no-one really needs it, we can
keep it as a private thing in struct gpio_chip for now.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] gpio: mmio: track the direction of chips without direction registers
2026-09-02 17:31 ` Linus Walleij
@ 2026-09-02 22:25 ` Mehmet Fide
0 siblings, 0 replies; 6+ messages in thread
From: Mehmet Fide @ 2026-09-02 22:25 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski
Cc: Dong Aisheng, Fabio Estevam, Frank Li, Jacky Bai, Sascha Hauer,
Pengutronix Kernel Team, imx, linux-gpio, linux-arm-kernel,
linux-kernel, Mehmet Fide
From: Mehmet Fide <mehmet.fide@screeningeagle.com>
> What about just implementing generic optional get_config()
> in struct gpio_chip, implement a likewise generic
> gpiochip_generic_get_config() in gpiolib and use that as
> the fallback?
Yes, that is cleaner: gpio-mmio then knows nothing about pinctrl, the
same way it does not for set_config today. v4 will add the callback and
gpiochip_generic_get_config() as a mirror of gpiochip_generic_config(),
with gpio-mmio installing it for the pinctrl backend and seeding the
shadow through gc->get_config.
One detail for the generic helper: with CONFIG_PINCTRL off the
pinctrl_gpio_get_config() stub returns 0 and leaves *config alone, so
the helper returns -ENOTSUPP there instead of pretending it answered,
like gpiochip_generic_config() does for a chip without pin ranges.
> I don't think it is necessary to provide any consumer API for this
> such as gpiod_get_config(gpiod); as no-one really needs it, we can
> keep it as a private thing in struct gpio_chip for now.
Agreed, nothing outside the chip needs it.
Patch 1 stays as it is, minus the npins check I already told the
Sashiko bot was redundant.
Thanks,
Mehmet
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-02 22:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-02 15:45 [PATCH v3 0/2] [PATCH v3 0/2] gpio: mmio: report the line direction on chips without direction registers Mehmet Fide
2026-09-02 15:45 ` [PATCH v3 1/2] pinctrl: imx: answer OUTPUT_ENABLE/INPUT_ENABLE queries from the pad register Mehmet Fide
[not found] ` <20260902155931.38FCC1F000E9@smtp.kernel.org>
2026-09-02 17:18 ` Mehmet Fide
2026-09-02 15:45 ` [PATCH v3 2/2] gpio: mmio: track the direction of chips without direction registers Mehmet Fide
2026-09-02 17:31 ` Linus Walleij
2026-09-02 22:25 ` Mehmet Fide
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox