[PATCH] drivers: gpio: pca953x: add compatibility for pcal6524 and pcal9555a

[PATCH] drivers: gpio: pca953x: add compatibility for pcal6524 and pcal9555a

[H. Nikolaus Schaller]

The Pyra-Handheld originally used the tca6424 but recently we have
replaced it by the pin and package compatible pcal6524. So let's
add this to the bindings and the driver.

And while we are at it, the pcal9555a does not have a compatible entry
either but is already supported by the device id table.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 Documentation/devicetree/bindings/gpio/gpio-pca953x.txt | 2 ++
 drivers/gpio/gpio-pca953x.c                             | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
index 0d0158728f89..d2a937682836 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
@@ -16,6 +16,8 @@ Required properties:
 	nxp,pca9574
 	nxp,pca9575
 	nxp,pca9698
+	nxp,pcal6524
+	nxp,pcal9555a
 	maxim,max7310
 	maxim,max7312
 	maxim,max7313
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index a0a5f9730aa7..d2ead4b1cf61 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -70,6 +70,7 @@ static const struct i2c_device_id pca953x_id[] = {
 	{ "pca9575", 16 | PCA957X_TYPE | PCA_INT, },
 	{ "pca9698", 40 | PCA953X_TYPE, },
 
+	{ "pcal6524", 24 | PCA953X_TYPE | PCA_INT | PCA_PCAL, },
 	{ "pcal9555a", 16 | PCA953X_TYPE | PCA_INT | PCA_PCAL, },
 
 	{ "max7310", 8  | PCA953X_TYPE, },
@@ -935,6 +936,9 @@ static const struct of_device_id pca953x_dt_ids[] = {
 	{ .compatible = "nxp,pca9575", .data = OF_957X(16, PCA_INT), },
 	{ .compatible = "nxp,pca9698", .data = OF_953X(40, 0), },
 
+	{ .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_INT), },
+	{ .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_INT), },
+
 	{ .compatible = "maxim,max7310", .data = OF_953X( 8, 0), },
 	{ .compatible = "maxim,max7312", .data = OF_953X(16, PCA_INT), },
 	{ .compatible = "maxim,max7313", .data = OF_953X(16, PCA_INT), },
-- 
2.12.2

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

[Andy Shevchenko]

On Sat, Mar 10, 2018 at 1:00 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
> The Pyra-Handheld originally used the tca6424 but recently we have
> replaced it by the pin and package compatible pcal6524. So let's
> add this to the bindings and the driver.
>
> And while we are at it, the pcal9555a does not have a compatible entry
> either but is already supported by the device id table.


> +       { "pcal6524", 24 | PCA953X_TYPE | PCA_INT | PCA_PCAL, },
>         { "pcal9555a", 16 | PCA953X_TYPE | PCA_INT | PCA_PCAL, },

So, from your description I can get that PCA_PCAL is redundant for
6524. Is it correct? What does L means in the model code?
Perhaps we need to rename PCA_PCAL to be more specific?


> +       { .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_INT), },
> +       { .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_INT), },

Other way around, you missed PCA_PCAL in the second case.

-- 
With Best Regards,
Andy Shevchenko

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

[H. Nikolaus Schaller]

Hi Andy,

> Am 13.03.2018 um 17:56 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> 
> On Sat, Mar 10, 2018 at 1:00 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> The Pyra-Handheld originally used the tca6424 but recently we have
>> replaced it by the pin and package compatible pcal6524. So let's
>> add this to the bindings and the driver.
>> 
>> And while we are at it, the pcal9555a does not have a compatible entry
>> either but is already supported by the device id table.
> 
> 
>> +       { "pcal6524", 24 | PCA953X_TYPE | PCA_INT | PCA_PCAL, },
>>        { "pcal9555a", 16 | PCA953X_TYPE | PCA_INT | PCA_PCAL, },
> 
> So, from your description I can get that PCA_PCAL is redundant for
> 6524. Is it correct?

We might not be using all features of the 6524 and therefore our test
coverage is not 100%. So I would not draw the conclusion that PCA_PCAL
is *not* needed. The data sheet should tell.

> What does L means in the model code?

Good question. The data sheets don't tell. But 6424 and 6524 are not identical
from register set and overall functions, although quite similar.
Only for pin and package.

As far as I understand the 6524 (and all PCAL) have additional interrupt
latch mechanisms and registers so that it is possible to disable each
I/O pin individually as an interrupt while for the 6424 they are always
enabled. Maybe this is the "L" designator.

But we aren't using interrupts yet.

