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), },
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.
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-kern...
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
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.
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(+)
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
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
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....
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
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....
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).
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....
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
-
Andy Shevchenko -
H. Nikolaus Schaller -
Linus Walleij -
Rob Herring