| Message ID | 1435858426-19623-2-git-send-email-clemens.gruber@pqgruber.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Jul 2, 2015 at 7:33 PM, Clemens Gruber <clemens.gruber@pqgruber.com> wrote: > This patch adds support for the NXP PCAL9555A GPIO expander's extra > features, called Agile I/O: > Input latching, interrupt masks and open-drain output stages can be > configured via 3 optional device tree properties. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Alexandre Courbot <gnurou@gmail.com> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> (...) > +Optional properties for chips with Agile I/O: > +- nxp,input-latch: Latch input states by setting the pin's corresponding bits What does "latch" mean here. We need to know if this is something we will see a hundred times in the future or only this once. I am also very suspicious as to whether this should be in the GPIO controller, I think it should be on the consumer, because surely it is the user of the GPIO line that requires this, not the GPIO chip. > +- nxp,intr-mask: Enable interrupts by clearing the pin's corresponding mask bits This is just wrong. Let the irqchip portions of the driver enable interrupts by masking them, don't try to outsmart operating system functions with hammering down things in device tree. > +- nxp,open-drain: Enable the open-drain output stage, one bit per port (bank) Again, this should be done on the consumer side. Any driver that reference a GPIO line can do this: foo-gpios = <&gpioN 12 GPIO_ACTIVE_LOW>; So why should it not be able to do this: foo-gpios = <&gpioN 12 GPIO_OPEN_DRAIN>; Add GPIO_OPEN_DRAIN to include/dt-bindings/gpio/gpio.h and implement handling in the GPIO core drivers/gpio/gpiolib-of.c instead of trying to duct-tape it with things like this. We already have GPIO_OPEN_DRAIN in include/linux/gpio/machine.h for the boardfile case, so implement similar handling. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 16, 2015 at 01:54:55PM +0200, Linus Walleij wrote: > On Thu, Jul 2, 2015 at 7:33 PM, Clemens Gruber > <clemens.gruber@pqgruber.com> wrote: > > > This patch adds support for the NXP PCAL9555A GPIO expander's extra > > features, called Agile I/O: > > Input latching, interrupt masks and open-drain output stages can be > > configured via 3 optional device tree properties. > > > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Alexandre Courbot <gnurou@gmail.com> > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > > (...) > > > +Optional properties for chips with Agile I/O: > > +- nxp,input-latch: Latch input states by setting the pin's corresponding bits > > What does "latch" mean here. We need to know if this is something we will see > a hundred times in the future or only this once. The input latch feature ensures that input port registers keep their values until they are read, even if the connected signal changes in between. (I have a PWM high side driver which has a status signal for every output. When an error occurs the status signal goes to high and a few ms later, the chip is reset. During the reset, the status signal is goes to low and then to high again as soon as the error condition appears again) The PCAL9555A is used to remember which status register change caused the interrupt and whether an error appeared or disappeared. All chips with the "Agile I/O" feature set support input latching, so there will come more in the future. I noticed a problem with the way the pca953x driver checks which line triggered the interrupt. Just checking against the previous values does not work, because if the value changed back, we can't determine the source anymore and have an increasing number of unhandled interrupts, until the IRQ is disabled. I'd change pca953x_irq_pending for the PCAL9555A. Instead of comparing with the previous input values, I'd use the interrupt status registers in the PCAL9555A chip. There, the interrupt line which triggered the interrupt is guaranteed to stay until we read the input register of said line. So even if the input state changed from 0 to 1 and back to 0 we do not lose the interrupt. > > I am also very suspicious as to whether this should be in the GPIO controller, > I think it should be on the consumer, because surely it is the user of the > GPIO line that requires this, not the GPIO chip. OK. I'll add a GPIO_INPUT_LATCH flag for the consumer. > > > +- nxp,intr-mask: Enable interrupts by clearing the pin's corresponding mask bits > > This is just wrong. Let the irqchip portions of the driver enable interrupts by > masking them, don't try to outsmart operating system functions with > hammering down things in device tree. The problem is that this hardware interrupt mask is enabled by default on the PCAL9555A, so unless we set those interrupt mask registers to 0, interrupts are disabled (to avoid false interrupts, according to the datasheet). Should we just clear this hardware interrupt mask when the chip is probed and other than that ignore this hardware mask register / let the irqchip handle the masking by software? > > > +- nxp,open-drain: Enable the open-drain output stage, one bit per port (bank) > > Again, this should be done on the consumer side. Any driver that reference > a GPIO line can do this: > > foo-gpios = <&gpioN 12 GPIO_ACTIVE_LOW>; > > So why should it not be able to do this: > > foo-gpios = <&gpioN 12 GPIO_OPEN_DRAIN>; > > Add GPIO_OPEN_DRAIN to include/dt-bindings/gpio/gpio.h and implement > handling in the GPIO core drivers/gpio/gpiolib-of.c instead of trying to > duct-tape it with things like this. We already have GPIO_OPEN_DRAIN > in include/linux/gpio/machine.h for the boardfile case, so implement similar > handling. Here, the problem is that the open-drain output stage of the PCAL9555A can only be enabled for the whole port/bank and not for each output pin individually. > > Yours, > Linus Walleij Thanks for your help! Regards, Clemens -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 16, 2015 at 3:52 PM, Clemens Gruber <clemens.gruber@pqgruber.com> wrote: > On Thu, Jul 16, 2015 at 01:54:55PM +0200, Linus Walleij wrote: >> > +Optional properties for chips with Agile I/O: >> > +- nxp,input-latch: Latch input states by setting the pin's corresponding bits >> >> What does "latch" mean here. We need to know if this is something we will see >> a hundred times in the future or only this once. > > The input latch feature ensures that input port registers keep their values > until they are read, even if the connected signal changes in between. > (I have a PWM high side driver which has a status signal for every output. When > an error occurs the status signal goes to high and a few ms later, the chip is > reset. During the reset, the status signal is goes to low and then to high again > as soon as the error condition appears again) > The PCAL9555A is used to remember which status register change caused the > interrupt and whether an error appeared or disappeared. > > All chips with the "Agile I/O" feature set support input latching, so there will > come more in the future. > > I noticed a problem with the way the pca953x driver checks which line triggered > the interrupt. Just checking against the previous values does not work, because > if the value changed back, we can't determine the source anymore and have an > increasing number of unhandled interrupts, until the IRQ is disabled. > > I'd change pca953x_irq_pending for the PCAL9555A. Instead of comparing with > the previous input values, I'd use the interrupt status registers in the > PCAL9555A chip. There, the interrupt line which triggered the interrupt is > guaranteed to stay until we read the input register of said line. So even if the > input state changed from 0 to 1 and back to 0 we do not lose the interrupt. OK so should we not do this change and always use the latched IRQs if available on the hardware? I don't see why we would need a device tree property for this. I would more think something like, if the compatible string is so and so, our hardware supports latching, and then we just use it. That is more helpful than adding extra flags. >> I am also very suspicious as to whether this should be in the GPIO controller, >> I think it should be on the consumer, because surely it is the user of the >> GPIO line that requires this, not the GPIO chip. > > OK. I'll add a GPIO_INPUT_LATCH flag for the consumer. I don't think that's a good idea. Just use the latch without asking instead. I don't think the consumer cares how the chip achieves the latch, the provider should just do its job as well as possible. >> > +- nxp,intr-mask: Enable interrupts by clearing the pin's corresponding mask bits >> >> This is just wrong. Let the irqchip portions of the driver enable interrupts by >> masking them, don't try to outsmart operating system functions with >> hammering down things in device tree. > > The problem is that this hardware interrupt mask is enabled by default on the > PCAL9555A, so unless we set those interrupt mask registers to 0, interrupts are > disabled (to avoid false interrupts, according to the datasheet). Should we just > clear this hardware interrupt mask when the chip is probed and other than that > ignore this hardware mask register / let the irqchip handle the masking by > software? I'd say mask off all IRQs in probe and mask them on selectively as the irqchip portions request IRQs. Should work, right? >> > +- nxp,open-drain: Enable the open-drain output stage, one bit per port (bank) >> >> Again, this should be done on the consumer side. Any driver that reference >> a GPIO line can do this: >> >> foo-gpios = <&gpioN 12 GPIO_ACTIVE_LOW>; >> >> So why should it not be able to do this: >> >> foo-gpios = <&gpioN 12 GPIO_OPEN_DRAIN>; >> >> Add GPIO_OPEN_DRAIN to include/dt-bindings/gpio/gpio.h and implement >> handling in the GPIO core drivers/gpio/gpiolib-of.c instead of trying to >> duct-tape it with things like this. We already have GPIO_OPEN_DRAIN >> in include/linux/gpio/machine.h for the boardfile case, so implement similar >> handling. > > Here, the problem is that the open-drain output stage of the PCAL9555A can only > be enabled for the whole port/bank and not for each output pin individually. OK so implement it on a first-come-first-served order. Add a bool open_drain state to your state container, set it to true when the first consumer requests open drain, but noone else has yet requested any pins. So logic: request(req_open_drain): if (req_open_drain && any_pins_requested && !state->open_drain) print_verbose_error if (!any_pins_requested) state->open_drain = true; increase_requested_pins ... So the API should assume we can request this per-pin (since such controllers exist) and then the driver will impose this limitation for the PCA953x and deny consumers to configure conflicting modes. Too keep track of how many pins are currently requested you can use a bitmask or <linux/kref.h> if you want to be fancy but I think some simple solution should be OK. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 27, 2015 at 12:07:14PM +0200, Linus Walleij wrote: > OK so should we not do this change and always use the latched IRQs if > available on the hardware? I don't see why we would need a device tree > property for this. > > I would more think something like, if the compatible string is so and > so, our hardware supports latching, and then we just use it. That is more > helpful than adding extra flags. Sounds good, if somebody does not want latching, he can just specify the pca9555 compatible string. Then, no GPIO_INPUT_LATCH flag is necessary. > I'd say mask off all IRQs in probe and mask them on selectively as the > irqchip portions request IRQs. Should work, right? I think so, yes. I'll give it a try! > OK so implement it on a first-come-first-served order. > > Add a bool open_drain state to your state container, set it to true > when the first consumer requests open drain, but noone else has > yet requested any pins. So logic: > > request(req_open_drain): > if (req_open_drain && any_pins_requested && !state->open_drain) > print_verbose_error > if (!any_pins_requested) > state->open_drain = true; > increase_requested_pins > ... > > So the API should assume we can request this per-pin (since such controllers > exist) and then the driver will impose this limitation for the PCA953x > and deny consumers to configure conflicting modes. OK, will do! > > Too keep track of how many pins are currently requested you can use > a bitmask or <linux/kref.h> if you want to be fancy but I think some simple > solution should be OK. Thank you very much for your feedback. I'll come back with a RFC patch series. > > Yours, > Linus Walleij Regards, Clemens Gruber -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt index b9a42f2..5c58687 100644 --- a/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt +++ b/Documentation/devicetree/bindings/gpio/gpio-pca953x.txt @@ -11,6 +11,7 @@ Required properties: nxp,pca9539 nxp,pca9554 nxp,pca9555 + nxp,pcal9555a nxp,pca9556 nxp,pca9557 nxp,pca9574 @@ -26,8 +27,15 @@ Required properties: ti,tca6424 exar,xra1202 -Example: +Supported chips with Agile I/O features: +- nxp,pcal9555a +Optional properties for chips with Agile I/O: +- nxp,input-latch: Latch input states by setting the pin's corresponding bits +- nxp,intr-mask: Enable interrupts by clearing the pin's corresponding mask bits +- nxp,open-drain: Enable the open-drain output stage, one bit per port (bank) + +Examples: gpio@20 { compatible = "nxp,pca9505"; @@ -37,3 +45,17 @@ Example: interrupt-parent = <&gpio3>; interrupts = <23 IRQ_TYPE_LEVEL_LOW>; }; + + gpio@22 { + compatible = "nxp,pcal9555a"; + reg = <0x22>; + gpio-controller; + #gpio-cells = <2>; + interrupt-controller; + #interrupt-cells = <2>; + interrupt-parent = <&gpio2>; + interrupts = <20 IRQ_TYPE_EDGE_FALLING>; + nxp,input-latch = <0x8000>; /* Latch the input of pin I0.0 */ + nxp,intr-mask = <0xf000>; /* Mask interrupts I0.0-I0.3 */ + nxp,open-drain = <0x2>; /* Output port 1 (O1.0-O1.7) as open-drain */ + }; diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index d233eb3..95698c9 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -3,6 +3,7 @@ * * Copyright (C) 2005 Ben Gardner <bgardner@wabtec.com> * Copyright (C) 2007 Marvell International Ltd. + * Copyright (C) 2015 Clemens Gruber <clemens.gruber@pqgruber.com> * * Derived from drivers/i2c/chips/pca9539.c * @@ -26,6 +27,9 @@ #define PCA953X_OUTPUT 1 #define PCA953X_INVERT 2 #define PCA953X_DIRECTION 3 +#define PCAL95XXA_INPUT_LATCH 0x44 +#define PCAL95XXA_INTR_MASK 0x4A +#define PCAL95XXA_OUTPUT_CONFIG 0x4F #define REG_ADDR_AI 0x80 @@ -40,6 +44,7 @@ #define PCA_GPIO_MASK 0x00FF #define PCA_INT 0x0100 +#define PCA_AGILEIO 0x0200 #define PCA953X_TYPE 0x1000 #define PCA957X_TYPE 0x2000 @@ -53,6 +58,7 @@ static const struct i2c_device_id pca953x_id[] = { { "pca9539", 16 | PCA953X_TYPE | PCA_INT, }, { "pca9554", 8 | PCA953X_TYPE | PCA_INT, }, { "pca9555", 16 | PCA953X_TYPE | PCA_INT, }, + { "pcal9555a", 16 | PCA953X_TYPE | PCA_INT | PCA_AGILEIO, }, { "pca9556", 8 | PCA953X_TYPE, }, { "pca9557", 8 | PCA953X_TYPE, }, { "pca9574", 8 | PCA957X_TYPE | PCA_INT, }, @@ -614,6 +620,53 @@ out: return ret; } +static int device_pcal95xxa_agileio_setup(struct pca953x_chip *chip, + struct device_node *node) +{ + int ret; + u32 input_latch = 0; + u32 intr_mask = 0; + u32 open_drain = 0; + + /* Input latch */ + if (of_property_read_u32(node, "nxp,input-latch", &input_latch) == 0) { + if (input_latch > 0xFFFF) + return -EINVAL; + + ret = i2c_smbus_write_word_data(chip->client, + PCAL95XXA_INPUT_LATCH, + rol16(input_latch, 8)); + if (ret) + return -EIO; + } + + /* Interrupt mask */ + if (of_property_read_u32(node, "nxp,intr-mask", &intr_mask) == 0) { + if (intr_mask > 0xFFFF) + return -EINVAL; + + ret = i2c_smbus_write_word_data(chip->client, + PCAL95XXA_INTR_MASK, + rol16(intr_mask, 8)); + if (ret) + return -EIO; + } + + /* Open-drain output stage per port (bank) */ + if (of_property_read_u32(node, "nxp,open-drain", &open_drain) == 0) { + if (open_drain > 0x3) + return -EINVAL; + + ret = i2c_smbus_write_byte_data(chip->client, + PCAL95XXA_OUTPUT_CONFIG, + (u8)open_drain); + if (ret) + return -EIO; + } + + return 0; +} + static int device_pca957x_init(struct pca953x_chip *chip, u32 invert) { int ret; @@ -645,6 +698,7 @@ out: static int pca953x_probe(struct i2c_client *client, const struct i2c_device_id *id) { + struct device_node *node = client->dev.of_node; struct pca953x_platform_data *pdata; struct pca953x_chip *chip; int irq_base = 0; @@ -700,6 +754,19 @@ static int pca953x_probe(struct i2c_client *client, dev_warn(&client->dev, "setup failed, %d\n", ret); } + /* Configure Agile I/O features, if supported by the chip */ + if ((id->driver_data & PCA_AGILEIO) && IS_ENABLED(CONFIG_OF) && node) { + /* Only expanders with 16-bit supported */ + if (NBANK(chip) == 2) { + ret = device_pcal95xxa_agileio_setup(chip, node); + if (ret < 0) + dev_warn(&client->dev, + "Agile I/O setup failed, %d\n", ret); + } else + dev_warn(&client->dev, + "Agile I/O not supported on this chip\n"); + } + i2c_set_clientdata(client, chip); return 0; } @@ -735,6 +802,7 @@ static const struct of_device_id pca953x_dt_ids[] = { { .compatible = "nxp,pca9539", }, { .compatible = "nxp,pca9554", }, { .compatible = "nxp,pca9555", }, + { .compatible = "nxp,pcal9555a", }, { .compatible = "nxp,pca9556", }, { .compatible = "nxp,pca9557", }, { .compatible = "nxp,pca9574", },
This patch adds support for the NXP PCAL9555A GPIO expander's extra features, called Agile I/O: Input latching, interrupt masks and open-drain output stages can be configured via 3 optional device tree properties. Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Alexandre Courbot <gnurou@gmail.com> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> --- .../devicetree/bindings/gpio/gpio-pca953x.txt | 24 +++++++- drivers/gpio/gpio-pca953x.c | 68 ++++++++++++++++++++++ 2 files changed, 91 insertions(+), 1 deletion(-)