[PATCH v2 0/7] pinctrl: mcp32s08: add support for mcp23018

[PATCH v2 0/7] pinctrl: mcp32s08: add support for mcp23018

[Phil Reid]

Changes from v1:
- rebase as driver moved from gpio to pinctrl
  applied on top of Lars Poeschel <poeschel@lemonage.de> patch to move 
  mcp documentation from gpio to pinctrl.
- Allow manual selection of PINCTRL in KConfig
- Add open-drain config for irq
- remove hardcode irq polarity when register irq
- remove used variables

Phil Reid (7):
  pinctrl: change Kconfig PINCTRL variable to a menuconfig
  dt-bindings: pinctrl: add mcp23018 to mcp23s08 documentation
  gpio: mcp23s08: add support for mcp23018
  dt-bindings: pinctrl: mcp23s08: add documentation for irq-open-drain
  pinctrl: mcp23s08: add open drain configuration for irq pinctrl
  pinctrl: mcp23s08: remove hardcoded irq polarity in irq_setup
  pinctrl: mcp23s08: remove unused variables from pinconf_set

 .../bindings/pinctrl/pinctrl-mcp23s08.txt          |  3 +++
 drivers/pinctrl/Kconfig                            |  9 +++----
 drivers/pinctrl/pinctrl-mcp23s08.c                 | 31 +++++++++++++++-------
 3 files changed, 28 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[PATCH v2 4/7] dt-bindings: pinctrl: mcp23s08: add documentation for irq-open-drain

[Phil Reid]

This flag set the mcp23s08 device series 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 8a5bf99..7a0808f 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
@@ -58,6 +58,8 @@ Optional device specific properties:
         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-open-drain: Sets the ODR flag in the IOCON register. This
+        configures the IRQ output as open drain active low.
 
 Example I2C (with interrupt):
 gpiom1: gpio@20 {
-- 
1.8.3.1

Re: [PATCH v2 4/7] dt-bindings: pinctrl: mcp23s08: add documentation for irq-open-drain

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1242 bytes --]

Hi,

On Fri, Oct 06, 2017 at 01:08:08PM +0800, Phil Reid wrote:
> This flag set the mcp23s08 device series 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 8a5bf99..7a0808f 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> @@ -58,6 +58,8 @@ Optional device specific properties:
>          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-open-drain: Sets the ODR flag in the IOCON register. This
> +        configures the IRQ output as open drain active low.
>  
>  Example I2C (with interrupt):
>  gpiom1: gpio@20 {
> -- 
> 1.8.3.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/7] dt-bindings: pinctrl: mcp23s08: add documentation for irq-open-drain

[Linus Walleij]

So I applied patches to this point.

On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:

> This flag set the mcp23s08 device series irq type to open drain active low.
>
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---
(...)
>  - microchip,irq-active-high: Sets the INTPOL flag in the IOCON register. This
>          configures the IRQ output polarity as active high.
> +- microchip,irq-open-drain: Sets the ODR flag in the IOCON register. This
> +        configures the IRQ output as open drain active low.

I have cold feet on these two.

I do not think either of these flags should be there or even be
used.

It is a bit tricky wrt device tree because it is Linuxisms, but I hope
the DT maintainers know what is applicable also for other
operating systems.

So the MCP23xxx outputs an IRQ. Someone, somewhere, is using
this IRQ.

If that *user* needs to have the IRQ fireing active high or set as
open drain (because there are more clients on the line) then
it needs this set up.

