[PATCH v3 0/5] i2c: mux: pca954x: Add interrupt controller support

[PATCH v3 0/5] i2c: mux: pca954x: Add interrupt controller support

[Phil Reid]

Various muxes can aggregate multiple interrupts from each i2c bus.
All of the muxes with interrupt support combine the active low irq lines
using an internal 'and' function and generate a combined active low
output. The muxes do provide the ability to read a control register to
determine which irq is active. By making the mux an irq controller isr
latenct can potentially be reduced by reading the status register and 
then only calling the registered isr on that bus segment.

In addition an additional enable mask is added to work around devices
that assert irq immediately before being setup buy disabling the irq
from the mux until all devices are registered.

Changes from v2:
- p1: Added Acked-by
- p5: fixup 2 typos

Changes from v1:
- Update for new ACPI table
- Fix typo in documentation
- Fix typo in function names
- Fix typo in irq name
- Added spaces around '+' / '='
- Change goto label names
- Change property name from i2c-mux-irq-mask-en to nxp,irq-mask-enable
- Change variable name irq_mask_en to irq_mask_enable
- Add commentt about irq_mask_enable
- Added Acked-By's


Phil Reid (5):
  i2c: mux: pca954x: Add missing pca9542 definition to chip_desc
  dt: bindings: i2c-mux-pca954x: Add documentation for interrupt
    controller
  i2c: mux: pca954x: Add interrupt controller support
  dt: bindings: i2c-mux-pca954x: Add documentation for
    i2c-mux-irq-mask-en
  i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |  17 ++-
 drivers/i2c/muxes/i2c-mux-pca954x.c                | 156 ++++++++++++++++++++-
 2 files changed, 168 insertions(+), 5 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

[PATCH v3 1/5] i2c: mux: pca954x: Add missing pca9542 definition to chip_desc

[Phil Reid]

The spec for the pca954x was missing. This chip is the same as the pca9540
except that it has interrupt lines. While the i2c_device_id table mapped
the pca9542 to the pca9540 definition the compatible table did not. In
preparation for irq support add the pca9542 definition.

Acked-by: Peter Rosin <peda@axentia.se>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index dd18b9c..bbf088e 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -84,6 +84,11 @@ struct pca954x {
 		.enable = 0x4,
 		.muxtype = pca954x_ismux,
 	},
