BUG: gpio: pca953x: 24 bit expanders broken since v5.2-rc1

BUG: gpio: pca953x: 24 bit expanders broken since v5.2-rc1

[H. Nikolaus Schaller]

We discovered that the ti,tca6424 and the nxp,pcal6524 used on the
omap5uevm resp. pyra-handheld are broken starting with
v5.2-rc1.

The symptom is:

> [    7.524125] pca953x 4-0022: failed writing register
> [    7.529444] pca953x: probe of 4-0022 failed with error -22    

Tony also noticed this to happen with some BeagleBone Black cape.

Some analysis shows that it happens in device_pca95xx_init()
when trying to write the invert register (PCA953X_INVERT).

The PCA953X_INVERT 0x02 is translated into 0x88. Because
since

	b32cecb46bdc8 gpio: pca953x: Extract the register address mangling to single function

a 24 bit expander always gets the autoincrement flag REG_ADDR_AI
(0x80) set. I don't know if that happened before.

Now, this was not a (visible) problem until patch

	8b9f9d4dc511 regmap: verify if register is writeable before writing operations

enforces to check the register number before invoking the
callback pca953x_writeable_register(). pca953x_writeable_register()
seems to know about REG_ADDR_AI (through reg & REG_ADDR_MASK) and
accepts 0x88 as a valid register number.

After the regmap patch the register is checked against
pca953x_i2c_regmap.max_register before applying REG_ADDR_MASK
and 0x88 is obviously beyond, explaining the symptom.

A test shows that reverting 8b9f9d4dc511 makes the driver work again.

But IMHO this is the wrong fix, because I think the combined use of
regmap and REG_ADDR_AI in the pca953x driver seems to be fundamentally
broken and not the new regmap address test. Unless we expect that regmap can
pass the REG_ADDR_AI address bit to i2c but ignores it during cache
lookup. Then, I think the pca953x_i2c_regmap.reg_bits should also be
7 and not 8.

So the real solution should IMHO be to eliminate using the autoincrement
mode of these chips. But this may break the PCA9575 which seems
to need AI for multi-byte writes according to some comment.

BR and thanks,
Nikolaus

Re: BUG: gpio: pca953x: 24 bit expanders broken since v5.2-rc1

[Linus Walleij]

On Fri, May 31, 2019 at 7:06 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:

> Now, this was not a (visible) problem until patch
>
>         8b9f9d4dc511 regmap: verify if register is writeable before writing operations
>
> enforces to check the register number before invoking the
> callback pca953x_writeable_register(). pca953x_writeable_register()
> seems to know about REG_ADDR_AI (through reg & REG_ADDR_MASK) and
> accepts 0x88 as a valid register number.
>
> After the regmap patch the register is checked against
> pca953x_i2c_regmap.max_register before applying REG_ADDR_MASK
> and 0x88 is obviously beyond, explaining the symptom.

Can we simply bump the .max_register in
pca953x_i2c_regmap to 0xff for a quick fix with a comment
FIXME to figure it out the right way?

Yours,
Linus Walleij

Re: BUG: gpio: pca953x: 24 bit expanders broken since v5.2-rc1

[Linus Walleij]

On Fri, May 31, 2019 at 7:06 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:

> Now, this was not a (visible) problem until patch
>
>         8b9f9d4dc511 regmap: verify if register is writeable before writing operations
>
> enforces to check the register number before invoking the
> callback pca953x_writeable_register(). pca953x_writeable_register()
> seems to know about REG_ADDR_AI (through reg & REG_ADDR_MASK) and
> accepts 0x88 as a valid register number.
>
> After the regmap patch the register is checked against
> pca953x_i2c_regmap.max_register before applying REG_ADDR_MASK
> and 0x88 is obviously beyond, explaining the symptom.

Can we simply bump the .max_register in
pca953x_i2c_regmap to 0xff for a quick fix with a comment
FIXME to figure it out the right way?

Yours,
Linus Walleij

Re: BUG: gpio: pca953x: 24 bit expanders broken since v5.2-rc1

[H. Nikolaus Schaller]

> Am 01.06.2019 um 23:57 schrieb Linus Walleij <linus.walleij@linaro.org>:
> 
> On Fri, May 31, 2019 at 7:06 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> 
>> Now, this was not a (visible) problem until patch
>> 
>>        8b9f9d4dc511 regmap: verify if register is writeable before writing operations
>> 
>> enforces to check the register number before invoking the
>> callback pca953x_writeable_register(). pca953x_writeable_register()
>> seems to know about REG_ADDR_AI (through reg & REG_ADDR_MASK) and
>> accepts 0x88 as a valid register number.
>> 
>> After the regmap patch the register is checked against
>> pca953x_i2c_regmap.max_register before applying REG_ADDR_MASK
>> and 0x88 is obviously beyond, explaining the symptom.
> 
> Can we simply bump the .max_register in
> pca953x_i2c_regmap to 0xff for a quick fix with a comment
> FIXME to figure it out the right way?

Yes, that might be a quick workaround closer to the correct code location
in the driver.

There seem not to be many regmap accesses with randomly toggled REG_ADDR_AI
bit and therefore risk of seeing two different cache entries where they would
assume the same is low (and not higher than before we fix anything).

I'll give it a try asap.

BR,
Nikolaus

Re: BUG: gpio: pca953x: 24 bit expanders broken since v5.2-rc1

[H. Nikolaus Schaller]

> Am 01.06.2019 um 23:57 schrieb Linus Walleij <linus.walleij@linaro.org>:
> 
> On Fri, May 31, 2019 at 7:06 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:
> 
>> Now, this was not a (visible) problem until patch
>> 
>>        8b9f9d4dc511 regmap: verify if register is writeable before writing operations
>> 
>> enforces to check the register number before invoking the
>> callback pca953x_writeable_register(). pca953x_writeable_register()
>> seems to know about REG_ADDR_AI (through reg & REG_ADDR_MASK) and
>> accepts 0x88 as a valid register number.
>> 
>> After the regmap patch the register is checked against
>> pca953x_i2c_regmap.max_register before applying REG_ADDR_MASK
>> and 0x88 is obviously beyond, explaining the symptom.
> 
> Can we simply bump the .max_register in
> pca953x_i2c_regmap to 0xff for a quick fix with a comment
> FIXME to figure it out the right way?

Yes, that might be a quick workaround closer to the correct code location
in the driver.

There seem not to be many regmap accesses with randomly toggled REG_ADDR_AI
bit and therefore risk of seeing two different cache entries where they would
assume the same is low (and not higher than before we fix anything).

I'll give it a try asap.

BR,
Nikolaus