Look at this code from drivers/iio/gyro/mpu3050-core.c:

        /* Check if IRQ is open drain */
        if (of_property_read_bool(mpu3050->dev->of_node, "drive-open-drain"))
                mpu3050->irq_opendrain = true;

        irq_trig = irqd_get_trigger_type(irq_get_irq_data(irq));
        /*
         * Configure the interrupt generator hardware to supply whatever
         * the interrupt is configured for, edges low/high level low/high,
         * we can provide it all.
         */
        switch (irq_trig) {
        case IRQF_TRIGGER_RISING:
                dev_info(&indio_dev->dev,
                         "pulse interrupts on the rising edge\n");
                break;
        case IRQF_TRIGGER_FALLING:
                mpu3050->irq_actl = true;
                dev_info(&indio_dev->dev,
                         "pulse interrupts on the falling edge\n");
                break;
        case IRQF_TRIGGER_HIGH:
                mpu3050->irq_latch = true;
                dev_info(&indio_dev->dev,
                         "interrupts active high level\n");
                /*
                 * With level IRQs, we mask the IRQ until it is processed,
                 * but with edge IRQs (pulses) we can queue several interrupts
                 * in the top half.
                 */
                irq_trig |= IRQF_ONESHOT;
                break;
        case IRQF_TRIGGER_LOW:
                mpu3050->irq_latch = true;
                mpu3050->irq_actl = true;
                irq_trig |= IRQF_ONESHOT;
                dev_info(&indio_dev->dev,
                         "interrupts active low level\n");
                break;
        default:
                /* This is the most preferred mode, if possible */
                dev_err(&indio_dev->dev,
                        "unsupported IRQ trigger specified (%lx), enforce "
                        "rising edge\n", irq_trig);
                irq_trig = IRQF_TRIGGER_RISING;
                break;
        }

        /* An open drain line can be shared with several devices */
        if (mpu3050->irq_opendrain)
                irq_trig |= IRQF_SHARED;

        ret = request_threaded_irq(irq,
                                   mpu3050_irq_handler,
                                   mpu3050_irq_thread,
                                   irq_trig,
                                   mpu3050->trig->name,
                                   mpu3050->trig);
        if (ret) {
                dev_err(mpu3050->dev,
                        "can't get IRQ %d, error %d\n", irq, ret);
                return ret;
        }

Notice that the IIO gyro here is also a *producer* of IRQs sending
then to some *consumer* such as a GPIO pin or dedicated IRQ
in on a SoC.

Lessons learned:

1) Use the standard binding "drive-open-drain" for this, not any
  mcp,* custom namespaced things.

2) WRT active high: please deprecate that flag, and ignore it
  completely in the driver or at least print a warning. Instead look
  up the trigger type from the irq descriptor just like the MPU3050
  IIO driver does.

The information is already inside the kernel, from the consumer
side. The reciever states what kind of interrupt it wants.

It is fair to add a flag for open drain (with the standard binding), as the
IRQ subsystem has no idea about such things, but with that comes the
inevitable consequence of requesting a shared IRQ.

I will check if I can anyways apply some more patches not related
to this.

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

[PATCH v2 1/7] pinctrl: change Kconfig PINCTRL variable to a menuconfig

[Phil Reid]

This allows PINCTRL to be selected manually to allow enabling of the
mcp23s08 i2c/spi gpio driver. Which is not platform specific.

Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 drivers/pinctrl/Kconfig | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 1778cf4..8da29e9 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -2,11 +2,10 @@
 # PINCTRL infrastructure and drivers
 #
 
-config PINCTRL
-	bool
+menuconfig PINCTRL
+	bool "Pin controllers"
 
-menu "Pin controllers"
-	depends on PINCTRL
+if PINCTRL
 
 config GENERIC_PINCTRL_GROUPS
 	bool
@@ -379,4 +378,4 @@ config PINCTRL_TB10X
 	depends on OF && ARC_PLAT_TB10X
 	select GPIOLIB
 
-endmenu
+endif
-- 
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

Re: [PATCH v2 1/7] pinctrl: change Kconfig PINCTRL variable to a menuconfig

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]

Hi,

On Fri, Oct 06, 2017 at 01:08:05PM +0800, Phil Reid wrote:
> This allows PINCTRL to be selected manually to allow enabling of the
> mcp23s08 i2c/spi gpio driver. Which is not platform specific.

