linux-kernel - Re: [PATCH] drivers: gpio: pca953x: add compatibility for pcal6524 and pcal9555a

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 4 Apr 2018 10:32:03 +0200
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Tony Lindgren <tony@...mide.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexandre Courbot <gnurou@...il.com>,
        Pawel Moll <pawel.moll@....com>,
        Ian Campbell <ijc+devicetree@...lion.org.uk>,
        kernel@...a-handheld.com,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Rob Herring <robh+dt@...nel.org>,
        Kumar Gala <galak@...eaurora.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>
Subject: Re: [PATCH] drivers: gpio: pca953x: add compatibility for pcal6524 and pcal9555a

Hi,

> Am 30.03.2018 um 23:39 schrieb Andy Shevchenko <andy.shevchenko@...il.com>:
> 
> On Thu, Mar 29, 2018 at 10:29 PM, H. Nikolaus Schaller
> <hns@...delico.com> wrote:
> 
>> Another issue I have fixed is that the pcal6524 has 4 registers per
>> bank while the pcal9555a has only 2.
> 
> So, you mean it's still in non-working case in Linus' tree?

Only the pcal6524 has this problem (pcal9555a should be fine).
IMHO, it does not matter since nobody is currently using it by in-tree DTS.

> Please, send patches!

Will come for review today.

> 
>> This is encoded in the address
>> constants for the new "L"-registers and has to be taken care of in
>> pca953x_read_regs_24() and pca953x_write_regs_24().
>> 
>> But I still have another problem:
>> 
>> [    4.808823] pca953x 4-0022: irq 186: unsupported type 8
>> [    4.814303] genirq: Setting trigger mode 8 for irq 186 failed (pca953x_irq_set_type+0x0/0xa8)
>> 
>> This comes from https://elixir.bootlin.com/linux/v4.16-rc7/source/sound/soc/codecs/ts3a227e.c#L314
>> 
>> It appears that the pca953x driver/interrupt-controller can't handle
>> IRQF_TRIGGER_LOW, but that is hard coded into the ts3a227 driver.
>> 
>> Anyone with knowledge and help about this issue?
> 
> This is easy to fix (not read datasheet for better solution though):
> emulate it via FALLING (LOW) and RISING (HIGH).

I have tested it by some ugly but simple hack (will include the patch),
but we have to consider:

* the request of LEVEL_LOW vs. EDGE_FALLING is defined in the driver of the attached device
  i.e. changing it there might break a lot of systems where the interrupt-controller can handle
  LEVEL_LOW properly
* I could not find any code that allows to overwrite it by DT (by the second parameter of interrupts = <number type>)
* EDGE_FALLING as a replacement of LEVEL_LOW may loose interrupts if multiple interrupts are
  present and the connected chip does not provide a pulse for each one
* I am not sure if the tca/pca hardware can really handle LEVEL interrupts - so it might need some
  emulation (repeat the irq while the input is still active after the handler did run)

Anyways, the pcal6524 basically works in interrupt mode and code can always be improved.

There is another thing I came across: the pcal6524 has a built-in pull-up/down option
which seems to be enabled by default, which might make trouble if there are external
pull-downs in the circuits. This is different to the pin-compatible tca6424.

So I am tempted to turn off this pull-up/down by probe code to stay compatible.
The right thing would be to define some pinctrl but that is beyond my capabilities
and capacity for this chip.

BR and thanks,
Nikolaus