> Perhaps we need to rename PCA_PCAL to be more specific?

My assumption is that it should be there for all PCAL variants,
but I think the original author who introduced this constant should know.

Hm. git blame tells that you have introduced it :) But this seems to
be a false alarm because you have just changed formatting.

It was introduced by

commit 44896beae605b93f2232301befccb7ef42953198
Author: Yong Li <yong.b.li@intel.com>
Date:   Thu Apr 7 12:56:32 2016 +0800

    gpio: pca953x: add PCAL9535 interrupt support for Galileo Gen2
    
    Galileo Gen2 board uses the PCAL9535 as the GPIO expansion,
    it is different from PCA9535 and includes interrupt mask/status registers,
    The current driver does not support the interrupt registers configuration,
    it causes some gpio pins cannot trigger interrupt events,
    this patch fix this issue.
    
    The original patch was submitted by
    Josef Ahmad <josef.ahmad@linux.intel.com>
    http://git.yoctoproject.org/cgit/cgit.cgi/meta-intel-quark/tree/recipes-kernel/linux/files/0015-Quark-GPIO-1-2-quark.patch
    
    Signed-off-by: Yong Li <yong.b.li@intel.com>
    Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> 
> 
>> +       { .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_INT), },
>> +       { .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_INT), },
> 
> Other way around, you missed PCA_PCAL in the second case.

Ah, ok. It wasn't clear how these flag relate to the i2c table because they
are hidden by a macro here. I'd assume that PCA_PCAL is missing for both.

So it might be that we run the pcal6524 in non-PCAL mode because we use DT.
I can do a test as soon as I have access to the hardware. 

BR and thanks,
Nikolaus

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

[Andy Shevchenko]

On Wed, Mar 14, 2018 at 2:39 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> Am 13.03.2018 um 17:56 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Sat, Mar 10, 2018 at 1:00 PM, H. Nikolaus Schaller <hns@goldelico.com> wrote:

>> What does L means in the model code?
>
> Good question. The data sheets don't tell. But 6424 and 6524 are not identical
> from register set and overall functions, although quite similar.
> Only for pin and package.
>
> As far as I understand the 6524 (and all PCAL) have additional interrupt
> latch mechanisms and registers so that it is possible to disable each
> I/O pin individually as an interrupt while for the 6424 they are always
> enabled. Maybe this is the "L" designator.

IIRC "L" stands for the "Latch" as you correctly described above.

> But we aren't using interrupts yet.

Are you planning? Or let's put it this way: does the hardware support it?

>> Perhaps we need to rename PCA_PCAL to be more specific?
>
> My assumption is that it should be there for all PCAL variants,
> but I think the original author who introduced this constant should know.

>     The original patch was submitted by
>     Josef Ahmad <josef.ahmad@linux.intel.com>
>     http://git.yoctoproject.org/cgit/cgit.cgi/meta-intel-quark/tree/recipes-kernel/linux/files/0015-Quark-GPIO-1-2-quark.patch

I'm not sure we can reach to Josef (at least easily). So, better to
rely on official datasheets.

>>> +       { .compatible = "nxp,pcal6524", .data = OF_953X(24, PCA_INT), },
>>> +       { .compatible = "nxp,pcal9555a", .data = OF_953X(16, PCA_INT), },
>>
>> Other way around, you missed PCA_PCAL in the second case.
>
> Ah, ok. It wasn't clear how these flag relate to the i2c table because they
> are hidden by a macro here. I'd assume that PCA_PCAL is missing for both.

Yep, if HW supports it.

> So it might be that we run the pcal6524 in non-PCAL mode because we use DT.
> I can do a test as soon as I have access to the hardware.

Please, do.

-- 
With Best Regards,
Andy Shevchenko

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

[Rob Herring]

On Sat, Mar 10, 2018 at 12:00:01PM +0100, H. Nikolaus Schaller wrote:
> The Pyra-Handheld originally used the tca6424 but recently we have
> replaced it by the pin and package compatible pcal6524. So let's
> add this to the bindings and the driver.
> 
> And while we are at it, the pcal9555a does not have a compatible entry
> either but is already supported by the device id table.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  Documentation/devicetree/bindings/gpio/gpio-pca953x.txt | 2 ++

Reviewed-by: Rob Herring <robh@kernel.org>

>  drivers/gpio/gpio-pca953x.c                             | 4 ++++
>  2 files changed, 6 insertions(+)

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

[Linus Walleij]

On Sat, Mar 10, 2018 at 12:00 PM, H. Nikolaus Schaller
<hns@goldelico.com> wrote:

> The Pyra-Handheld originally used the tca6424 but recently we have
> replaced it by the pin and package compatible pcal6524. So let's
> add this to the bindings and the driver.
>
> And while we are at it, the pcal9555a does not have a compatible entry
> either but is already supported by the device id table.
>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>

Patch applied with Rob's review tag.

If there are any further concerns from the discussion with Andy,
please follow up with additional patches if need be, thanks!

Yours,
Linus Walleij

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

[H. Nikolaus Schaller]

Hi Linus,

> Am 27.03.2018 um 15:02 schrieb Linus Walleij <linus.walleij@linaro.org>:
> 
> On Sat, Mar 10, 2018 at 12:00 PM, H. Nikolaus Schaller
> <hns@goldelico.com> wrote:
> 
>> The Pyra-Handheld originally used the tca6424 but recently we have
>> replaced it by the pin and package compatible pcal6524. So let's
>> add this to the bindings and the driver.
>> 
>> And while we are at it, the pcal9555a does not have a compatible entry
>> either but is already supported by the device id table.
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> 
> Patch applied with Rob's review tag.

Thanks!

> 
> If there are any further concerns from the discussion with Andy,
> please follow up with additional patches if need be, thanks!

Yes, there will be something.

But before submitting, I want to try to get interrupt handling tested.

First of all I have found the board which I can use for testing.
It has a ts3a227 as interrupt source connected to input 14 of the
pcal6524 (instead of tca6424) which has the interrupt output
connected to the gpio6_161 of an OMAP5.

To see what happens I have added some printk to

	drivers/gpio/gpio-pca953x.c
	kernel/irq/manage.c
	sound/soc/codecs/ts3a227e.c

The result is a little puzzling. I see both drivers call
request_threaded_irq() with flags = IRQF_ONESHOT | IRQF_SHARED
and | IRQF_TRIGGER_LOW for the pcal. It seems as if the flags
from DT are ignored here.

Then strange things happen. The ts3a227e.c handler is called
permanently. Even before interrupts are enabled in the chip,
although I don't see any pca953x interrupt to occur. And
so far I could neither see pca953x_irq_mask() pca953x_irq_unmask()
nor pca953x_irq_bus_sync_unlock() pca953x_irq_bus_sync_lock()
being called.

So I'd suspect that something is wrong with setting up the
chained interrupts and the thread is not sleeping. Maybe a bug
in my DT but I don't know yet.

Here is a shortened version of the relevant DT setup which may
be wrong:

> &omap5_pmx_core {
> 	pinctrl-0 = <&pyra_other_pins>;
> 
> 	pyra_other_pins: pinmux_other_gpio_pins {
> 		pinctrl-single,pins = <
> 			OMAP5_IOPAD(0x12e, PIN_INPUT_PULLUP | MUX_MODE6)	/* 0x12C:[31:16]  gpio6_161 - TCA6424 active low interrupt */
> 			>;
> 	};
> };
> 
> 
> &i2c5 {
> 	/* microphone detect */
> 	ts3a227@3b {
> 		compatible = "ti,ts3a227e";
> 		reg = <0x3b>;
> 		interrupt-parent = <&gpio99>;
> 		interrupts = <14 IRQ_TYPE_EDGE_RISING>;
> 		ti,micbias = <0>;	/* 2.1V */
> 	};
> 
> 	/* system-led and other controls */
> 	gpio99: tca6424@22 {
> 		compatible = "nxp,pcal6524";
> 		interrupt-parent = <&gpio6>;
> 		interrupts = <1 IRQ_TYPE_EDGE_FALLING>;	/* gpio6_161 */
> 		vcc-supply = <&vdds_1v8_main>;
> 
> 		reg = <0x22>;
> 		gpio-controller;
> 		#gpio-cells = <2>;
> 		gpio-line-names =
> 			"hdmi-ct-hpd", "hdmi.ls-oe", "p02", "p03", "vibra", "fault2", "p06", "p07",
> 			"en-usb", "en-host1", "en-host2", "chg-int", "p14", "p15", "mic-int", "en-modem",
> 			"shdn-hs-amp", "chg-status+red", "green", "blue", "en-esata", "fault1", "p26", "p27";
> 	};
> };

Any hints how to debug this?

Especially how pca953x_irq_bus_sync_unlock() should be called from the
initialization of the ts3a227e driver?

And what could make the ts3a227e handler being called without interrupt.

BR and thanks,
Nikolaus

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

[H. Nikolaus Schaller]