FWIW with i2c/spi based pin controllers available it makes sense
to have this available everywhere. I actually did test my changes
using an i2c-tiny-usb controller with my x86 based notebook, so:

Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>

-- Sebastian

> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---
>  drivers/pinctrl/Kconfig | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 1778cf4..8da29e9 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -2,11 +2,10 @@
>  # PINCTRL infrastructure and drivers
>  #
>  
> -config PINCTRL
> -	bool
> +menuconfig PINCTRL
> +	bool "Pin controllers"
>  
> -menu "Pin controllers"
> -	depends on PINCTRL
> +if PINCTRL
>  
>  config GENERIC_PINCTRL_GROUPS
>  	bool
> @@ -379,4 +378,4 @@ config PINCTRL_TB10X
>  	depends on OF && ARC_PLAT_TB10X
>  	select GPIOLIB
>  
> -endmenu
> +endif
> -- 
> 1.8.3.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/7] pinctrl: change Kconfig PINCTRL variable to a menuconfig

[Linus Walleij]

On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid@electromag.com.au> wrote:

> This allows PINCTRL to be selected manually to allow enabling of the
> mcp23s08 i2c/spi gpio driver. Which is not platform specific.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>

Patch applied with Sebastian's review tag.

Yours,
Linus Walleij

[PATCH v2 2/7] dt-bindings: pinctrl: add mcp23018 to mcp23s08 documentation

[Phil Reid]

This adds the compatible string for the mcp23018, which is the i2c variant
of the mcp23s18.

Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
index c934106..8a5bf99 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
@@ -13,6 +13,7 @@ Required properties:
     - "microchip,mcp23s18" for 16 GPIO SPI version
     - "microchip,mcp23008" for  8 GPIO I2C version or
     - "microchip,mcp23017" for 16 GPIO I2C version of the chip
+    - "microchip,mcp23018" for 16 GPIO I2C version
     NOTE: Do not use the old mcp prefix any more. It is deprecated and will be
     removed.
 - #gpio-cells : Should be two.
-- 
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

Re: [PATCH v2 2/7] dt-bindings: pinctrl: add mcp23018 to mcp23s08 documentation

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

Hi,

On Fri, Oct 06, 2017 at 01:08:06PM +0800, Phil Reid wrote:
> This adds the compatible string for the mcp23018, which is the i2c variant
> of the mcp23s18.
> 
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>

-- Sebastian

>  Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> index c934106..8a5bf99 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-mcp23s08.txt
> @@ -13,6 +13,7 @@ Required properties:
>      - "microchip,mcp23s18" for 16 GPIO SPI version
>      - "microchip,mcp23008" for  8 GPIO I2C version or
>      - "microchip,mcp23017" for 16 GPIO I2C version of the chip
> +    - "microchip,mcp23018" for 16 GPIO I2C version
>      NOTE: Do not use the old mcp prefix any more. It is deprecated and will be
>      removed.
>  - #gpio-cells : Should be two.
> -- 
> 1.8.3.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/7] dt-bindings: pinctrl: add mcp23018 to mcp23s08 documentation

[Linus Walleij]

On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:

> This adds the compatible string for the mcp23018, which is the i2c variant
> of the mcp23s18.
>
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>

Patch applied with Sebastian's tag.

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

[PATCH v2 3/7] gpio: mcp23s08: add support for mcp23018

[Phil Reid]

This adds the required definitions for the mcp23018 which is the i2c
variant of the mcp23s18.

Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 3e40d42..8d356df 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -25,6 +25,7 @@
 #define MCP_TYPE_008	2
 #define MCP_TYPE_017	3
 #define MCP_TYPE_S18    4
+#define MCP_TYPE_018    5
 
 #define MCP_MAX_DEV_PER_CS	8
 
@@ -837,6 +838,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 		mcp->chip.ngpio = 16;
 		mcp->chip.label = "mcp23017";
 		break;
