* [PATCH v3 0/5] pinctrl: mcp32s08: add open drain config for irq @ 2017-11-21 8:21 Phil Reid 2017-11-21 8:21 ` [PATCH v3 2/5] dt-bindings: pinctrl: mcp23s08: add documentation for drive-open-drain Phil Reid ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Phil Reid @ 2017-11-21 8:21 UTC (permalink / raw) To: linus.walleij-QSEj5FYQhm4dnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8, sre-DgEjT+Ai2ygdnm+yROfE0A, preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO, linux-gpio-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA This is a continuation of patch 4/5/6 from my previous series "gpio: mcp23s08: add support for mcp23018" The other patches from that series having already been applied. Hopefully I've address all of Linus's concerns. Wasn't too sure on how to handle the depcrated property documentation. Changes from v2: - Use standard 'drive-open-drain' for irq output configuration - deprecate 'microchip,irq-active-high' property and extract the irq polarity from the 'interrupts' property. - 'microchip,irq-active-high' property is now ignored except for a warning if the property exists. This is probably safe as the 'interrupts' property is most likely setup correctly. Phil Reid (5): pinctrl: mcp23s08: remove hard coded irq polarity in irq_setup dt-bindings: pinctrl: mcp23s08: add documentation for drive-open-drain pinctrl: mcp23s08: add open drain configuration for irq output pinctrl: mcp23s08: configure irq polarity using irq data dt-bindings: pinctrl: deprecate 'microchip,irq-active-high' property .../bindings/pinctrl/pinctrl-mcp23s08.txt | 6 +++-- drivers/pinctrl/pinctrl-mcp23s08.c | 28 ++++++++++++---------- 2 files changed, 20 insertions(+), 14 deletions(-) -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 2/5] dt-bindings: pinctrl: mcp23s08: add documentation for drive-open-drain 2017-11-21 8:21 [PATCH v3 0/5] pinctrl: mcp32s08: add open drain config for irq Phil Reid @ 2017-11-21 8:21 ` Phil Reid 2017-11-21 12:56 ` Sebastian Reichel [not found] ` <1511252491-79952-3-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> [not found] ` <1511252491-79952-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> 2017-11-21 8:21 ` [PATCH v3 5/5] dt-bindings: pinctrl: deprecate 'microchip,irq-active-high' property Phil Reid 2 siblings, 2 replies; 21+ messages in thread From: Phil Reid @ 2017-11-21 8:21 UTC (permalink / raw) To: linus.walleij, robh+dt, mark.rutland, sre, preid, linux-gpio, devicetree This flag set the mcp23s08 device irq type to open drain active low. Signed-off-by: Phil Reid <preid@electromag.com.au> --- Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt index 9c451c2..a5a8322 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt @@ -45,6 +45,8 @@ Optional properties: - first cell is the pin number - second cell is used to specify flags. - interrupt-controller: Marks the device node as a interrupt controller. +- drive-open-drain: Sets the ODR flag in the IOCON register. This configures + the IRQ output as open drain active low. Optional device specific properties: - microchip,irq-mirror: Sets the mirror flag in the IOCON register. Devices -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 2/5] dt-bindings: pinctrl: mcp23s08: add documentation for drive-open-drain 2017-11-21 8:21 ` [PATCH v3 2/5] dt-bindings: pinctrl: mcp23s08: add documentation for drive-open-drain Phil Reid @ 2017-11-21 12:56 ` Sebastian Reichel [not found] ` <1511252491-79952-3-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> 1 sibling, 0 replies; 21+ messages in thread From: Sebastian Reichel @ 2017-11-21 12:56 UTC (permalink / raw) To: Phil Reid; +Cc: linus.walleij, robh+dt, mark.rutland, linux-gpio, devicetree [-- Attachment #1: Type: text/plain, Size: 1211 bytes --] Hi, On Tue, Nov 21, 2017 at 04:21:28PM +0800, Phil Reid wrote: > This flag set the mcp23s08 device irq type to open drain active low. > > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> -- Sebastian > Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt > index 9c451c2..a5a8322 100644 > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt > @@ -45,6 +45,8 @@ Optional properties: > - first cell is the pin number > - second cell is used to specify flags. > - interrupt-controller: Marks the device node as a interrupt controller. > +- drive-open-drain: Sets the ODR flag in the IOCON register. This configures > + the IRQ output as open drain active low. > > Optional device specific properties: > - microchip,irq-mirror: Sets the mirror flag in the IOCON register. Devices > -- > 1.8.3.1 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <1511252491-79952-3-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>]
* Re: [PATCH v3 2/5] dt-bindings: pinctrl: mcp23s08: add documentation for drive-open-drain [not found] ` <1511252491-79952-3-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> @ 2017-11-21 18:37 ` Rob Herring 0 siblings, 0 replies; 21+ messages in thread From: Rob Herring @ 2017-11-21 18:37 UTC (permalink / raw) To: Phil Reid Cc: linus.walleij-QSEj5FYQhm4dnm+yROfE0A, mark.rutland-5wv7dgnIgG8, sre-DgEjT+Ai2ygdnm+yROfE0A, linux-gpio-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA On Tue, Nov 21, 2017 at 04:21:28PM +0800, Phil Reid wrote: > This flag set the mcp23s08 device irq type to open drain active low. > > Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> > --- > Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt | 2 ++ > 1 file changed, 2 insertions(+) Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <1511252491-79952-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>]
* [PATCH v3 1/5] pinctrl: mcp23s08: remove hard coded irq polarity in irq_setup [not found] ` <1511252491-79952-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> @ 2017-11-21 8:21 ` Phil Reid 2017-11-21 13:17 ` Sebastian Reichel 2017-11-21 8:21 ` [PATCH v3 3/5] pinctrl: mcp23s08: add open drain configuration for irq output Phil Reid 2017-11-21 8:21 ` [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data Phil Reid 2 siblings, 1 reply; 21+ messages in thread From: Phil Reid @ 2017-11-21 8:21 UTC (permalink / raw) To: linus.walleij-QSEj5FYQhm4dnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8, sre-DgEjT+Ai2ygdnm+yROfE0A, preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO, linux-gpio-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA The polarity of the irq should be defined in the configuration for the irq. eg The device tree bind already allows for either active high / low interrupt configuration. Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> --- drivers/pinctrl/pinctrl-mcp23s08.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c index 0aef30e..cc1f9f6 100644 --- a/drivers/pinctrl/pinctrl-mcp23s08.c +++ b/drivers/pinctrl/pinctrl-mcp23s08.c @@ -56,7 +56,6 @@ struct mcp23s08 { u8 addr; - bool irq_active_high; bool reg_shift; u16 irq_rise; @@ -627,11 +626,6 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp) int err; unsigned long irqflags = IRQF_ONESHOT | IRQF_SHARED; - if (mcp->irq_active_high) - irqflags |= IRQF_TRIGGER_HIGH; - else - irqflags |= IRQF_TRIGGER_LOW; - err = devm_request_threaded_irq(chip->parent, mcp->irq, NULL, mcp23s08_irq, irqflags, dev_name(chip->parent), mcp); @@ -777,12 +771,12 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, { int status, ret; bool mirror = false; + bool irq_active_high = false; mutex_init(&mcp->lock); mcp->dev = dev; mcp->addr = addr; - mcp->irq_active_high = false; mcp->chip.direction_input = mcp23s08_direction_input; mcp->chip.get = mcp23s08_get; @@ -868,7 +862,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, mcp->irq_controller = device_property_read_bool(dev, "interrupt-controller"); if (mcp->irq && mcp->irq_controller) { - mcp->irq_active_high = + irq_active_high = device_property_read_bool(dev, "microchip,irq-active-high"); @@ -876,11 +870,11 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, } if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror || - mcp->irq_active_high) { + irq_active_high) { /* mcp23s17 has IOCON twice, make sure they are in sync */ status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8)); status |= IOCON_HAEN | (IOCON_HAEN << 8); - if (mcp->irq_active_high) + if (irq_active_high) status |= IOCON_INTPOL | (IOCON_INTPOL << 8); else status &= ~(IOCON_INTPOL | (IOCON_INTPOL << 8)); -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 1/5] pinctrl: mcp23s08: remove hard coded irq polarity in irq_setup 2017-11-21 8:21 ` [PATCH v3 1/5] pinctrl: mcp23s08: remove hard coded irq polarity in irq_setup Phil Reid @ 2017-11-21 13:17 ` Sebastian Reichel 0 siblings, 0 replies; 21+ messages in thread From: Sebastian Reichel @ 2017-11-21 13:17 UTC (permalink / raw) To: Phil Reid; +Cc: linus.walleij, robh+dt, mark.rutland, linux-gpio, devicetree [-- Attachment #1: Type: text/plain, Size: 2932 bytes --] Hi, On Tue, Nov 21, 2017 at 04:21:27PM +0800, Phil Reid wrote: > The polarity of the irq should be defined in the configuration > for the irq. eg The device tree bind already allows for either > active high / low interrupt configuration. > > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- I think the patch is right, but the long patch description is not. I would expect something like this: This changes the driver, so that the "microchip,irq-active-high" property only configures the mcp23017 interrupt output, but not the host interrupt input. The host interrupt should be configured using the standard interrupt flags. -- Sebastian > drivers/pinctrl/pinctrl-mcp23s08.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > index 0aef30e..cc1f9f6 100644 > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > @@ -56,7 +56,6 @@ > > struct mcp23s08 { > u8 addr; > - bool irq_active_high; > bool reg_shift; > > u16 irq_rise; > @@ -627,11 +626,6 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp) > int err; > unsigned long irqflags = IRQF_ONESHOT | IRQF_SHARED; > > - if (mcp->irq_active_high) > - irqflags |= IRQF_TRIGGER_HIGH; > - else > - irqflags |= IRQF_TRIGGER_LOW; > - > err = devm_request_threaded_irq(chip->parent, mcp->irq, NULL, > mcp23s08_irq, > irqflags, dev_name(chip->parent), mcp); > @@ -777,12 +771,12 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > { > int status, ret; > bool mirror = false; > + bool irq_active_high = false; > > mutex_init(&mcp->lock); > > mcp->dev = dev; > mcp->addr = addr; > - mcp->irq_active_high = false; > > mcp->chip.direction_input = mcp23s08_direction_input; > mcp->chip.get = mcp23s08_get; > @@ -868,7 +862,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > mcp->irq_controller = > device_property_read_bool(dev, "interrupt-controller"); > if (mcp->irq && mcp->irq_controller) { > - mcp->irq_active_high = > + irq_active_high = > device_property_read_bool(dev, > "microchip,irq-active-high"); > > @@ -876,11 +870,11 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > } > > if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror || > - mcp->irq_active_high) { > + irq_active_high) { > /* mcp23s17 has IOCON twice, make sure they are in sync */ > status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8)); > status |= IOCON_HAEN | (IOCON_HAEN << 8); > - if (mcp->irq_active_high) > + if (irq_active_high) > status |= IOCON_INTPOL | (IOCON_INTPOL << 8); > else > status &= ~(IOCON_INTPOL | (IOCON_INTPOL << 8)); > -- > 1.8.3.1 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 3/5] pinctrl: mcp23s08: add open drain configuration for irq output [not found] ` <1511252491-79952-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> 2017-11-21 8:21 ` [PATCH v3 1/5] pinctrl: mcp23s08: remove hard coded irq polarity in irq_setup Phil Reid @ 2017-11-21 8:21 ` Phil Reid 2017-11-21 12:57 ` Sebastian Reichel 2017-11-21 8:21 ` [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data Phil Reid 2 siblings, 1 reply; 21+ messages in thread From: Phil Reid @ 2017-11-21 8:21 UTC (permalink / raw) To: linus.walleij-QSEj5FYQhm4dnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8, sre-DgEjT+Ai2ygdnm+yROfE0A, preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO, linux-gpio-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA The mcp23s08 series device can be configured for wired and interrupts using an external pull-up and open drain output via the IOCON_ODR bit. And "drive-open-drain" property to enable this. Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> --- drivers/pinctrl/pinctrl-mcp23s08.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c index cc1f9f6..8ff9b77 100644 --- a/drivers/pinctrl/pinctrl-mcp23s08.c +++ b/drivers/pinctrl/pinctrl-mcp23s08.c @@ -772,6 +772,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, int status, ret; bool mirror = false; bool irq_active_high = false; + bool open_drain = false; mutex_init(&mcp->lock); @@ -867,10 +868,11 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, "microchip,irq-active-high"); mirror = device_property_read_bool(dev, "microchip,irq-mirror"); + open_drain = device_property_read_bool(dev, "drive-open-drain"); } if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror || - irq_active_high) { + irq_active_high || open_drain) { /* mcp23s17 has IOCON twice, make sure they are in sync */ status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8)); status |= IOCON_HAEN | (IOCON_HAEN << 8); @@ -882,6 +884,9 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, if (mirror) status |= IOCON_MIRROR | (IOCON_MIRROR << 8); + if (open_drain) + status |= IOCON_ODR | (IOCON_ODR << 8); + if (type == MCP_TYPE_S18 || type == MCP_TYPE_018) status |= IOCON_INTCC | (IOCON_INTCC << 8); -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 3/5] pinctrl: mcp23s08: add open drain configuration for irq output 2017-11-21 8:21 ` [PATCH v3 3/5] pinctrl: mcp23s08: add open drain configuration for irq output Phil Reid @ 2017-11-21 12:57 ` Sebastian Reichel 0 siblings, 0 replies; 21+ messages in thread From: Sebastian Reichel @ 2017-11-21 12:57 UTC (permalink / raw) To: Phil Reid; +Cc: linus.walleij, robh+dt, mark.rutland, linux-gpio, devicetree [-- Attachment #1: Type: text/plain, Size: 1969 bytes --] Hi, On Tue, Nov 21, 2017 at 04:21:29PM +0800, Phil Reid wrote: > The mcp23s08 series device can be configured for wired and interrupts > using an external pull-up and open drain output via the IOCON_ODR bit. > And "drive-open-drain" property to enable this. > > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> -- Sebastian > drivers/pinctrl/pinctrl-mcp23s08.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > index cc1f9f6..8ff9b77 100644 > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > @@ -772,6 +772,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > int status, ret; > bool mirror = false; > bool irq_active_high = false; > + bool open_drain = false; > > mutex_init(&mcp->lock); > > @@ -867,10 +868,11 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > "microchip,irq-active-high"); > > mirror = device_property_read_bool(dev, "microchip,irq-mirror"); > + open_drain = device_property_read_bool(dev, "drive-open-drain"); > } > > if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror || > - irq_active_high) { > + irq_active_high || open_drain) { > /* mcp23s17 has IOCON twice, make sure they are in sync */ > status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8)); > status |= IOCON_HAEN | (IOCON_HAEN << 8); > @@ -882,6 +884,9 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > if (mirror) > status |= IOCON_MIRROR | (IOCON_MIRROR << 8); > > + if (open_drain) > + status |= IOCON_ODR | (IOCON_ODR << 8); > + > if (type == MCP_TYPE_S18 || type == MCP_TYPE_018) > status |= IOCON_INTCC | (IOCON_INTCC << 8); > > -- > 1.8.3.1 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data [not found] ` <1511252491-79952-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> 2017-11-21 8:21 ` [PATCH v3 1/5] pinctrl: mcp23s08: remove hard coded irq polarity in irq_setup Phil Reid 2017-11-21 8:21 ` [PATCH v3 3/5] pinctrl: mcp23s08: add open drain configuration for irq output Phil Reid @ 2017-11-21 8:21 ` Phil Reid 2017-11-21 13:34 ` Sebastian Reichel 2017-11-30 14:17 ` Linus Walleij 2 siblings, 2 replies; 21+ messages in thread From: Phil Reid @ 2017-11-21 8:21 UTC (permalink / raw) To: linus.walleij-QSEj5FYQhm4dnm+yROfE0A, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8, sre-DgEjT+Ai2ygdnm+yROfE0A, preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO, linux-gpio-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA The irq polarity is already encoded in the irq config. Use that to determine the polarity for the mcp32s08 irq output instead of the custom microchip,irq-active-high property. Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> --- drivers/pinctrl/pinctrl-mcp23s08.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c index 8ff9b77..6b3f810 100644 --- a/drivers/pinctrl/pinctrl-mcp23s08.c +++ b/drivers/pinctrl/pinctrl-mcp23s08.c @@ -773,6 +773,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, bool mirror = false; bool irq_active_high = false; bool open_drain = false; + u32 irq_trig; mutex_init(&mcp->lock); @@ -863,9 +864,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, mcp->irq_controller = device_property_read_bool(dev, "interrupt-controller"); if (mcp->irq && mcp->irq_controller) { - irq_active_high = - device_property_read_bool(dev, - "microchip,irq-active-high"); + if (device_property_present(dev, "microchip,irq-active-high")) + dev_warn(dev, + "microchip,irq-active-high is deprecated\n"); + + irq_trig = irqd_get_trigger_type(irq_get_irq_data(mcp->irq)); + if (irq_trig == IRQF_TRIGGER_HIGH) + irq_active_high = true; mirror = device_property_read_bool(dev, "microchip,irq-mirror"); open_drain = device_property_read_bool(dev, "drive-open-drain"); -- 1.8.3.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data 2017-11-21 8:21 ` [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data Phil Reid @ 2017-11-21 13:34 ` Sebastian Reichel 2017-11-21 14:46 ` Phil Reid 2017-11-30 14:17 ` Linus Walleij 1 sibling, 1 reply; 21+ messages in thread From: Sebastian Reichel @ 2017-11-21 13:34 UTC (permalink / raw) To: Phil Reid; +Cc: linus.walleij, robh+dt, mark.rutland, linux-gpio, devicetree [-- Attachment #1: Type: text/plain, Size: 2320 bytes --] Hi, On Tue, Nov 21, 2017 at 04:21:30PM +0800, Phil Reid wrote: > The irq polarity is already encoded in the irq config. Use that to > determine the polarity for the mcp32s08 irq output instead of the > custom microchip,irq-active-high property. > > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- I don't like, that we use the flags for configuring the host interrupt input and the mcp23xxx interrupt output. Usually when the interrupt line has an inverter on it, board DTS files just toggle the interrupts polarity. This will not work with this patch applied. We would need to explicitly add an inverter in the interrupt line, which is completely different to how its implemented everywhere else (I know at least some Tegra devices have implicit inverters on interrupt lines). In case this is really wanted, this patch and the first patch should be merged to avoid temporarily exposing the splitted logic. -- Sebastian > drivers/pinctrl/pinctrl-mcp23s08.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > index 8ff9b77..6b3f810 100644 > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > @@ -773,6 +773,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > bool mirror = false; > bool irq_active_high = false; > bool open_drain = false; > + u32 irq_trig; > > mutex_init(&mcp->lock); > > @@ -863,9 +864,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > mcp->irq_controller = > device_property_read_bool(dev, "interrupt-controller"); > if (mcp->irq && mcp->irq_controller) { > - irq_active_high = > - device_property_read_bool(dev, > - "microchip,irq-active-high"); > + if (device_property_present(dev, "microchip,irq-active-high")) > + dev_warn(dev, > + "microchip,irq-active-high is deprecated\n"); > + > + irq_trig = irqd_get_trigger_type(irq_get_irq_data(mcp->irq)); > + if (irq_trig == IRQF_TRIGGER_HIGH) > + irq_active_high = true; > > mirror = device_property_read_bool(dev, "microchip,irq-mirror"); > open_drain = device_property_read_bool(dev, "drive-open-drain"); > -- > 1.8.3.1 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data 2017-11-21 13:34 ` Sebastian Reichel @ 2017-11-21 14:46 ` Phil Reid [not found] ` <c191a9d6-3ebb-e95b-7342-aa18598ddf2b-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> 0 siblings, 1 reply; 21+ messages in thread From: Phil Reid @ 2017-11-21 14:46 UTC (permalink / raw) To: Sebastian Reichel Cc: linus.walleij, robh+dt, mark.rutland, linux-gpio, devicetree G'day Sebastian, On 21/11/2017 21:34, Sebastian Reichel wrote: > Hi, > > On Tue, Nov 21, 2017 at 04:21:30PM +0800, Phil Reid wrote: >> The irq polarity is already encoded in the irq config. Use that to >> determine the polarity for the mcp32s08 irq output instead of the >> custom microchip,irq-active-high property. >> >> Signed-off-by: Phil Reid <preid@electromag.com.au> >> --- > > I don't like, that we use the flags for configuring the host > interrupt input and the mcp23xxx interrupt output. Usually > when the interrupt line has an inverter on it, board DTS files > just toggle the interrupts polarity. This will not work with > this patch applied. We would need to explicitly add an inverter > in the interrupt line, which is completely different to how its > implemented everywhere else (I know at least some Tegra devices > have implicit inverters on interrupt lines). > > In case this is really wanted, this patch and the first patch > should be merged to avoid temporarily exposing the splitted > logic. > Thanks for looking at the series. Yes I understand where your coming from. And that's exactly what I was trying to do in v2. I have 2 of these devices with open drain output that is feed to an inverter. So active low output from devices and irq consumer is active high input. However Linux wasn't a fan of the property and wanted it gone. He suggested we need a "inverter" device to allow for that in the device tree. I haven't got my head around how to do that thou. And if someone is relying on that implicit behaviour are we allowed to break things? Probably ok with this one as it's currently not possible due to code patch 1 removes. If we need to model the invert to get the patches accepted I look into that. I don't actually need it for my system as I can set open-drain with overrides the active-high control on this device, while have active high irq consumer. :) > >> drivers/pinctrl/pinctrl-mcp23s08.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c >> index 8ff9b77..6b3f810 100644 >> --- a/drivers/pinctrl/pinctrl-mcp23s08.c >> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c >> @@ -773,6 +773,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, >> bool mirror = false; >> bool irq_active_high = false; >> bool open_drain = false; >> + u32 irq_trig; >> >> mutex_init(&mcp->lock); >> >> @@ -863,9 +864,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, >> mcp->irq_controller = >> device_property_read_bool(dev, "interrupt-controller"); >> if (mcp->irq && mcp->irq_controller) { >> - irq_active_high = >> - device_property_read_bool(dev, >> - "microchip,irq-active-high"); >> + if (device_property_present(dev, "microchip,irq-active-high")) >> + dev_warn(dev, >> + "microchip,irq-active-high is deprecated\n"); >> + >> + irq_trig = irqd_get_trigger_type(irq_get_irq_data(mcp->irq)); >> + if (irq_trig == IRQF_TRIGGER_HIGH) >> + irq_active_high = true; >> >> mirror = device_property_read_bool(dev, "microchip,irq-mirror"); >> open_drain = device_property_read_bool(dev, "drive-open-drain"); >> -- >> 1.8.3.1 >> -- Regards Phil Reid ElectroMagnetic Imaging Technology Pty Ltd Development of Geophysical Instrumentation & Software www.electromag.com.au 3 The Avenue, Midland WA 6056, AUSTRALIA Ph: +61 8 9250 8100 Fax: +61 8 9250 7100 Email: preid@electromag.com.au ^ permalink raw reply [flat|nested] 21+ messages in thread
[parent not found: <c191a9d6-3ebb-e95b-7342-aa18598ddf2b-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>]
* Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data [not found] ` <c191a9d6-3ebb-e95b-7342-aa18598ddf2b-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> @ 2017-11-21 15:21 ` Sebastian Reichel 2017-11-21 15:38 ` Phil Reid ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Sebastian Reichel @ 2017-11-21 15:21 UTC (permalink / raw) To: Phil Reid, robh+dt-DgEjT+Ai2ygdnm+yROfE0A Cc: linus.walleij-QSEj5FYQhm4dnm+yROfE0A, mark.rutland-5wv7dgnIgG8, linux-gpio-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA [-- Attachment #1: Type: text/plain, Size: 5292 bytes --] Hi, On Tue, Nov 21, 2017 at 10:46:29PM +0800, Phil Reid wrote: > G'day Sebastian, > > On 21/11/2017 21:34, Sebastian Reichel wrote: > > Hi, > > > > On Tue, Nov 21, 2017 at 04:21:30PM +0800, Phil Reid wrote: > > > The irq polarity is already encoded in the irq config. Use that to > > > determine the polarity for the mcp32s08 irq output instead of the > > > custom microchip,irq-active-high property. > > > > > > Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> > > > --- > > > > I don't like, that we use the flags for configuring the host > > interrupt input and the mcp23xxx interrupt output. Usually > > when the interrupt line has an inverter on it, board DTS files > > just toggle the interrupts polarity. This will not work with > > this patch applied. We would need to explicitly add an inverter > > in the interrupt line, which is completely different to how its > > implemented everywhere else (I know at least some Tegra devices > > have implicit inverters on interrupt lines). > > > > In case this is really wanted, this patch and the first patch > > should be merged to avoid temporarily exposing the splitted > > logic. > > > Thanks for looking at the series. > > Yes I understand where your coming from. And that's exactly what I > was trying to do in v2. I have 2 of these devices with open drain output > that is feed to an inverter. So active low output from devices and irq > consumer is active high input. > > However Linux wasn't a fan of the property and wanted it gone. I guess s/Linux/Linus Walleij/? > He suggested we need a "inverter" device to allow for that in the > device tree. I haven't got my head around how to do that thou. Just to be on the same term, we are talking about these two variants: -------------------------------------------- gpio: host-gpio { random-properties; } inv: line-inverter { /* * configure the gpio controller input to be active low * and the inverter interrupt output to be active low */ interrupts = <&gpio ACTIVE_LOW>; }; mcp23xxx { random-properties; /* * configure the chip interrupt output to be active high * and the inverter interrupt input to be active high */ interrupts = <&inv ACTIVE_HIGH>; } -------------------------------------------- versus -------------------------------------------- gpio: host-gpio { random-properties; } mcp23xxx { random-properties; /* configure host interrupt input pin to be active low */ interrupts = <&gpio ACTIVE_LOW>; /* configure chip interrupt output pin to be active high */ microchip,irq-active-high; } -------------------------------------------- I think this is something, that Rob should comment on. Obviously at least in the mainline kernel nobody implemented the first solution (since there is no fitting interrupt-invert driver), but there are a few instances of the second variant. On the other hand the first solution describes the hardware more detailed. > And if someone is relying on that implicit behaviour are we allowed > to break things? Probably ok with this one as it's currently not possible > due to code patch 1 removes. > > If we need to model the invert to get the patches accepted I look into that. > I don't actually need it for my system as I can set open-drain with overrides > the active-high control on this device, while have active high irq consumer. > :) IMHO the explicit line-inverter is a bit over-engineered and implicit line-inverter is enough, but I'm fine with both solutions. I think the DT binding maintainers should comment on this though, since it's pretty much a core decision about interrupt specifiers. -- Sebastian > > > drivers/pinctrl/pinctrl-mcp23s08.c | 11 ++++++++--- > > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c > > > index 8ff9b77..6b3f810 100644 > > > --- a/drivers/pinctrl/pinctrl-mcp23s08.c > > > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c > > > @@ -773,6 +773,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > > > bool mirror = false; > > > bool irq_active_high = false; > > > bool open_drain = false; > > > + u32 irq_trig; > > > mutex_init(&mcp->lock); > > > @@ -863,9 +864,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, > > > mcp->irq_controller = > > > device_property_read_bool(dev, "interrupt-controller"); > > > if (mcp->irq && mcp->irq_controller) { > > > - irq_active_high = > > > - device_property_read_bool(dev, > > > - "microchip,irq-active-high"); > > > + if (device_property_present(dev, "microchip,irq-active-high")) > > > + dev_warn(dev, > > > + "microchip,irq-active-high is deprecated\n"); > > > + > > > + irq_trig = irqd_get_trigger_type(irq_get_irq_data(mcp->irq)); > > > + if (irq_trig == IRQF_TRIGGER_HIGH) > > > + irq_active_high = true; > > > mirror = device_property_read_bool(dev, "microchip,irq-mirror"); > > > open_drain = device_property_read_bool(dev, "drive-open-drain"); > > > -- > > > 1.8.3.1 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data 2017-11-21 15:21 ` Sebastian Reichel @ 2017-11-21 15:38 ` Phil Reid 2017-11-21 16:04 ` Alexander Stein 2017-11-30 14:21 ` Linus Walleij 2 siblings, 0 replies; 21+ messages in thread From: Phil Reid @ 2017-11-21 15:38 UTC (permalink / raw) To: Sebastian Reichel, robh+dt Cc: linus.walleij, mark.rutland, linux-gpio, devicetree On 21/11/2017 23:21, Sebastian Reichel wrote: > Hi, > > On Tue, Nov 21, 2017 at 10:46:29PM +0800, Phil Reid wrote: >> G'day Sebastian, >> >> On 21/11/2017 21:34, Sebastian Reichel wrote: >>> Hi, >>> >>> On Tue, Nov 21, 2017 at 04:21:30PM +0800, Phil Reid wrote: >>>> The irq polarity is already encoded in the irq config. Use that to >>>> determine the polarity for the mcp32s08 irq output instead of the >>>> custom microchip,irq-active-high property. >>>> >>>> Signed-off-by: Phil Reid <preid@electromag.com.au> >>>> --- >>> >>> I don't like, that we use the flags for configuring the host >>> interrupt input and the mcp23xxx interrupt output. Usually >>> when the interrupt line has an inverter on it, board DTS files >>> just toggle the interrupts polarity. This will not work with >>> this patch applied. We would need to explicitly add an inverter >>> in the interrupt line, which is completely different to how its >>> implemented everywhere else (I know at least some Tegra devices >>> have implicit inverters on interrupt lines). >>> >>> In case this is really wanted, this patch and the first patch >>> should be merged to avoid temporarily exposing the splitted >>> logic. >>> >> Thanks for looking at the series. >> >> Yes I understand where your coming from. And that's exactly what I >> was trying to do in v2. I have 2 of these devices with open drain output >> that is feed to an inverter. So active low output from devices and irq >> consumer is active high input. >> >> However Linux wasn't a fan of the property and wanted it gone. > > I guess s/Linux/Linus Walleij/? oops, yes. > >> He suggested we need a "inverter" device to allow for that in the >> device tree. I haven't got my head around how to do that thou. > > Just to be on the same term, we are talking about these two variants: > > -------------------------------------------- > gpio: host-gpio { > random-properties; > } > > inv: line-inverter { > /* > * configure the gpio controller input to be active low > * and the inverter interrupt output to be active low > */ > interrupts = <&gpio ACTIVE_LOW>; > }; > > mcp23xxx { > random-properties; > > /* > * configure the chip interrupt output to be active high > * and the inverter interrupt input to be active high > */ > interrupts = <&inv ACTIVE_HIGH>; > } > -------------------------------------------- > > versus > > -------------------------------------------- > gpio: host-gpio { > random-properties; > } > > mcp23xxx { > random-properties; > > /* configure host interrupt input pin to be active low */ > interrupts = <&gpio ACTIVE_LOW>; > > /* configure chip interrupt output pin to be active high */ > microchip,irq-active-high; > } > -------------------------------------------- > > I think this is something, that Rob should comment on. Obviously at > least in the mainline kernel nobody implemented the first solution > (since there is no fitting interrupt-invert driver), but there are > a few instances of the second variant. On the other hand the first > solution describes the hardware more detailed. Yes that was my understanding of the options, with option 1 being favoured. Nice summary of the options. > >> And if someone is relying on that implicit behaviour are we allowed >> to break things? Probably ok with this one as it's currently not possible >> due to code patch 1 removes. >> >> If we need to model the invert to get the patches accepted I look into that. >> I don't actually need it for my system as I can set open-drain with overrides >> the active-high control on this device, while have active high irq consumer. >> :) > > IMHO the explicit line-inverter is a bit over-engineered and > implicit line-inverter is enough, but I'm fine with both solutions. > I think the DT binding maintainers should comment on this though, > since it's pretty much a core decision about interrupt specifiers. Thanks again, I'll await further feedback on the preferred direction.> > -- Sebastian > >>>> drivers/pinctrl/pinctrl-mcp23s08.c | 11 ++++++++--- >>>> 1 file changed, 8 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c >>>> index 8ff9b77..6b3f810 100644 >>>> --- a/drivers/pinctrl/pinctrl-mcp23s08.c >>>> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c >>>> @@ -773,6 +773,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, >>>> bool mirror = false; >>>> bool irq_active_high = false; >>>> bool open_drain = false; >>>> + u32 irq_trig; >>>> mutex_init(&mcp->lock); >>>> @@ -863,9 +864,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev, >>>> mcp->irq_controller = >>>> device_property_read_bool(dev, "interrupt-controller"); >>>> if (mcp->irq && mcp->irq_controller) { >>>> - irq_active_high = >>>> - device_property_read_bool(dev, >>>> - "microchip,irq-active-high"); >>>> + if (device_property_present(dev, "microchip,irq-active-high")) >>>> + dev_warn(dev, >>>> + "microchip,irq-active-high is deprecated\n"); >>>> + >>>> + irq_trig = irqd_get_trigger_type(irq_get_irq_data(mcp->irq)); >>>> + if (irq_trig == IRQF_TRIGGER_HIGH) >>>> + irq_active_high = true; >>>> mirror = device_property_read_bool(dev, "microchip,irq-mirror"); >>>> open_drain = device_property_read_bool(dev, "drive-open-drain"); >>>> -- >>>> 1.8.3.1 -- Regards Phil Reid ElectroMagnetic Imaging Technology Pty Ltd Development of Geophysical Instrumentation & Software www.electromag.com.au 3 The Avenue, Midland WA 6056, AUSTRALIA Ph: +61 8 9250 8100 Fax: +61 8 9250 7100 Email: preid@electromag.com.au ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data 2017-11-21 15:21 ` Sebastian Reichel 2017-11-21 15:38 ` Phil Reid @ 2017-11-21 16:04 ` Alexander Stein 2017-11-21 16:30 ` Sebastian Reichel 2017-11-30 14:21 ` Linus Walleij 2 siblings, 1 reply; 21+ messages in thread From: Alexander Stein @ 2017-11-21 16:04 UTC (permalink / raw) To: Sebastian Reichel Cc: Phil Reid, robh+dt-DgEjT+Ai2ygdnm+yROfE0A, linus.walleij-QSEj5FYQhm4dnm+yROfE0A, mark.rutland-5wv7dgnIgG8, linux-gpio-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA Hi, On Tuesday, November 21, 2017, 4:21:42 PM CET Sebastian Reichel wrote: >[...] > -------------------------------------------- > gpio: host-gpio { > random-properties; > } > > inv: line-inverter { > /* > * configure the gpio controller input to be active low > * and the inverter interrupt output to be active low > */ > interrupts = <&gpio ACTIVE_LOW>; > }; > > mcp23xxx { > random-properties; > > /* > * configure the chip interrupt output to be active high > * and the inverter interrupt input to be active high > */ > interrupts = <&inv ACTIVE_HIGH>; > } > -------------------------------------------- > > versus > > -------------------------------------------- > gpio: host-gpio { > random-properties; > } > > mcp23xxx { > random-properties; > > /* configure host interrupt input pin to be active low */ > interrupts = <&gpio ACTIVE_LOW>; > > /* configure chip interrupt output pin to be active high */ > microchip,irq-active-high; > } > -------------------------------------------- > > I think this is something, that Rob should comment on. Obviously at > least in the mainline kernel nobody implemented the first solution > (since there is no fitting interrupt-invert driver), but there are > a few instances of the second variant. On the other hand the first > solution describes the hardware more detailed. > > > And if someone is relying on that implicit behaviour are we allowed > > to break things? Probably ok with this one as it's currently not possible > > due to code patch 1 removes. > > > > If we need to model the invert to get the patches accepted I look into that. > > I don't actually need it for my system as I can set open-drain with overrides > > the active-high control on this device, while have active high irq consumer. > > :) > > IMHO the explicit line-inverter is a bit over-engineered and > implicit line-inverter is enough, but I'm fine with both solutions. > I think the DT binding maintainers should comment on this though, > since it's pretty much a core decision about interrupt specifiers. Once you have a hardware, where one of several IRQ users is not attached to the inverter you need this inverter node anyway, caused by the mixed polarities. Also some IRQ controllers, like ARM GIC, only support rising edge interrupts (also level high). This might get important if there are dedicated IRQ pads connected to several users. Just my 2 cents. Alexander -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data 2017-11-21 16:04 ` Alexander Stein @ 2017-11-21 16:30 ` Sebastian Reichel 0 siblings, 0 replies; 21+ messages in thread From: Sebastian Reichel @ 2017-11-21 16:30 UTC (permalink / raw) To: Alexander Stein Cc: Phil Reid, robh+dt, linus.walleij, mark.rutland, linux-gpio, devicetree [-- Attachment #1: Type: text/plain, Size: 3694 bytes --] Hi, On Tue, Nov 21, 2017 at 05:04:51PM +0100, Alexander Stein wrote: > Hi, > > On Tuesday, November 21, 2017, 4:21:42 PM CET Sebastian Reichel wrote: > >[...] > > -------------------------------------------- > > gpio: host-gpio { > > random-properties; > > } > > > > inv: line-inverter { > > /* > > * configure the gpio controller input to be active low > > * and the inverter interrupt output to be active low > > */ > > interrupts = <&gpio ACTIVE_LOW>; > > }; > > > > mcp23xxx { > > random-properties; > > > > /* > > * configure the chip interrupt output to be active high > > * and the inverter interrupt input to be active high > > */ > > interrupts = <&inv ACTIVE_HIGH>; > > } > > -------------------------------------------- > > > > versus > > > > -------------------------------------------- > > gpio: host-gpio { > > random-properties; > > } > > > > mcp23xxx { > > random-properties; > > > > /* configure host interrupt input pin to be active low */ > > interrupts = <&gpio ACTIVE_LOW>; > > > > /* configure chip interrupt output pin to be active high */ > > microchip,irq-active-high; > > } > > -------------------------------------------- > > > > I think this is something, that Rob should comment on. Obviously at > > least in the mainline kernel nobody implemented the first solution > > (since there is no fitting interrupt-invert driver), but there are > > a few instances of the second variant. On the other hand the first > > solution describes the hardware more detailed. > > > > > And if someone is relying on that implicit behaviour are we allowed > > > to break things? Probably ok with this one as it's currently not possible > > > due to code patch 1 removes. > > > > > > If we need to model the invert to get the patches accepted I look into that. > > > I don't actually need it for my system as I can set open-drain with overrides > > > the active-high control on this device, while have active high irq consumer. > > > :) > > > > IMHO the explicit line-inverter is a bit over-engineered and > > implicit line-inverter is enough, but I'm fine with both solutions. > > I think the DT binding maintainers should comment on this though, > > since it's pretty much a core decision about interrupt specifiers. > > Once you have a hardware, where one of several IRQ users is not attached to the > inverter you need this inverter node anyway, caused by the mixed polarities. > Also some IRQ controllers, like ARM GIC, only support rising edge interrupts > (also level high). > > This might get important if there are dedicated IRQ pads connected to several > users. Both binding formats support this use-case as far as I can tell. Since you seem to understand how it looks like with the inverter node here is an example without it: irqctrl: irq-controller { random-properties; } mcp23xxx0 { random-properties; /* * configure host interrupt input pin to be active high, since * nothing else is supported by the interrupt controller */ interrupts = <&irqctrl ACTIVE_HIGH>; /* * configure chip interrupt output pin to be active low due to * line invert */ microchip,irq-active-high; } mcp23xxx1 { random-properties; /* * shared irq, see description from other chip */ interrupts = <&irqctrl ACTIVE_HIGH>; /* * configure chip interrupt output pin to be active high, since * it's routed through the line invert */ /* microchip,irq-active-high; */ } -- Sebastian [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data 2017-11-21 15:21 ` Sebastian Reichel 2017-11-21 15:38 ` Phil Reid 2017-11-21 16:04 ` Alexander Stein @ 2017-11-30 14:21 ` Linus Walleij 2017-11-30 15:50 ` Marc Zyngier 2 siblings, 1 reply; 21+ messages in thread From: Linus Walleij @ 2017-11-30 14:21 UTC (permalink / raw) To: Sebastian Reichel, Marc Zyngier Cc: Phil Reid, Rob Herring, Mark Rutland, linux-gpio-u79uwXL29TY76Z2rM5mHXA, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS On Tue, Nov 21, 2017 at 4:21 PM, Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote: > IMHO the explicit line-inverter is a bit over-engineered and > implicit line-inverter is enough, but I'm fine with both solutions. > I think the DT binding maintainers should comment on this though, > since it's pretty much a core decision about interrupt specifiers. I feel the same. I am very much back and forth on the subject. Simplicity of use vs modelling the system as it actually works. Back and forth. I honestly have just a very vague idea about this. I don't know if Marc Z as irqchip maintainer has some idea on how to model inverters on irq lines or if he's seen some solutions to it out there. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data 2017-11-30 14:21 ` Linus Walleij @ 2017-11-30 15:50 ` Marc Zyngier 2017-12-01 8:38 ` Linus Walleij 0 siblings, 1 reply; 21+ messages in thread From: Marc Zyngier @ 2017-11-30 15:50 UTC (permalink / raw) To: Linus Walleij, Sebastian Reichel Cc: Phil Reid, Rob Herring, Mark Rutland, linux-gpio, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS On 30/11/17 14:21, Linus Walleij wrote: > On Tue, Nov 21, 2017 at 4:21 PM, Sebastian Reichel <sre@kernel.org> wrote: > >> IMHO the explicit line-inverter is a bit over-engineered and >> implicit line-inverter is enough, but I'm fine with both solutions. >> I think the DT binding maintainers should comment on this though, >> since it's pretty much a core decision about interrupt specifiers. > > I feel the same. > > I am very much back and forth on the subject. > > Simplicity of use vs modelling the system as it actually works. > > Back and forth. > > I honestly have just a very vague idea about this. > > I don't know if Marc Z as irqchip maintainer has some idea > on how to model inverters on irq lines or if he's seen some > solutions to it out there. So far, I've seen two types of solutions: - One based on a stacked irqchip driver that implements the inverter on the irq_set_type method - One based on per-device vendor-specific properties in DT While the first one is clearly a big hammer, it has the advantage of not adding new stuff to the DT spec, and accurately describe the signal path (see the mediatek stuff for reference). The second one is just a hack, frankly. It just has the advantage of being trivial to implement. I'm clearly inclined to prefer the first solution. But maybe it is time to invent a "generic inverter" driver that could be reusable, just like we have a generic irqchip? Thanks, M. -- Jazz is not dead. It just smells funny... ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data 2017-11-30 15:50 ` Marc Zyngier @ 2017-12-01 8:38 ` Linus Walleij 0 siblings, 0 replies; 21+ messages in thread From: Linus Walleij @ 2017-12-01 8:38 UTC (permalink / raw) To: Marc Zyngier Cc: Sebastian Reichel, Phil Reid, Rob Herring, Mark Rutland, linux-gpio, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS On Thu, Nov 30, 2017 at 4:50 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > I'm clearly inclined to prefer the first solution. But maybe it is time > to invent a "generic inverter" driver that could be reusable, just like > we have a generic irqchip? I'm leaning towards that we should create compatible = "irqline-inverter"; irqchip and just slap that into drivers/of/irq.c for everyone. *Maybe* with a Kconfig for it if people worry about the extra bytes. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data 2017-11-21 8:21 ` [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data Phil Reid 2017-11-21 13:34 ` Sebastian Reichel @ 2017-11-30 14:17 ` Linus Walleij 1 sibling, 0 replies; 21+ messages in thread From: Linus Walleij @ 2017-11-30 14:17 UTC (permalink / raw) To: Phil Reid Cc: Rob Herring, Mark Rutland, Sebastian Reichel, linux-gpio, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS On Tue, Nov 21, 2017 at 9:21 AM, Phil Reid <preid@electromag.com.au> wrote: > The irq polarity is already encoded in the irq config. Use that to > determine the polarity for the mcp32s08 irq output instead of the > custom microchip,irq-active-high property. > > Signed-off-by: Phil Reid <preid@electromag.com.au> (...) > if (mcp->irq && mcp->irq_controller) { > - irq_active_high = > - device_property_read_bool(dev, > - "microchip,irq-active-high"); > + if (device_property_present(dev, "microchip,irq-active-high")) > + dev_warn(dev, > + "microchip,irq-active-high is deprecated\n"); > + > + irq_trig = irqd_get_trigger_type(irq_get_irq_data(mcp->irq)); > + if (irq_trig == IRQF_TRIGGER_HIGH) > + irq_active_high = true; This makes the setting from the interrupt line override the custom property. Should it not be the other way around to preserve compatibility with elder device trees? I.e. the custom property overrides the line setting? Otherwise it looks good. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 5/5] dt-bindings: pinctrl: deprecate 'microchip,irq-active-high' property 2017-11-21 8:21 [PATCH v3 0/5] pinctrl: mcp32s08: add open drain config for irq Phil Reid 2017-11-21 8:21 ` [PATCH v3 2/5] dt-bindings: pinctrl: mcp23s08: add documentation for drive-open-drain Phil Reid [not found] ` <1511252491-79952-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> @ 2017-11-21 8:21 ` Phil Reid 2017-11-21 18:37 ` Rob Herring 2 siblings, 1 reply; 21+ messages in thread From: Phil Reid @ 2017-11-21 8:21 UTC (permalink / raw) To: linus.walleij, robh+dt, mark.rutland, sre, preid, linux-gpio, devicetree The irq output polarity can be determined from the 'interrupts' configuration. So 'microchip,irq-active-high' is not required. Signed-off-by: Phil Reid <preid@electromag.com.au> --- Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt index a5a8322..72bac30 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt @@ -58,8 +58,8 @@ Optional device specific properties: occurred on. If it is not set, the interrupt are only generated for the bank they belong to. On devices with only one interrupt output this property is useless. -- microchip,irq-active-high: Sets the INTPOL flag in the IOCON register. This - configures the IRQ output polarity as active high. +- microchip,irq-active-high: (DEPRECATED) + irq polarity is determined from the 'interrupts' configuration Example I2C (with interrupt): gpiom1: gpio@20 { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3 5/5] dt-bindings: pinctrl: deprecate 'microchip,irq-active-high' property 2017-11-21 8:21 ` [PATCH v3 5/5] dt-bindings: pinctrl: deprecate 'microchip,irq-active-high' property Phil Reid @ 2017-11-21 18:37 ` Rob Herring 0 siblings, 0 replies; 21+ messages in thread From: Rob Herring @ 2017-11-21 18:37 UTC (permalink / raw) To: Phil Reid; +Cc: linus.walleij, mark.rutland, sre, linux-gpio, devicetree On Tue, Nov 21, 2017 at 04:21:31PM +0800, Phil Reid wrote: > The irq output polarity can be determined from the 'interrupts' > configuration. So 'microchip,irq-active-high' is not required. > > Signed-off-by: Phil Reid <preid@electromag.com.au> > --- > Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-12-01 8:38 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-21 8:21 [PATCH v3 0/5] pinctrl: mcp32s08: add open drain config for irq Phil Reid 2017-11-21 8:21 ` [PATCH v3 2/5] dt-bindings: pinctrl: mcp23s08: add documentation for drive-open-drain Phil Reid 2017-11-21 12:56 ` Sebastian Reichel [not found] ` <1511252491-79952-3-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> 2017-11-21 18:37 ` Rob Herring [not found] ` <1511252491-79952-1-git-send-email-preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> 2017-11-21 8:21 ` [PATCH v3 1/5] pinctrl: mcp23s08: remove hard coded irq polarity in irq_setup Phil Reid 2017-11-21 13:17 ` Sebastian Reichel 2017-11-21 8:21 ` [PATCH v3 3/5] pinctrl: mcp23s08: add open drain configuration for irq output Phil Reid 2017-11-21 12:57 ` Sebastian Reichel 2017-11-21 8:21 ` [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data Phil Reid 2017-11-21 13:34 ` Sebastian Reichel 2017-11-21 14:46 ` Phil Reid [not found] ` <c191a9d6-3ebb-e95b-7342-aa18598ddf2b-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> 2017-11-21 15:21 ` Sebastian Reichel 2017-11-21 15:38 ` Phil Reid 2017-11-21 16:04 ` Alexander Stein 2017-11-21 16:30 ` Sebastian Reichel 2017-11-30 14:21 ` Linus Walleij 2017-11-30 15:50 ` Marc Zyngier 2017-12-01 8:38 ` Linus Walleij 2017-11-30 14:17 ` Linus Walleij 2017-11-21 8:21 ` [PATCH v3 5/5] dt-bindings: pinctrl: deprecate 'microchip,irq-active-high' property Phil Reid 2017-11-21 18:37 ` Rob Herring
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.