MCP23S17 bias pull-up not working

[KawaiiNahida]

Describe the bug

pinctrl-mcp23s08 driver unable to enable pull-up resister on MCP23S17(dtoverlay=mcp23s17)

Steps to reproduce the behaviour

using the following command to read pin 0 on gpiochip2 (MCP23S17 on SPI0.0)

gpioget --bias=pull-up gpiochip2 0
# got 0, expect 1

Device (s)

Raspberry Pi 4 Mod. B

System

Linux dev 6.12.20+rpt-rpi-v8 #1 SMP PREEMPT Debian 1:6.12.20-1+rpt1~bpo12+1 (2025-03-19) aarch64 GNU/Linux

Logs

No response

Additional context

reading GPPUA directly from device return 0b00000000, which indicate that pull-up is not enabled

[pelwell]

Has this worked before with older kernels?

[KawaiiNahida]

It does not appear to have worked in older kernels either, I’ve tested on 6.6.

I‘ve also added some logging and it seems that the driver does not write the GPPU register

[KawaiiNahida]

It might be a driver issue. I created the following patch, and it seems to work well.

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index f384c72d9..fc782bc7f 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -260,6 +260,7 @@ static int mcp_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
                arg = pinconf_to_config_argument(configs[i]);

                switch (param) {
+               case PIN_CONFIG_BIAS_DISABLE:
                case PIN_CONFIG_BIAS_PULL_UP:
                        mutex_lock(&mcp->lock);
                        ret = mcp_set_bit(mcp, MCP_GPPU, pin, arg);
@@ -589,6 +590,29 @@ static const struct irq_chip mcp23s08_irq_chip = {
        GPIOCHIP_IRQ_RESOURCE_HELPERS,
 };

+static int mcp23s08_add_pin_ranges(struct gpio_chip *gc)
+{
+       struct mcp23s08 *mcp = gpiochip_get_data(gc);
+       struct pinctrl_dev *pctldev = mcp->pctldev;
+       if (!pctldev)
+               return 0;
+
+       return gpiochip_add_pin_range(gc, pinctrl_dev_get_devname(pctldev), 0,
+                                     0, gc->ngpio);
+}
+
+static int mcp23s08_set_config(struct gpio_chip *chip, unsigned int offset,
+                              unsigned long config)
+{
+       switch (pinconf_to_config_param(config)) {
+       case PIN_CONFIG_BIAS_DISABLE:
+       case PIN_CONFIG_BIAS_PULL_UP:
+               return gpiochip_generic_config(chip, offset, config);
+       default:
+               return -ENOTSUPP;
+       }
+}
+
 /*----------------------------------------------------------------------*/

 int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
@@ -610,7 +635,9 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
        mcp->chip.get_multiple = mcp23s08_get_multiple;
        mcp->chip.direction_output = mcp23s08_direction_output;
        mcp->chip.set = mcp23s08_set;
+       mcp->chip.set_config = mcp23s08_set_config;
        mcp->chip.set_multiple = mcp23s08_set_multiple;
+       mcp->chip.add_pin_ranges = mcp23s08_add_pin_ranges;

        mcp->chip.base = base;
        mcp->chip.can_sleep = true;
@@ -675,10 +702,6 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
                girq->threaded = true;
        }

-       ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
-       if (ret < 0)
-               return dev_err_probe(dev, ret, "can't add GPIO chip\n");
-
        mcp->pinctrl_desc.pctlops = &mcp_pinctrl_ops;
        mcp->pinctrl_desc.confops = &mcp_pinconf_ops;
        mcp->pinctrl_desc.npins = mcp->chip.ngpio;
@@ -692,6 +715,10 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
        if (IS_ERR(mcp->pctldev))
                return dev_err_probe(dev, PTR_ERR(mcp->pctldev), "can't register controller\n");