+
+	case MCP_TYPE_018:
+		mcp->regmap = devm_regmap_init_i2c(data, &mcp23x17_regmap);
+		mcp->reg_shift = 1;
+		mcp->chip.ngpio = 16;
+		mcp->chip.label = "mcp23018";
+		break;
 #endif /* CONFIG_I2C */
 
 	default:
@@ -883,7 +891,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 		if (mirror)
 			status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
 
-		if (type == MCP_TYPE_S18)
+		if (type == MCP_TYPE_S18 || type == MCP_TYPE_018)
 			status |= IOCON_INTCC | (IOCON_INTCC << 8);
 
 		ret = mcp_write(mcp, MCP_IOCON, status);
@@ -964,6 +972,10 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 		.compatible = "microchip,mcp23017",
 		.data = (void *) MCP_TYPE_017,
 	},
+	{
+		.compatible = "microchip,mcp23018",
+		.data = (void *) MCP_TYPE_018,
+	},
 /* NOTE: The use of the mcp prefix is deprecated and will be removed. */
 	{
 		.compatible = "mcp,mcp23008",
@@ -1013,6 +1025,7 @@ static int mcp230xx_probe(struct i2c_client *client,
 static const struct i2c_device_id mcp230xx_id[] = {
 	{ "mcp23008", MCP_TYPE_008 },
 	{ "mcp23017", MCP_TYPE_017 },
+	{ "mcp23018", MCP_TYPE_018 },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, mcp230xx_id);
-- 
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

Re: [PATCH v2 3/7] gpio: mcp23s08: add support for mcp23018

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]

Hi,

On Fri, Oct 06, 2017 at 01:08:07PM +0800, Phil Reid wrote:
> This adds the required definitions for the mcp23018 which is the i2c
> variant of the mcp23s18.
> 
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>

-- Sebastian

>  drivers/pinctrl/pinctrl-mcp23s08.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> index 3e40d42..8d356df 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -25,6 +25,7 @@
>  #define MCP_TYPE_008	2
>  #define MCP_TYPE_017	3
>  #define MCP_TYPE_S18    4
> +#define MCP_TYPE_018    5
>  
>  #define MCP_MAX_DEV_PER_CS	8
>  
> @@ -837,6 +838,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>  		mcp->chip.ngpio = 16;
>  		mcp->chip.label = "mcp23017";
>  		break;
> +
> +	case MCP_TYPE_018:
> +		mcp->regmap = devm_regmap_init_i2c(data, &mcp23x17_regmap);
> +		mcp->reg_shift = 1;
> +		mcp->chip.ngpio = 16;
> +		mcp->chip.label = "mcp23018";
> +		break;
>  #endif /* CONFIG_I2C */
>  
>  	default:
> @@ -883,7 +891,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>  		if (mirror)
>  			status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
>  
> -		if (type == MCP_TYPE_S18)
> +		if (type == MCP_TYPE_S18 || type == MCP_TYPE_018)
>  			status |= IOCON_INTCC | (IOCON_INTCC << 8);
>  
>  		ret = mcp_write(mcp, MCP_IOCON, status);
> @@ -964,6 +972,10 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>  		.compatible = "microchip,mcp23017",
>  		.data = (void *) MCP_TYPE_017,
>  	},
> +	{
> +		.compatible = "microchip,mcp23018",
> +		.data = (void *) MCP_TYPE_018,
> +	},
>  /* NOTE: The use of the mcp prefix is deprecated and will be removed. */
>  	{
>  		.compatible = "mcp,mcp23008",
> @@ -1013,6 +1025,7 @@ static int mcp230xx_probe(struct i2c_client *client,
>  static const struct i2c_device_id mcp230xx_id[] = {
>  	{ "mcp23008", MCP_TYPE_008 },
>  	{ "mcp23017", MCP_TYPE_017 },
> +	{ "mcp23018", MCP_TYPE_018 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(i2c, mcp230xx_id);
> -- 
> 1.8.3.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/7] gpio: mcp23s08: add support for mcp23018

[Phil Reid]

G'day Linus,

did this one in the series get applied?
or did you have a problem with it and want it rebased with the other 3 patches needing work.

haven't seen any of them on devel branch git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git


On 9/10/2017 05:18, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Oct 06, 2017 at 01:08:07PM +0800, Phil Reid wrote:
>> This adds the required definitions for the mcp23018 which is the i2c
>> variant of the mcp23s18.
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
> 
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> 
> -- Sebastian
> 
>>   drivers/pinctrl/pinctrl-mcp23s08.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
>> index 3e40d42..8d356df 100644
>> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
>> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
>> @@ -25,6 +25,7 @@
>>   #define MCP_TYPE_008	2
>>   #define MCP_TYPE_017	3
>>   #define MCP_TYPE_S18    4
>> +#define MCP_TYPE_018    5
>>   
>>   #define MCP_MAX_DEV_PER_CS	8
>>   
>> @@ -837,6 +838,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>>   		mcp->chip.ngpio = 16;
>>   		mcp->chip.label = "mcp23017";
>>   		break;
>> +
>> +	case MCP_TYPE_018:
>> +		mcp->regmap = devm_regmap_init_i2c(data, &mcp23x17_regmap);
>> +		mcp->reg_shift = 1;
>> +		mcp->chip.ngpio = 16;
>> +		mcp->chip.label = "mcp23018";
>> +		break;
>>   #endif /* CONFIG_I2C */
>>   
>>   	default:
>> @@ -883,7 +891,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>>   		if (mirror)
>>   			status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
>>   
>> -		if (type == MCP_TYPE_S18)
>> +		if (type == MCP_TYPE_S18 || type == MCP_TYPE_018)
>>   			status |= IOCON_INTCC | (IOCON_INTCC << 8);
>>   
>>   		ret = mcp_write(mcp, MCP_IOCON, status);
>> @@ -964,6 +972,10 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>>   		.compatible = "microchip,mcp23017",
>>   		.data = (void *) MCP_TYPE_017,
>>   	},
>> +	{
>> +		.compatible = "microchip,mcp23018",
>> +		.data = (void *) MCP_TYPE_018,
>> +	},
>>   /* NOTE: The use of the mcp prefix is deprecated and will be removed. */
>>   	{
>>   		.compatible = "mcp,mcp23008",
>> @@ -1013,6 +1025,7 @@ static int mcp230xx_probe(struct i2c_client *client,
>>   static const struct i2c_device_id mcp230xx_id[] = {
>>   	{ "mcp23008", MCP_TYPE_008 },
>>   	{ "mcp23017", MCP_TYPE_017 },
>> +	{ "mcp23018", MCP_TYPE_018 },
>>   	{ },
>>   };
>>   MODULE_DEVICE_TABLE(i2c, mcp230xx_id);
>> -- 
>> 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

Re: [PATCH v2 3/7] gpio: mcp23s08: add support for mcp23018

[Linus Walleij]

On Thu, Oct 19, 2017 at 9:00 AM, Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:

> did this one in the series get applied?
> or did you have a problem with it and want it rebased with the other 3
> patches needing work.

No I just missed this patch, sorry :(

I have applied it now.

> haven't seen any of them on devel branch
> git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git

This is on linux-pinctrl.git as it is a pin control driver now.

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

[PATCH v2 5/7] pinctrl: mcp23s08: add open drain configuration for irq pinctrl

[Phil Reid]

The mcp23s08 series device can be configured for wired and interupts
using an external pullup and open drain output via the IOCON_ODR bit.
And "microchip,irq-open-drain" property to enable this.

Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 8d356df..150f216 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -780,6 +780,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 {
 	int status, ret;
 	bool mirror = false;
+	bool open_drain = false;
 
 	mutex_init(&mcp->lock);
 
@@ -876,6 +877,8 @@ 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,
+						    "microchip,irq-open-drain");
 	}
 
 	if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror ||
@@ -891,6 +894,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

Re: [PATCH v2 5/7] pinctrl: mcp23s08: add open drain configuration for irq pinctrl

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1712 bytes --]

Hi,

On Fri, Oct 06, 2017 at 01:08:09PM +0800, Phil Reid wrote:
> The mcp23s08 series device can be configured for wired and interupts
> using an external pullup and open drain output via the IOCON_ODR bit.
> And "microchip,irq-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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> index 8d356df..150f216 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -780,6 +780,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>  {
>  	int status, ret;
>  	bool mirror = false;
> +	bool open_drain = false;
>  
>  	mutex_init(&mcp->lock);
>  
> @@ -876,6 +877,8 @@ 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,
> +						    "microchip,irq-open-drain");
>  	}
>  
>  	if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror ||
> @@ -891,6 +894,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 --]