+	[pca_9542] = {
+		.nchans = 2,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+	},
 	[pca_9543] = {
 		.nchans = 2,
 		.muxtype = pca954x_isswi,
@@ -110,7 +115,7 @@ struct pca954x {
 
 static const struct i2c_device_id pca954x_id[] = {
 	{ "pca9540", pca_9540 },
-	{ "pca9542", pca_9540 },
+	{ "pca9542", pca_9542 },
 	{ "pca9543", pca_9543 },
 	{ "pca9544", pca_9544 },
 	{ "pca9545", pca_9545 },
@@ -124,7 +129,7 @@ struct pca954x {
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id pca954x_acpi_ids[] = {
 	{ .id = "PCA9540", .driver_data = pca_9540 },
-	{ .id = "PCA9542", .driver_data = pca_9540 },
+	{ .id = "PCA9542", .driver_data = pca_9542 },
 	{ .id = "PCA9543", .driver_data = pca_9543 },
 	{ .id = "PCA9544", .driver_data = pca_9544 },
 	{ .id = "PCA9545", .driver_data = pca_9545 },
-- 
1.8.3.1

[PATCH v3 2/5] dt: bindings: i2c-mux-pca954x: Add documentation for interrupt controller

[Phil Reid]

Various muxes can aggregate multiple irq lines and provide a control
register to determine the active line. Add bindings for interrupt
controller support.

Acked-by: Peter Rosin <peda@axentia.se>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index cf53d5f..aa09704 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -19,7 +19,14 @@ Optional Properties:
   - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
     children in idle state. This is necessary for example, if there are several
     multiplexers on the bus and the devices behind them use same I2C addresses.
-
+  - interrupt-parent: Phandle for the interrupt controller that services
+    interrupts for this device.
+  - interrupts: Interrupt mapping for IRQ.
+  - interrupt-controller: Marks the device node as an interrupt controller.
+  - #interrupt-cells : Should be two.
+    - first cell is the pin number
+    - second cell is used to specify flags.
+    See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 
 Example:
 
@@ -29,6 +36,11 @@ Example:
 		#size-cells = <0>;
 		reg = <0x74>;
 
+		interrupt-parent = <&ipic>;
+		interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+
 		i2c@2 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
1.8.3.1

[PATCH v3 3/5] i2c: mux: pca954x: Add interrupt controller support

[Phil Reid]

Various muxes can aggregate multiple interrupts from each i2c bus.
All of the muxes with interrupt support combine the active low irq lines
using an internal 'and' function and generate a combined active low
output. The muxes do provide the ability to read a control register to
determine which irq is active. By making the mux an irq controller isr
latency can potentially be reduced by reading the status register and
then only calling the registered isr on that bus segment.

As there is no irq masking on the mux irq are disabled until irq_unmask is
called at least once.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 127 +++++++++++++++++++++++++++++++++++-
 1 file changed, 125 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index bbf088e..84fc767 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -41,14 +41,19 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/i2c/pca954x.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
 
 #define PCA954X_MAX_NCHANS 8
 
+#define PCA954X_IRQ_OFFSET 4
+
 enum pca_type {
 	pca_9540,
 	pca_9542,
@@ -63,6 +68,7 @@ enum pca_type {
 struct chip_desc {
 	u8 nchans;
 	u8 enable;	/* used for muxes only */
+	u8 has_irq;
 	enum muxtype {
 		pca954x_ismux = 0,
 		pca954x_isswi
@@ -75,6 +81,9 @@ struct pca954x {
 	u8 last_chan;		/* last register value */
 	u8 deselect;
 	struct i2c_client *client;
+
+	struct irq_domain *irq;
+	unsigned int irq_mask;
 };
 
 /* Provide specs for the PCA954x types we know about */
@@ -87,19 +96,23 @@ struct pca954x {
 	[pca_9542] = {
 		.nchans = 2,
 		.enable = 0x4,
+		.has_irq = 1,
 		.muxtype = pca954x_ismux,
 	},
 	[pca_9543] = {
 		.nchans = 2,
+		.has_irq = 1,
 		.muxtype = pca954x_isswi,
 	},
 	[pca_9544] = {
 		.nchans = 4,
 		.enable = 0x4,
+		.has_irq = 1,
 		.muxtype = pca954x_ismux,
 	},
 	[pca_9545] = {
 		.nchans = 4,
+		.has_irq = 1,
 		.muxtype = pca954x_isswi,
 	},
 	[pca_9547] = {
@@ -222,6 +235,102 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 	return pca954x_reg_write(muxc->parent, client, data->last_chan);
 }
 
+static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
+{
+	struct pca954x *data = dev_id;
+	unsigned int child_irq;
+	int ret, i, handled;
+
+	ret = i2c_smbus_read_byte(data->client);
+	if (ret < 0)
+		return IRQ_NONE;
+
+	for (i = 0; i < data->chip->nchans; i++) {
+		if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
+			child_irq = irq_linear_revmap(data->irq, i);
+			handle_nested_irq(child_irq);
+			handled++;
+		}
+	}
+	return handled ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static void pca954x_irq_mask(struct irq_data *idata)
+{
+	struct pca954x *data = irq_data_get_irq_chip_data(idata);
+	unsigned int pos = idata->hwirq;
+
+	data->irq_mask &= ~BIT(pos);
+	if (!data->irq_mask)
+		disable_irq(data->client->irq);
+}
+
+static void pca954x_irq_unmask(struct irq_data *idata)
+{
+	struct pca954x *data = irq_data_get_irq_chip_data(idata);
+	unsigned int pos = idata->hwirq;
+
+	if (!data->irq_mask)
+		enable_irq(data->client->irq);
+	data->irq_mask |= BIT(pos);
+}
+
+static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
+{
+	if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
+		return -EINVAL;
+	return 0;
+}
+
+static struct irq_chip pca954x_irq_chip = {
+	.name = "i2c-mux-pca954x",
+	.irq_mask = pca954x_irq_mask,
+	.irq_unmask = pca954x_irq_unmask,
+	.irq_set_type = pca954x_irq_set_type,
+};
+
+static int pca954x_irq_setup(struct i2c_mux_core *muxc)
+{
+	struct pca954x *data = i2c_mux_priv(muxc);
+	struct i2c_client *client = data->client;
+	int c, err, irq;
+
+	if (!data->chip->has_irq || client->irq <= 0)
+		return 0;
+
+	data->irq = irq_domain_add_linear(client->dev.of_node,
+					  data->chip->nchans,
+					  &irq_domain_simple_ops, data);
+	if (!data->irq)
+		return -ENODEV;
+
+	for (c = 0; c < data->chip->nchans; c++) {
+		irq = irq_create_mapping(data->irq, c);
+		irq_set_chip_data(irq, data);
+		irq_set_chip_and_handler(irq, &pca954x_irq_chip,
+			handle_simple_irq);
+	}
+
+	err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
+					pca954x_irq_handler,
+					IRQF_ONESHOT | IRQF_SHARED,
+					"pca954x", data);
+	if (err)
+		goto err_req_irq;
+
+	disable_irq(data->client->irq);
+
+	return 0;
+err_req_irq:
+	for (c = 0; c < data->chip->nchans; c++) {
+		irq = irq_find_mapping(data->irq, c);
+		irq_dispose_mapping(irq);
+	}
+	irq_domain_remove(data->irq);
+
+	return err;
+}
+
 /*
  * I2C init/probing/exit functions
  */
@@ -286,6 +395,10 @@ static int pca954x_probe(struct i2c_client *client,
 	idle_disconnect_dt = of_node &&
 		of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
 
+	ret = pca954x_irq_setup(muxc);
+	if (ret)
+		goto fail_del_adapters;
+
 	/* Now create an adapter for each channel */
 	for (num = 0; num < data->chip->nchans; num++) {
 		bool idle_disconnect_pd = false;
@@ -311,7 +424,7 @@ static int pca954x_probe(struct i2c_client *client,
 			dev_err(&client->dev,
 				"failed to register multiplexed adapter"
 				" %d as bus %d\n", num, force);
-			goto virt_reg_failed;
+			goto fail_del_adapters;
 		}
 	}
 
@@ -322,7 +435,7 @@ static int pca954x_probe(struct i2c_client *client,
 
 	return 0;
 
-virt_reg_failed:
+fail_del_adapters:
 	i2c_mux_del_adapters(muxc);
 	return ret;
 }
@@ -330,6 +443,16 @@ static int pca954x_probe(struct i2c_client *client,
 static int pca954x_remove(struct i2c_client *client)
 {
 	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+	struct pca954x *data = i2c_mux_priv(muxc);
+	int c, irq;
+
+	if (data->irq) {
+		for (c = 0; c < data->chip->nchans; c++) {
+			irq = irq_find_mapping(data->irq, c);
+			irq_dispose_mapping(irq);
+		}
+		irq_domain_remove(data->irq);
+	}
 
 	i2c_mux_del_adapters(muxc);
 	return 0;
-- 
1.8.3.1

Re: [PATCH v3 3/5] i2c: mux: pca954x: Add interrupt controller support

[Wolfram Sang]

On Mon, Jan 09, 2017 at 05:02:54PM +0800, Phil Reid wrote:
> Various muxes can aggregate multiple interrupts from each i2c bus.
> All of the muxes with interrupt support combine the active low irq lines
> using an internal 'and' function and generate a combined active low
> output. The muxes do provide the ability to read a control register to
> determine which irq is active. By making the mux an irq controller isr
> latency can potentially be reduced by reading the status register and
> then only calling the registered isr on that bus segment.
> 
> As there is no irq masking on the mux irq are disabled until irq_unmask is
> called at least once.
> 
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>

Is the ack from Peda here forgotten or still missing?

@peda: Once you are happy, do you want to take these patches via your
shiny new mux-tree or do you prefer if I pick them?

Regards,

   Wolfram

--
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 v3 3/5] i2c: mux: pca954x: Add interrupt controller support

[Phil Reid]

On 13/01/2017 03:15, Wolfram Sang wrote:
> On Mon, Jan 09, 2017 at 05:02:54PM +0800, Phil Reid wrote:
>> Various muxes can aggregate multiple interrupts from each i2c bus.
>> All of the muxes with interrupt support combine the active low irq lines
>> using an internal 'and' function and generate a combined active low
>> output. The muxes do provide the ability to read a control register to
>> determine which irq is active. By making the mux an irq controller isr
>> latency can potentially be reduced by reading the status register and
>> then only calling the registered isr on that bus segment.
>>
>> As there is no irq masking on the mux irq are disabled until irq_unmask is
>> called at least once.
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>
> Is the ack from Peda here forgotten or still missing?
>
> @peda: Once you are happy, do you want to take these patches via your
> shiny new mux-tree or do you prefer if I pick them?
>
G'day Wolfram,

I think he was hoping someone else would have a look at it based on this
feedback.

On 2017-01-04 15:13, Peter Rosin wrote:
> Irqs are not my strong point, I would prefer if someone else also had
> a look. And there are some comments below...

-- 
Regards
Phil Reid

Re: [PATCH v3 3/5] i2c: mux: pca954x: Add interrupt controller support

[Peter Rosin]

On 2017-01-09 10:02, Phil Reid wrote:
> Various muxes can aggregate multiple interrupts from each i2c bus.
> All of the muxes with interrupt support combine the active low irq lines
> using an internal 'and' function and generate a combined active low
> output. The muxes do provide the ability to read a control register to
> determine which irq is active. By making the mux an irq controller isr
> latency can potentially be reduced by reading the status register and
> then only calling the registered isr on that bus segment.
> 
> As there is no irq masking on the mux irq are disabled until irq_unmask is
> called at least once.
> 

I had a second reading of this patch. I'm still no master-of-irqs, though.
Anyway, I have some questions below. I guess it mostly shows that I don't
really know what I'm talking about here...

> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 127 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 125 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index bbf088e..84fc767 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -41,14 +41,19 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/i2c/pca954x.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/of_irq.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
>  
>  #define PCA954X_MAX_NCHANS 8
>  
> +#define PCA954X_IRQ_OFFSET 4
> +
>  enum pca_type {
>  	pca_9540,
>  	pca_9542,
> @@ -63,6 +68,7 @@ enum pca_type {
>  struct chip_desc {
>  	u8 nchans;
>  	u8 enable;	/* used for muxes only */
> +	u8 has_irq;
>  	enum muxtype {
>  		pca954x_ismux = 0,
>  		pca954x_isswi
> @@ -75,6 +81,9 @@ struct pca954x {
>  	u8 last_chan;		/* last register value */
>  	u8 deselect;
>  	struct i2c_client *client;
> +
> +	struct irq_domain *irq;
> +	unsigned int irq_mask;
>  };
>  
>  /* Provide specs for the PCA954x types we know about */
> @@ -87,19 +96,23 @@ struct pca954x {
>  	[pca_9542] = {
>  		.nchans = 2,
>  		.enable = 0x4,
> +		.has_irq = 1,
>  		.muxtype = pca954x_ismux,
>  	},
>  	[pca_9543] = {
>  		.nchans = 2,
> +		.has_irq = 1,
>  		.muxtype = pca954x_isswi,
>  	},
>  	[pca_9544] = {
>  		.nchans = 4,
>  		.enable = 0x4,
> +		.has_irq = 1,
>  		.muxtype = pca954x_ismux,
>  	},
>  	[pca_9545] = {
>  		.nchans = 4,
> +		.has_irq = 1,
>  		.muxtype = pca954x_isswi,
>  	},
>  	[pca_9547] = {
> @@ -222,6 +235,102 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
>  	return pca954x_reg_write(muxc->parent, client, data->last_chan);
>  }
>  
> +static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
> +{
> +	struct pca954x *data = dev_id;
> +	unsigned int child_irq;
> +	int ret, i, handled;
> +
> +	ret = i2c_smbus_read_byte(data->client);
> +	if (ret < 0)
> +		return IRQ_NONE;
> +
> +	for (i = 0; i < data->chip->nchans; i++) {
> +		if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
> +			child_irq = irq_linear_revmap(data->irq, i);
> +			handle_nested_irq(child_irq);
> +			handled++;
> +		}
> +	}
> +	return handled ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static void pca954x_irq_mask(struct irq_data *idata)
> +{
> +	struct pca954x *data = irq_data_get_irq_chip_data(idata);
> +	unsigned int pos = idata->hwirq;
> +
> +	data->irq_mask &= ~BIT(pos);
> +	if (!data->irq_mask)
> +		disable_irq(data->client->irq);
> +}
> +
> +static void pca954x_irq_unmask(struct irq_data *idata)
> +{
> +	struct pca954x *data = irq_data_get_irq_chip_data(idata);
> +	unsigned int pos = idata->hwirq;
> +
> +	if (!data->irq_mask)
> +		enable_irq(data->client->irq);
> +	data->irq_mask |= BIT(pos);
> +}

I assume the irq core makes sure that .irq_mask and .irq_unmask may not
be called concurrently?

> +
> +static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
> +{
> +	if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static struct irq_chip pca954x_irq_chip = {
> +	.name = "i2c-mux-pca954x",
> +	.irq_mask = pca954x_irq_mask,
> +	.irq_unmask = pca954x_irq_unmask,
> +	.irq_set_type = pca954x_irq_set_type,
> +};
> +
> +static int pca954x_irq_setup(struct i2c_mux_core *muxc)
> +{
> +	struct pca954x *data = i2c_mux_priv(muxc);
> +	struct i2c_client *client = data->client;
> +	int c, err, irq;
> +
> +	if (!data->chip->has_irq || client->irq <= 0)
> +		return 0;

I assume "client->irq <= 0" means that users not specifying any interrupts
continue to behave as they use to, right?

BTW, what does client->irq == 0 represent?

Cheers,
peda


> +
> +	data->irq = irq_domain_add_linear(client->dev.of_node,
> +					  data->chip->nchans,
> +					  &irq_domain_simple_ops, data);
> +	if (!data->irq)
> +		return -ENODEV;
> +
> +	for (c = 0; c < data->chip->nchans; c++) {
> +		irq = irq_create_mapping(data->irq, c);
> +		irq_set_chip_data(irq, data);
> +		irq_set_chip_and_handler(irq, &pca954x_irq_chip,
> +			handle_simple_irq);
> +	}
> +
> +	err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
> +					pca954x_irq_handler,
> +					IRQF_ONESHOT | IRQF_SHARED,
> +					"pca954x", data);
> +	if (err)
> +		goto err_req_irq;
> +
> +	disable_irq(data->client->irq);
> +
> +	return 0;
> +err_req_irq:
> +	for (c = 0; c < data->chip->nchans; c++) {
> +		irq = irq_find_mapping(data->irq, c);
> +		irq_dispose_mapping(irq);
> +	}
> +	irq_domain_remove(data->irq);
> +
> +	return err;
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -286,6 +395,10 @@ static int pca954x_probe(struct i2c_client *client,
>  	idle_disconnect_dt = of_node &&
>  		of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>  
> +	ret = pca954x_irq_setup(muxc);
> +	if (ret)
> +		goto fail_del_adapters;
> +
>  	/* Now create an adapter for each channel */
>  	for (num = 0; num < data->chip->nchans; num++) {
>  		bool idle_disconnect_pd = false;
> @@ -311,7 +424,7 @@ static int pca954x_probe(struct i2c_client *client,
>  			dev_err(&client->dev,
>  				"failed to register multiplexed adapter"
>  				" %d as bus %d\n", num, force);
> -			goto virt_reg_failed;
> +			goto fail_del_adapters;
>  		}
>  	}
>  
> @@ -322,7 +435,7 @@ static int pca954x_probe(struct i2c_client *client,
>  
>  	return 0;
>  
> -virt_reg_failed:
> +fail_del_adapters:
>  	i2c_mux_del_adapters(muxc);
>  	return ret;
>  }
> @@ -330,6 +443,16 @@ static int pca954x_probe(struct i2c_client *client,
>  static int pca954x_remove(struct i2c_client *client)
>  {
>  	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +	struct pca954x *data = i2c_mux_priv(muxc);
> +	int c, irq;
> +
> +	if (data->irq) {
> +		for (c = 0; c < data->chip->nchans; c++) {
> +			irq = irq_find_mapping(data->irq, c);
> +			irq_dispose_mapping(irq);
> +		}
> +		irq_domain_remove(data->irq);
> +	}
>  
>  	i2c_mux_del_adapters(muxc);
>  	return 0;
> 

Re: [PATCH v3 3/5] i2c: mux: pca954x: Add interrupt controller support

[Phil Reid]

On 13/01/2017 17:11, Peter Rosin wrote:
> On 2017-01-09 10:02, Phil Reid wrote:
>> Various muxes can aggregate multiple interrupts from each i2c bus.
>> All of the muxes with interrupt support combine the active low irq lines
>> using an internal 'and' function and generate a combined active low
>> output. The muxes do provide the ability to read a control register to
>> determine which irq is active. By making the mux an irq controller isr
>> latency can potentially be reduced by reading the status register and
>> then only calling the registered isr on that bus segment.
>>
>> As there is no irq masking on the mux irq are disabled until irq_unmask is
>> called at least once.
>>
>
> I had a second reading of this patch. I'm still no master-of-irqs, though.
> Anyway, I have some questions below. I guess it mostly shows that I don't
> really know what I'm talking about here...
I'm no expert either...

>
>> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
>> ---
>>  drivers/i2c/muxes/i2c-mux-pca954x.c | 127 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 125 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> index bbf088e..84fc767 100644
>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> @@ -41,14 +41,19 @@
>>  #include <linux/i2c.h>
>>  #include <linux/i2c-mux.h>
>>  #include <linux/i2c/pca954x.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>>  #include <linux/of_device.h>
>> +#include <linux/of_irq.h>
>>  #include <linux/pm.h>
>>  #include <linux/slab.h>
>>
>>  #define PCA954X_MAX_NCHANS 8
>>
>> +#define PCA954X_IRQ_OFFSET 4
>> +
>>  enum pca_type {
>>  	pca_9540,
>>  	pca_9542,
>> @@ -63,6 +68,7 @@ enum pca_type {
>>  struct chip_desc {
>>  	u8 nchans;
>>  	u8 enable;	/* used for muxes only */
>> +	u8 has_irq;
>>  	enum muxtype {
>>  		pca954x_ismux = 0,
>>  		pca954x_isswi
>> @@ -75,6 +81,9 @@ struct pca954x {
>>  	u8 last_chan;		/* last register value */
>>  	u8 deselect;
>>  	struct i2c_client *client;
>> +
>> +	struct irq_domain *irq;
>> +	unsigned int irq_mask;
>>  };
>>
>>  /* Provide specs for the PCA954x types we know about */
>> @@ -87,19 +96,23 @@ struct pca954x {
>>  	[pca_9542] = {
>>  		.nchans = 2,
>>  		.enable = 0x4,
>> +		.has_irq = 1,
>>  		.muxtype = pca954x_ismux,
>>  	},
>>  	[pca_9543] = {
>>  		.nchans = 2,
>> +		.has_irq = 1,
>>  		.muxtype = pca954x_isswi,
>>  	},
>>  	[pca_9544] = {
>>  		.nchans = 4,
>>  		.enable = 0x4,
>> +		.has_irq = 1,
>>  		.muxtype = pca954x_ismux,
>>  	},
>>  	[pca_9545] = {
>>  		.nchans = 4,
>> +		.has_irq = 1,
>>  		.muxtype = pca954x_isswi,
>>  	},
>>  	[pca_9547] = {
>> @@ -222,6 +235,102 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
>>  	return pca954x_reg_write(muxc->parent, client, data->last_chan);
>>  }
>>
>> +static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct pca954x *data = dev_id;
>> +	unsigned int child_irq;
>> +	int ret, i, handled;
>> +
>> +	ret = i2c_smbus_read_byte(data->client);
>> +	if (ret < 0)
>> +		return IRQ_NONE;
>> +
>> +	for (i = 0; i < data->chip->nchans; i++) {
>> +		if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
>> +			child_irq = irq_linear_revmap(data->irq, i);
>> +			handle_nested_irq(child_irq);
>> +			handled++;
>> +		}
>> +	}
>> +	return handled ? IRQ_HANDLED : IRQ_NONE;
>> +}
>> +
>> +static void pca954x_irq_mask(struct irq_data *idata)
>> +{
>> +	struct pca954x *data = irq_data_get_irq_chip_data(idata);
>> +	unsigned int pos = idata->hwirq;
>> +
>> +	data->irq_mask &= ~BIT(pos);
>> +	if (!data->irq_mask)
>> +		disable_irq(data->client->irq);
>> +}
>> +
>> +static void pca954x_irq_unmask(struct irq_data *idata)
>> +{
>> +	struct pca954x *data = irq_data_get_irq_chip_data(idata);
>> +	unsigned int pos = idata->hwirq;
>> +
>> +	if (!data->irq_mask)
>> +		enable_irq(data->client->irq);
>> +	data->irq_mask |= BIT(pos);
>> +}
>
> I assume the irq core makes sure that .irq_mask and .irq_unmask may not
> be called concurrently?
I also wasn't 100% sure about this myself.
Looking again at other drivers and the core irq code I'm still not sure.
But I think on review a lock of some kind is needed.
I think a spin_lock_irqsave would do it.
Will test next week and post a new version.

>
>> +
>> +static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
>> +{
>> +	if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
>> +		return -EINVAL;
>> +	return 0;
>> +}
>> +
>> +static struct irq_chip pca954x_irq_chip = {
>> +	.name = "i2c-mux-pca954x",
>> +	.irq_mask = pca954x_irq_mask,
>> +	.irq_unmask = pca954x_irq_unmask,
>> +	.irq_set_type = pca954x_irq_set_type,
>> +};
>> +
>> +static int pca954x_irq_setup(struct i2c_mux_core *muxc)
>> +{
>> +	struct pca954x *data = i2c_mux_priv(muxc);
>> +	struct i2c_client *client = data->client;
>> +	int c, err, irq;
>> +
>> +	if (!data->chip->has_irq || client->irq <= 0)
>> +		return 0;
>
> I assume "client->irq <= 0" means that users not specifying any interrupts
> continue to behave as they use to, right?
>
> BTW, what does client->irq == 0 represent?

This one I'm fairly confident on. Initially I had "client->irq < 0"
Which caused problems with an i2c mux that doesn't have irq's defined in the dt.

This commit seems to confirm that.
commit dab472eb931b ("i2c / ACPI: Use 0 to indicate that device does not have interrupt assigned")

And various patches converting to "<= 0" checks.
eg:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-August/364630.html

>
>
>
>> +
>> +	data->irq = irq_domain_add_linear(client->dev.of_node,
>> +					  data->chip->nchans,
>> +					  &irq_domain_simple_ops, data);
>> +	if (!data->irq)
>> +		return -ENODEV;
>> +
>> +	for (c = 0; c < data->chip->nchans; c++) {
>> +		irq = irq_create_mapping(data->irq, c);
>> +		irq_set_chip_data(irq, data);
>> +		irq_set_chip_and_handler(irq, &pca954x_irq_chip,
>> +			handle_simple_irq);
>> +	}
>> +
>> +	err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
>> +					pca954x_irq_handler,
>> +					IRQF_ONESHOT | IRQF_SHARED,
>> +					"pca954x", data);
>> +	if (err)
>> +		goto err_req_irq;
>> +
>> +	disable_irq(data->client->irq);
>> +
>> +	return 0;
>> +err_req_irq:
>> +	for (c = 0; c < data->chip->nchans; c++) {
>> +		irq = irq_find_mapping(data->irq, c);
>> +		irq_dispose_mapping(irq);
>> +	}
>> +	irq_domain_remove(data->irq);
>> +
>> +	return err;
>> +}
>> +
>>  /*
>>   * I2C init/probing/exit functions
>>   */
>> @@ -286,6 +395,10 @@ static int pca954x_probe(struct i2c_client *client,
>>  	idle_disconnect_dt = of_node &&
>>  		of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>>
>> +	ret = pca954x_irq_setup(muxc);
>> +	if (ret)
>> +		goto fail_del_adapters;
>> +
>>  	/* Now create an adapter for each channel */
>>  	for (num = 0; num < data->chip->nchans; num++) {
>>  		bool idle_disconnect_pd = false;
>> @@ -311,7 +424,7 @@ static int pca954x_probe(struct i2c_client *client,
>>  			dev_err(&client->dev,
>>  				"failed to register multiplexed adapter"
>>  				" %d as bus %d\n", num, force);
>> -			goto virt_reg_failed;
>> +			goto fail_del_adapters;
>>  		}
>>  	}
>>
>> @@ -322,7 +435,7 @@ static int pca954x_probe(struct i2c_client *client,
>>
>>  	return 0;
>>
>> -virt_reg_failed:
>> +fail_del_adapters:
>>  	i2c_mux_del_adapters(muxc);
>>  	return ret;
>>  }
>> @@ -330,6 +443,16 @@ static int pca954x_probe(struct i2c_client *client,
>>  static int pca954x_remove(struct i2c_client *client)
>>  {
>>  	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
>> +	struct pca954x *data = i2c_mux_priv(muxc);
>> +	int c, irq;
>> +
>> +	if (data->irq) {
>> +		for (c = 0; c < data->chip->nchans; c++) {
>> +			irq = irq_find_mapping(data->irq, c);
>> +			irq_dispose_mapping(irq);
>> +		}
>> +		irq_domain_remove(data->irq);
>> +	}
>>
>>  	i2c_mux_del_adapters(muxc);
>>  	return 0;
>>
>
>
>


-- 
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

Re: [PATCH v3 3/5] i2c: mux: pca954x: Add interrupt controller support

[Wolfram Sang]

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


> >BTW, what does client->irq == 0 represent?
> 
> This one I'm fairly confident on. Initially I had "client->irq < 0"
> Which caused problems with an i2c mux that doesn't have irq's defined in the dt.
> 
> This commit seems to confirm that.
> commit dab472eb931b ("i2c / ACPI: Use 0 to indicate that device does not have interrupt assigned")

In the Linux Kernel, 0 means no irq so one can write:

	if (!irq) ...

There used to be NO_IRQ per arch but this is converted over, although it
takes a lot of time... If there is a HW IRQ0, it needs to be remapped.
But since we rely on irq_desc these days and not irq numbers, this is
easy.

At least this was the transition a few years ago, but I don't think
something essential has changed. Please enlighten me, if so.

Regards,

   Wolfram


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

[PATCH v3 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for i2c-mux-irq-mask-en

[Phil Reid]

Unfortunately some hardware device will assert their irq line immediately
on power on and provide no mechanism to mask the irq. As the i2c muxes
provide no method to mask irq line this provides a work around by keeping
the parent irq masked until enough device drivers have loaded to service
all pending interrupts.

For example the the ltc1760 assert its SMBALERT irq immediately on power
on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
device is registered irq are enabled and fire continuously as the second
device driver has not yet loaded. Setting this parameter to 0x3 while
delay the irq being enabled until both devices are ready.

Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index aa09704..6de1e8e 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -19,6 +19,8 @@ Optional Properties:
   - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
     children in idle state. This is necessary for example, if there are several
     multiplexers on the bus and the devices behind them use same I2C addresses.
+  - nxp,irq-mask-enable: BitMask; Defines a mask for which irq lines need to be
+    unmasked before the parent irq line in enabled.
   - interrupt-parent: Phandle for the interrupt controller that services
     interrupts for this device.
   - interrupts: Interrupt mapping for IRQ.
@@ -36,6 +38,7 @@ Example:
 		#size-cells = <0>;
 		reg = <0x74>;
 
+		nxp,irq-mask-enable = <0x3>;
 		interrupt-parent = <&ipic>;
 		interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
 		interrupt-controller;
-- 
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 v3 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for i2c-mux-irq-mask-en

[Rob Herring]

On Mon, Jan 09, 2017 at 05:02:55PM +0800, Phil Reid wrote:
> Unfortunately some hardware device will assert their irq line immediately
> on power on and provide no mechanism to mask the irq. As the i2c muxes
> provide no method to mask irq line this provides a work around by keeping
> the parent irq masked until enough device drivers have loaded to service
> all pending interrupts.
> 
> For example the the ltc1760 assert its SMBALERT irq immediately on power
> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> device is registered irq are enabled and fire continuously as the second
> device driver has not yet loaded. Setting this parameter to 0x3 while
> delay the irq being enabled until both devices are ready.
> 
> Acked-by: Peter Rosin <peda@axentia.se>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++
>  1 file changed, 3 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v3 5/5] i2c: mux: pca954x: Add irq_mask_en to delay enabling irqs

[Phil Reid]

Unfortunately some hardware device will assert their irq line immediately
on power on and provide no mechanism to mask the irq. As the i2c muxes
provide no method to mask irq line this provides a work around by keeping
the parent irq masked until enough device drivers have loaded to service
all pending interrupts.

For example the the ltc1760 assert its SMBALERT irq immediately on power
on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
device is registered irq are enabled and fire continuously as the second
device driver has not yet loaded. Setting this parameter to 0x3 while
delay the irq being enabled until both devices are ready.

Acked-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 84fc767..fb1d245 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -75,6 +75,19 @@ struct chip_desc {
 	} muxtype;
 };
 
+/*
+ * irq_mask_enable: Provides a mechanism to work around hardware that asserts
+ * their irq immediately on power on. It allows the enabling of the irq to be
+ * delayed until the corresponding bits in the the irq_mask are set thru
+ * irq_unmask.
+ * For example the ltc1760 assert its SMBALERT irq immediately on power on.
+ * With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
+ * device is registered irq are enabled and fire continuously as the second
+ * device driver has not yet loaded. Setting this parameter to 0x3 while
+ * delay the irq being enabled until both devices are ready.
+ * This workaround will not work if two devices share an interrupt on the
+ * same bus segment.
+ */
 struct pca954x {
 	const struct chip_desc *chip;
 
@@ -84,6 +97,7 @@ struct pca954x {
 
 	struct irq_domain *irq;
 	unsigned int irq_mask;
+	unsigned int irq_mask_enable;
 };
 
 /* Provide specs for the PCA954x types we know about */
@@ -270,9 +284,12 @@ static void pca954x_irq_unmask(struct irq_data *idata)
 	struct pca954x *data = irq_data_get_irq_chip_data(idata);
 	unsigned int pos = idata->hwirq;
 
-	if (!data->irq_mask)
+	if (!data->irq_mask_enable && !data->irq_mask)
 		enable_irq(data->client->irq);
 	data->irq_mask |= BIT(pos);
+	if (data->irq_mask_enable &&
+		(data->irq_mask & data->irq_mask) == data->irq_mask_enable)
+		enable_irq(data->client->irq);
 }
 
 static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
@@ -395,6 +412,9 @@ static int pca954x_probe(struct i2c_client *client,
 	idle_disconnect_dt = of_node &&
 		of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
 
+	of_property_read_u32(of_node, "nxp,irq-mask-enable",
+			     &data->irq_mask_enable);
+
 	ret = pca954x_irq_setup(muxc);
 	if (ret)
 		goto fail_del_adapters;
-- 
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