Hi,

> Am 28.03.2018 um 18:50 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Linus,
> 
>> Am 27.03.2018 um 15:02 schrieb Linus Walleij <linus.walleij@linaro.org>:
>> 
>> On Sat, Mar 10, 2018 at 12:00 PM, H. Nikolaus Schaller
>> <hns@goldelico.com> wrote:
>> 
>>> The Pyra-Handheld originally used the tca6424 but recently we have
>>> replaced it by the pin and package compatible pcal6524. So let's
>>> add this to the bindings and the driver.
>>> 
>>> And while we are at it, the pcal9555a does not have a compatible entry
>>> either but is already supported by the device id table.
>>> 
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> 
>> Patch applied with Rob's review tag.
> 
> Thanks!
> 
>> 
>> If there are any further concerns from the discussion with Andy,
>> please follow up with additional patches if need be, thanks!
> 
> Yes, there will be something.
> 
> But before submitting, I want to try to get interrupt handling tested.
> 
> First of all I have found the board which I can use for testing.
> It has a ts3a227 as interrupt source connected to input 14 of the
> pcal6524 (instead of tca6424) which has the interrupt output
> connected to the gpio6_161 of an OMAP5.
> 
> To see what happens I have added some printk to
> 
> 	drivers/gpio/gpio-pca953x.c
> 	kernel/irq/manage.c
> 	sound/soc/codecs/ts3a227e.c
> 
> The result is a little puzzling. I see both drivers call
> request_threaded_irq() with flags = IRQF_ONESHOT | IRQF_SHARED
> and | IRQF_TRIGGER_LOW for the pcal. It seems as if the flags
> from DT are ignored here.
> 
> Then strange things happen. The ts3a227e.c handler is called
> permanently. Even before interrupts are enabled in the chip,
> although I don't see any pca953x interrupt to occur. And
> so far I could neither see pca953x_irq_mask() pca953x_irq_unmask()
> nor pca953x_irq_bus_sync_unlock() pca953x_irq_bus_sync_lock()
> being called.

I have solved this issue.

I had assumed that it is sufficient to refer to the pcal6524 phandle
as interrupt-parent.

But it turns out that the chip has to be marked in the DT
as an interrupt-controller.

The result without this seems to have been that the omap-gpio irq
was delivered to the ts3a227 driver which is not able to remove the
irq at the gpio.

> 
> So I'd suspect that something is wrong with setting up the
> chained interrupts and the thread is not sleeping. Maybe a bug
> in my DT but I don't know yet.
> 
> Here is a shortened version of the relevant DT setup which may
> be wrong:
> 
>> 
>> 	/* system-led and other controls */
>> 	gpio99: tca6424@22 {
>> 		compatible = "nxp,pcal6524";
>> 		interrupt-parent = <&gpio6>;
>> 		interrupts = <1 IRQ_TYPE_EDGE_FALLING>;	/* gpio6_161 */
>> 		vcc-supply = <&vdds_1v8_main>;

		interrupt-controller;
		#interrupt-cells = <2>;

>> 
>> 		reg = <0x22>;
>> 		gpio-controller;
>> 		#gpio-cells = <2>;
>> 		gpio-line-names =
>> 			"hdmi-ct-hpd", "hdmi.ls-oe", "p02", "p03", "vibra", "fault2", "p06", "p07",
>> 			"en-usb", "en-host1", "en-host2", "chg-int", "p14", "p15", "mic-int", "en-modem",
>> 			"shdn-hs-amp", "chg-status+red", "green", "blue", "en-esata", "fault1", "p26", "p27";
>> 	};
>> };
> 
> And what could make the ts3a227e handler being called without interrupt.

Now this seems to work, the ts3a227e interrupt flooding has stopped
and pca953x_irq_bus_sync_unlock() is called as expected.

I think I'll include a patch for the pca953x bindings documentation
to better point this out, maybe with example.

Another issue I have fixed is that the pcal6524 has 4 registers per
bank while the pcal9555a has only 2. 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?

BR and thanks,
Nikolaus

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

[Andy Shevchenko]

On Thu, Mar 29, 2018 at 10:29 PM, H. Nikolaus Schaller
<hns@goldelico.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?
Please, send patches!

> 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).


-- 
With Best Regards,
Andy Shevchenko

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

[H. Nikolaus Schaller]

Hi,

> Am 30.03.2018 um 23:39 schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> 
> On Thu, Mar 29, 2018 at 10:29 PM, H. Nikolaus Schaller
> <hns@goldelico.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