Re: [PATCH v2 5/7] pinctrl: mcp23s08: add open drain configuration for irq pinctrl

[Linus Walleij]

On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:

> The mcp23s08 series device can be configured for wired and interupts
> using an external pullup and open drain output via the IOCON_ODR bit.
> And "microchip,irq-open-drain" property to enable this.
>
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>

See my comment on patch 4/7 for directions on what to do with this
patch.

Use standard binding, look at irq descriptor props, IRQF_SHARED.

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

[PATCH v2 6/7] pinctrl: mcp23s08: remove hardcoded irq polarity in irq_setup

[Phil Reid]

The irq_active_high flag is for controlling the polarity of the output
from the mcp23s08 series devices. The polarity of the irq could be altered
by additional logic (eg inverters) between the device and irq input device.
The device-tree already allows for this as the irq can be specified in the
binding. So hardcoding it in the driver is overly restrictive.

There are no inkernel users of the mcp23s08 driver with irq's.

Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 150f216..8dceaa1 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -630,11 +630,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);
-- 
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

Re: [PATCH v2 6/7] pinctrl: mcp23s08: remove hardcoded irq polarity in irq_setup

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]

Hi,

On Fri, Oct 06, 2017 at 01:08:10PM +0800, Phil Reid wrote:
> The irq_active_high flag is for controlling the polarity of the output
> from the mcp23s08 series devices. The polarity of the irq could be altered
> by additional logic (eg inverters) between the device and irq input device.
> The device-tree already allows for this as the irq can be specified in the
> binding. So hardcoding it in the driver is overly restrictive.
> 
> There are no inkernel users of the mcp23s08 driver with irq's.
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

