[Letux-kernel] Pyra display driver once again broken in v5.2-rc1

[H. Nikolaus Schaller]

Hi,

> Am 29.05.2019 um 20:04 schrieb Andreas Kemnade <andreas at kemnade.info>:
> 
> On Wed, 29 May 2019 19:51:21 +0200
> Andreas Kemnade <andreas at kemnade.info> wrote:
> 
>> On Wed, 29 May 2019 02:12:27 -0700
>> Tony Lindgren <tony at atomide.com> wrote:
>> 
>>> Hi,
>>> 
>>> * H. Nikolaus Schaller <hns at goldelico.com> [190529 08:57]:  
>>>>> [    7.524125] pca953x 4-0022: failed writing register
>>>>> [    7.529444] pca953x: probe of 4-0022 failed with error -22    
>>> 
>>> I saw something similar with a BBB cape (early Baylibre
>>> ACME cape) and it was device_pca95xx_init() somehow trying to
>>> write to a read-only register causing an error.
>>> 
>>> I did not debug further and I do not have it handy, and I
>>> could not easily figure out what caused the regression..
>>> But maybe take a look and see if commenting out the register
>>> write device_pca95xx_init() helps.
>>> 
>> 
>> 
>> root at letux:/sys/bus/i2c/drivers/pca953x# echo 4-0022 >bind 
>> [  176.917013] pca953x 4-0022: writing reg 02 -> 88 = 00
>> -> apparently inversion register  
>> 
>> [  176.922460] pca953x 4-0022: failed writing register: 02
>> [  176.929805] pca953x: probe of 4-0022 failed with error -22
>> -bash: echo: write error: No such device
>> root at letux:/sys/bus/i2c/drivers/pca953x# i2cset -y 4 0x22 0x88 0
>> root at letux:/sys/bus/i2c/drivers/pca953x# i2cset -y 4 0x22 0x2 0                                                                                                                               
>> root at letux:/sys/bus/i2c/drivers/pca953x# 
>> 
>> debug output by this patch:
>> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
>> index b7ef33f63392..161c5d5ee8c8 100644
>> --- a/drivers/gpio/gpio-pca953x.c
>> +++ b/drivers/gpio/gpio-pca953x.c
>> @@ -338,10 +338,11 @@ static int pca953x_write_regs(struct pca953x_chip *chip, int reg, u8 *val)
>> {
>>        u8 regaddr = pca953x_recalc_addr(chip, reg, 0, true, true);
>>        int ret;
>> -
>> +       
>> +       dev_info(&chip->client->dev, "writing reg %02x -> %02x = %02x", reg, regaddr, *val);
>>        ret = regmap_bulk_write(chip->regmap, regaddr, val, NBANK(chip));
>>        if (ret < 0) {
>> -               dev_err(&chip->client->dev, "failed writing register\n");
>> +               dev_err(&chip->client->dev, "failed writing register: %02x\n", reg);
>>                return ret;
>>        }
>> 
>> perhaps this rings a bell for someone.
>> 
> ok, commented out the invert access in the init:
> @@ -865,7 +866,7 @@ static int device_pca95xx_init(struct pca953x_chip *chip, u32 invert)
>        else
>                memset(val, 0, NBANK(chip));
> 
> -       ret = pca953x_write_regs(chip, chip->regs->invert, val);
> +//     ret = pca953x_write_regs(chip, chip->regs->invert, val);
> out:
>        return ret;
> }
> 
> 
> and it probes successfully. Maybe we should just ignore that error
> there.

No...

I have now been able to confirm your findings. But the problem is much deeper.
The reason is that chip->regs->invert is 0x02 but translates into 0x88 as your
first dev_info shows. This 0x88 is beyond the number of regmap registers!

Why is it? Because the 0x80 component is not a register number but an autoincrement
flag.

This means that device_pca95xx_init tries to write a register with autoincrement
enabled through regmap. IMHO this can't ever work...

Why did it not appear in v5.1 although the driver is the same?

I have checked the log of drivers/base/regmap/regmap.c and there is one which
may explain:

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

This is not in 5.1 but in 5.2-rc1. This means that the pca953x_writeable_register()
callback is now never called in device_pca95xx_init but was before and could
allow the register write...

I have tried to revert 8b9f9d4dc511 and 

Why did not anyone else discover this? This autoincrement flag is special
for 24 bit expanders and I guess there are much more 16 bit gpio
expanders in the wild than 24 bit...

Now how can it be solved? I have no idea. Expanding the regmap to 256 addresses
and passing this REG_ADDR_AI to regmap seems to be wrong anyways since there will be
two cache entries for one register.

Maybe changing reg_bits to 7 in regmap_config may help - but I don't know if
regmap simply passes the REG_ADDR_AI flag when doing physical i2c access. 

I will ask the regmap and pca953x maintainers...

BR,
Nikolaus