Re: [PATCH] gpio: mcp23s08: Add irq functionality for i2c chips

[Mark Rutland <mark.rutland@arm.com>]

On Tue, Jan 07, 2014 at 02:05:48PM +0000, Lars Poeschel wrote:
> From: Lars Poeschel <poeschel@lemonage.de>
> 
> This adds interrupt functionality for i2c chips to the driver.
> They can act as a interrupt-controller and generate interrupts, if
> the inputs change.
> This is tested on a mcp23017 chip.
> 
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> ---
>  .../devicetree/bindings/gpio/gpio-mcp23s08.txt     |  21 +-
>  drivers/gpio/Kconfig                               |   1 +
>  drivers/gpio/gpio-mcp23s08.c                       | 220 ++++++++++++++++++++-
>  3 files changed, 236 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> index daa3017..b8376f3 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio-mcp23s08.txt
> @@ -38,12 +38,31 @@ Required device specific properties (only for SPI chips):
>      removed.
>  - spi-max-frequency = The maximum frequency this chip is able to handle
>  
> -Example I2C:
> +Optional properties:
> +- #interrupt-cells : Should be two.
> +  - first cell is the pin number
> +  - second cell is used to specify flags.
> +- interrupt-controller: Marks the device node as a interrupt controller.
> +NOTE: The interrupt functionality is only supported for i2c versions of the
> +chips yet.
> +
> +Optional device specific properties:
> +- microchip,irq-mirror: Sets the mirror flag in the IOCON register. This causes
> +        devices with two interrupt outputs to always trigger both interrupt
> +        outputs regardless where the interrupt happened.
> +

When is this useful?

When should it be set and when shouldin't it be?

[...]

> @@ -432,13 +618,21 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>  	/* verify MCP_IOCON.SEQOP = 0, so sequential reads work,
>  	 * and MCP_IOCON.HAEN = 1, so we work with all chips.
>  	 */
> +
>  	status = mcp->ops->read(mcp, MCP_IOCON);
>  	if (status < 0)
>  		goto fail;
> -	if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN)) {
> +
> +	mirror = of_find_property(mcp->chip.of_node,
> +				  "microchip,irq-mirror", NULL);

Use of_property_read_bool.

> +	if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror) {
>  		/* mcp23s17 has IOCON twice, make sure they are in sync */
>  		status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8));
>  		status |= IOCON_HAEN | (IOCON_HAEN << 8);
> +		status &= ~(IOCON_INTPOL | (IOCON_INTPOL << 8));
> +		if (mirror)
> +			status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
> +
>  		status = mcp->ops->write(mcp, MCP_IOCON, status);
>  		if (status < 0)
>  			goto fail;
> @@ -469,7 +663,17 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>  			goto fail;
>  	}
>  
> +	if (mcp->irq &&
> +	    of_find_property(mcp->chip.of_node, "interrupt-controller", NULL)) {

You could do so here too.

In fact, doesn't the irq-mirror proeprty only make sense if this is an
interrupt controller?

[...]

> @@ -583,6 +789,10 @@ static int mcp230xx_remove(struct i2c_client *client)
>  	if (status == 0)
>  		kfree(mcp);
>  
> +	if (client->irq &&
> +	    of_find_property(mcp->chip.of_node, "interrupt-controller", NULL))
> +		mcp23s08_irq_teardown(mcp);

Can you not cache the fact it's an interrupt controller?

Parsing the DT in a teardown path is very odd.

Thanks,
Mark.