+       ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
+       if (ret < 0)
+               return dev_err_probe(dev, ret, "can't add GPIO chip\n");
+
        if (mcp->irq) {
                ret = mcp23s08_irq_setup(mcp);
                if (ret)
@@ -704,3 +731,4 @@ EXPORT_SYMBOL_GPL(mcp23s08_probe_one);

 MODULE_DESCRIPTION("MCP23S08 SPI/I2C GPIO driver");
 MODULE_LICENSE("GPL");
+

[6by9]

I've reproduced the same issue on MCP23017 (the I2C version of the chip).

I'll agree that it's a driver issue. We haven't modified that driver from upstream, so it would be worth reporting upstream too.
From MAINTAINERS:

Linus Walleij <linus.walleij@linaro.org> (maintainer:PIN CONTROL SUBSYSTEM)
linux-gpio@vger.kernel.org (open list:PIN CONTROL SUBSYSTEM)

Although they'll obviously be happy with a patch fixing it too.

Your mcp23s08_set_config isn't required - you should be able to use gpiochip_generic_config as it calls through to pinctrl_gpio_set_config, pinconf_set_config, which calls the driver .pin_config_set, and that will fail if the option isn't supported.

I don't know if pinconf_set(PIN_CONFIG_BIAS_DISABLE) is guaranteed to have an argument of 0, if not then you could have a disable call result in setting the pull-up.

Note the docs for gpiochip_add_pin_range say

 * Calling this function directly from a DeviceTree-supported
 * pinctrl driver is DEPRECATED. Please see Section 2.1 of
 * Documentation/devicetree/bindings/gpio/gpio.txt on how to
 * bind pinctrl and gpio drivers via the "gpio-ranges" property.

I think that means that the overlay should be adding a gpio-ranges property, but I've not looked into the syntax that is required.

[pelwell]

For the Pi 4's combined GPIO/pinctrl driver node there is:

		gpio: gpio@7e200000 {
			compatible = "brcm,bcm2711-gpio";
			...
			gpio-ranges = <&gpio 0 0 58>;

This declares that all 58 GPIOs have corresponding pinctrl identities. Don't ask me which 0 is which. Therefore each combined gpio/pinctrl instance that is added will need gpio-ranges = <&mylabel 0 0 mycount>;, where mylabel is a DT label on the node and mycount is the number of GPIOs it provides.

[6by9]

pinctrl-bcm2835 (supporting up to bcm2711/Pi4) also has a manual gpiochip_add_pin_range call, declared deprecated by the docs.
https://github.com/raspberrypi/linux/blob/rpi-6.12.y/drivers/pinctrl/bcm/pinctrl-bcm2835.c#L384

I haven't checked whether that is called seeing as there is a gpio-ranges property defined.

[pelwell]

The name of the function from which it is called (bcm2835_add_pin_ranges_fallback) is a clue: bc96299

[6by9]

So it's not a driver bug, but the omission of the gpio-ranges from the overlay.

gpio-ranges = <&mcp23017 0 0 16> in fragment@4 fixes this for mcp23017. I'd expect a similar thing for the mcp23s17 overlay.
I'll create a PR for testing.

[6by9]

Take it back - I had a patched module on my test system :-(

[KawaiiNahida]

Maybe we still need to make some changes, including adding mcp->chip.set_config, changing the order of devm_pinctrl_register and devm_gpiochip_add_data, and adding logic to handle BIAS_DISABLE

[6by9]

Yup, it needs a combination of things. gpio-ranges removes the need for the gpiochip_add_pin_range dance, but the other bits are required.

I'll try and make a coherent patch set.

[KawaiiNahida]

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index f384c72d9..f98d6c478 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -260,6 +260,7 @@ static int mcp_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
                arg = pinconf_to_config_argument(configs[i]);

                switch (param) {
+               case PIN_CONFIG_BIAS_DISABLE:
                case PIN_CONFIG_BIAS_PULL_UP:
                        mutex_lock(&mcp->lock);
                        ret = mcp_set_bit(mcp, MCP_GPPU, pin, arg);
@@ -610,6 +611,7 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
        mcp->chip.get_multiple = mcp23s08_get_multiple;
        mcp->chip.direction_output = mcp23s08_direction_output;
        mcp->chip.set = mcp23s08_set;
+       mcp->chip.set_config = gpiochip_generic_config;
        mcp->chip.set_multiple = mcp23s08_set_multiple;

        mcp->chip.base = base;
@@ -675,10 +677,6 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
                girq->threaded = true;
        }

-       ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
-       if (ret < 0)
-               return dev_err_probe(dev, ret, "can't add GPIO chip\n");
-
        mcp->pinctrl_desc.pctlops = &mcp_pinctrl_ops;
        mcp->pinctrl_desc.confops = &mcp_pinconf_ops;
        mcp->pinctrl_desc.npins = mcp->chip.ngpio;
@@ -692,6 +690,10 @@ int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
        if (IS_ERR(mcp->pctldev))
                return dev_err_probe(dev, PTR_ERR(mcp->pctldev), "can't register controller\n");

+       ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
+       if (ret < 0)
+               return dev_err_probe(dev, ret, "can't add GPIO chip\n");
+
        if (mcp->irq) {
                ret = mcp23s08_irq_setup(mcp);
                if (ret)

[6by9]

Pretty much. See #6769.

Seeing as these should go upstream, each element needs to be a separate patch.
I've also implemented (hopefully correctly) mcp_pinconf_get(PIN_CONFIG_BIAS_DISABLE) as it seems logical that you should be able to get it as well as set it. I haven't found a mechanism to test that as yet though.

[KawaiiNahida]

Hi, I also noticed that the dtoverlay for the MCP23S17 sets the maximum SPI speed to 0.5 MHz, whereas according to the datasheet, these chips can operate at speeds up to 10 MHz.