> ---
>  drivers/pinctrl/pinctrl-mcp23s08.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> index 150f216..8dceaa1 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -630,11 +630,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);
> -- 
> 1.8.3.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 6/7] pinctrl: mcp23s08: remove hardcoded irq polarity in irq_setup

[Linus Walleij]

On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:

> The irq_active_high flag is for controlling the polarity of the output
> from the mcp23s08 series devices. The polarity of the irq could be altered
> by additional logic (eg inverters) between the device and irq input device.
> The device-tree already allows for this as the irq can be specified in the
> binding. So hardcoding it in the driver is overly restrictive.
>
> There are no inkernel users of the mcp23s08 driver with irq's.
>
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>

Look this up from the irq descriptor as discussed in 4/7.

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

Re: [PATCH v2 6/7] pinctrl: mcp23s08: remove hardcoded irq polarity in irq_setup

[Linus Walleij]

On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:

> The polarity of the irq could be altered
> by additional logic (eg inverters) between the device and irq input device.

Marc Zyngier has pointed out that inverters should be modeled
using hierarchichal irq domains. You would need to add a new
irqchip to invert the line.

See this PDF:
https://elinux.org/images/8/8c/Zyngier.pdf

drivers/irqchips/irq-uniphier-aidet.c supports inversion of IRQs
for example. For this it uses irq_domain_create_hierarchy()

