Re: gpio-mcp23sxx driver stopped working

[Alexandre Courbot <gnurou@xxxxxxxxx>]

On Tue, Jan 20, 2015 at 11:58 AM, Zhang, Sonic <Sonic.Zhang@xxxxxxxxxx> wrote:
> Hi Antonio,
>
>>-----Original Message-----
>>From: Antonio Fiol Bonnín [mailto:antonio@xxxxxxx]
>>Sent: Monday, January 19, 2015 6:55 PM
>>To: Alexandre Courbot
>>Cc: Zhang, Sonic; Linus Walleij; Grant Likely; Rob Herring; linux-gpio@xxxxxxxxxxxxxxx
>>Subject: Re: gpio-mcp23sxx driver stopped working
>>
>>On Mon, Jan 19, 2015 at 7:08 AM, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
>>> On Mon, Jan 19, 2015 at 1:01 PM, Zhang, Sonic <Sonic.Zhang@xxxxxxxxxx> wrote:
>>>> Hi Antonio,
>>>>
>>>>>-----Original Message-----
>>>>>From: Antonio Fiol Bonnín [mailto:antonio@xxxxxxx]
>>>>>Sent: Sunday, January 18, 2015 7:32 PM
>>>>>To: Alexandre Courbot; Zhang, Sonic
>>>>>Cc: Linus Walleij; Grant Likely; Rob Herring;
>>>>>linux-gpio@xxxxxxxxxxxxxxx
>>>>>Subject: Re: gpio-mcp23sxx driver stopped working
>>>>>
>>>>>And I have some more news...
>>>>>
>>>>>On Sun, Jan 18, 2015 at 9:34 AM, Antonio Fiol Bonnín <antonio@xxxxxxx> wrote:
>>>>>> On Sun, Jan 18, 2015 at 7:04 AM, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
>>>>>>> On Sat, Jan 17, 2015 at 3:28 AM, Antonio Fiol Bonnín
>>>>>>> <antonio@xxxxxxx>
>>>>>>> wrote:
>>>>>>> > I am writing to you as the maintainers for the gpio-mcp23sxx
>>>>>>> > driver (and open firmware maintainers) according to the kernel
>>>>>>> > documentation, before writing to the linux-kernel, linux-gpio or
>>>>>>> > devicetree mailing list, as recommeded by the FAQ.  If you
>>>>>>> > believe I should proceed otherwise, I will be pleased to do so.
>>>>>>>
>>>>>>> Thanks for reporting that issue and your detailed report. AFAICT
>>>>>>> you did everything well - you could maybe also have included the
>>>>>>> linux-gpio ML to reach a wider audience.
>>>>>
>>>>>OK, Adding the list now that I have some more info.
>>>>>
>>>>>>> > I was using that module on a raspberry pi on kernel 3.15.2,
>>>>>>> > successfully.
>>>>>>> > When I upgraded to 3.18.2, the module stopped working.
>>>>>>> > Specifically, the probes for a MCP23017 device fail with -EINVAL (-22).
>>>>>>> >
>>>>>>> > Some background:
>>>>>>> > The setup is as follows:
>>>>>>> > Some RPi GPIO pins drive leds. These continue working. Unrelated
>>>>>>> > to the failing module.
>>>>>>> > RPi GPIO port 4 is used as the master for a w1 bus holding a
>>>>>>> > couple temperature sensors. Still working properly. This is
>>>>>>> > relevant because I am upgrading after observing some kernel oops
>>>>>>> > related to the w1_therm module, but still unrelated to the failing module.
>>>>>>> >
>>>>>>> > The issue:
>>>>>>> > RPI I2C port connected to a MCP23017 which, in turn, has 16 I/O
>>>>>>> > pins configured to drive some more leds. This is still working
>>>>>>> > at the I2C level, but the gpio-mcp23sxx module fails to probe
>>>>>>> > the chip, and this means it fails to create the
>>>>>>> > /sys/bus/gpio/gpiochip240 so I can't export the pins which I
>>>>>>> > later use to change the led state, all this using sysfs.
>>>>>>> >
>>>>>>> > Some debugging:
>>>>>>> > I checked that the driver is rejecting the probe because of
>>>>>>> > "invalid platform data". I tracked it down to be "inexistent"
>>>>>>> > platform data, as pdata is null when the test is performed.
>>>>>>> > That code path is reached both when the kernel is configured
>>>>>>> > with CONFIG_OF and when it is configured without it.
>>>>>>> > Without it, I can understand, but with, I can't.
>>>>>>> > The OF matching does not succeed because dev.of_node is null too.
>>>>>>> >
>>>> Have you added the gpio node for mcp23sxx in the device tree file of your raspberry pi board? And have you rebuilt your
>>dtb file?
>>
>>No, I have not.
>>
>>> It seems like Antonio's device is using platform data (Antonio, can
>>> you confirm?), so is DT relevant at all here?
>>
>>Well, both device tree and platform data are new concepts to me. So I don't quite know...
>>
>>> By the look of it this patch might have broken platform support for
>>> this driver. According to the trace given by Antonio, the following
>>> condition in probe() is met:
>>>
>>>         if (!pdata || !gpio_is_valid(pdata->base)) {
>>>
>>> Antonio, could you display both pdata and pdata->base right before
>>> this test so we understand why this is happening?
>>
>>In the original failing version, as I explained above, pdata was NULL ("pdata is null when the test is performed")
>>
>>With the 3.15.2, when I
>>echo mcp23017 0x27 > /sys/bus/i2c/devices/i2c-1/new_device
>>it automatically created a "gpiochip240" under /sys/classes/gpio, and I could export the gpio pins 240-255.
>>
>>With 3.18.2 and the driver right before Sonic's patch (the latest working scenario), when I echo mcp23017 0x27 >
>>/sys/bus/i2c/devices/i2c-1/new_device
>>it automatically creates a "gpiochip496" under /sys/classes/gpio, and I can export the gpio pins 496-511.
>>
>>It seems like someone is assigning the "base" automatically, down from a certain maximum, that may have been increased
>>since 3.15.2.
> It seems the former mcp23017 driver create a default gpio base address and an irq number if you have neither a device node nor a platform data defined. Is it correct to skip both the device node and the platform data for a platform driver? If yes, I can send a patch to support this behavior.

If this configuration was working before it is desirable to preserve
this behavior in order to avoid regressions. And indeed, in 3.15 the
driver was doing this:

       if (match || !pdata) {
                base = -1;
                pullups = 0;
       }

which assigns the base number dynamically and allows the driver to
probe successfully if neither DT nor pdata are provided.

Could you send a patch that restores this?

Antonio, once the patch is sent, could you check it and give us your
Tested-by on it?

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html