I guess it would be helpful with a reusable generic "inverter
irq chip", that you can just enable and slam into your device
tree to tell the system that a line is statically inverted, i.e.
non-programmable logic on the board.

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

Re: [PATCH v2 6/7] pinctrl: mcp23s08: remove hardcoded irq polarity in irq_setup

[Phil Reid]

On 12/10/2017 17:04, Linus Walleij wrote:
> On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:
> 
>> The polarity of the irq could be altered
>> by additional logic (eg inverters) between the device and irq input device.
> 
> Marc Zyngier has pointed out that inverters should be modeled
> using hierarchichal irq domains. You would need to add a new
> irqchip to invert the line.
> 
> See this PDF:
> https://elinux.org/images/8/8c/Zyngier.pdf
> 
> drivers/irqchips/irq-uniphier-aidet.c supports inversion of IRQs
> for example. For this it uses irq_domain_create_hierarchy()
> 
> I guess it would be helpful with a reusable generic "inverter
> irq chip", that you can just enable and slam into your device
> tree to tell the system that a line is statically inverted, i.e.
> non-programmable logic on the board.
> 
G'day Linus.
Thanks for the pointers, I'll see what I can come up with.


-- 
Regards
Phil Reid
--
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

[PATCH v2 7/7] pinctrl: mcp23s08: remove unused variables from pinconf_set

[Phil Reid]

Variable mask and val are not used in the mcp_pinconf_set().

Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 8dceaa1..8e461cc 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -279,8 +279,7 @@ static int mcp_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 {
 	struct mcp23s08 *mcp = pinctrl_dev_get_drvdata(pctldev);
 	enum pin_config_param param;
-	u32 arg, mask;
-	u16 val;
+	u32 arg;
 	int ret = 0;
 	int i;
 
@@ -290,8 +289,6 @@ static int mcp_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
 
 		switch (param) {
 		case PIN_CONFIG_BIAS_PULL_UP:
-			val = arg ? 0xFFFF : 0x0000;
-			mask = BIT(pin);
 			ret = mcp_set_bit(mcp, MCP_GPPU, pin, arg);
 			break;
 		default:
-- 
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

Re: [PATCH v2 7/7] pinctrl: mcp23s08: remove unused variables from pinconf_set

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

Hi,

On Fri, Oct 06, 2017 at 01:08:11PM +0800, Phil Reid wrote:
> Variable mask and val are not used in the mcp_pinconf_set().
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>

Thanks, those are leftovers from before I added mcp_set_bit.

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

> ---
>  drivers/pinctrl/pinctrl-mcp23s08.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> index 8dceaa1..8e461cc 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -279,8 +279,7 @@ static int mcp_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
>  {
>  	struct mcp23s08 *mcp = pinctrl_dev_get_drvdata(pctldev);
>  	enum pin_config_param param;
> -	u32 arg, mask;
> -	u16 val;
> +	u32 arg;
>  	int ret = 0;
>  	int i;
>  
> @@ -290,8 +289,6 @@ static int mcp_pinconf_set(struct pinctrl_dev *pctldev, unsigned int pin,
>  
>  		switch (param) {
>  		case PIN_CONFIG_BIAS_PULL_UP:
> -			val = arg ? 0xFFFF : 0x0000;
> -			mask = BIT(pin);
>  			ret = mcp_set_bit(mcp, MCP_GPPU, pin, arg);
>  			break;
>  		default:
> -- 
> 1.8.3.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 7/7] pinctrl: mcp23s08: remove unused variables from pinconf_set

[Linus Walleij]

On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:

> Variable mask and val are not used in the mcp_pinconf_set().
>
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>

Patch applied with Sebastian's ACK.

Please rebase the remaining patches on my devel branch
and focus on getting open drain and active high/low things
right.

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