[RFC PATCH 0/1] i2c: i2c-mux-pca954x: Add interrupt controller support

[RFC PATCH 0/1] i2c: i2c-mux-pca954x: Add interrupt controller support

[Phil Reid]

G'day All,

I'm seeking some feedback on this. Not really happy with it but can't think
of a good solution.  

This patch adds support to read the irq status register from the a PCA9543
i2c mux. This chip has two active low interrupt inputs with are combined    
into a single active low interrupt output. It does not provide the ability 
to mask interrupts. Only to read which interrupt line is active.

On the hardware I'm interfacing too I have an ltc1760 sbs manager on each 
multiplexed bus. On booting these may assert there SMBALERT irq outputs 
immediately, again these devices have no ability to mask there interrupt. 
So the irq is asserted during boot until the ltc1760 driver is started.
And there's no way to mask it. My work around is to enable the irq until both
irq pins are unmasked. Not sure what else to do. The masking could be a dts
property to make it configurable.

ltc1760 driver is currently being review for submission in 
Karl-Heinz Schneider <karl-heinz@schneider-inet.de> patch series.

Appreciate any feedback on what could be done here.


Phil Reid (1):
  i2c: i2c-mux-pca954x: Add interrupt controller support.

 drivers/i2c/muxes/i2c-mux-pca954x.c | 116 ++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

-- 
1.8.3.1

[RFC PATCH 1/1] i2c: i2c-mux-pca954x: Add interrupt controller support.

[Phil Reid]

The pca9543 can aggregate multiple interrupts from each i2c bus.
However it provides no ability to mask interrupts on each channel.
So if one bus is asserting interrupts than it will continuously
trigger the interrupts until another driver clears it. As a
workaround don't enable interrupts until both irqs have been unmasked.

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

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 284e342..9ff2540 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -45,9 +45,14 @@
 #include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/interrupt.h>
 
 #define PCA954X_MAX_NCHANS 8
 
+#define PCA9543_IRQ_OFFSET 4
+
 enum pca_type {
 	pca_9540,
 	pca_9542,
@@ -67,6 +72,8 @@ struct pca954x {
 	struct i2c_client *client;
 	bool idle_reset;
 	struct gpio_desc *gpio;
+	struct irq_domain *irq_domain;
+	unsigned int irq_mask;
 };
 
 struct chip_desc {
@@ -194,6 +201,110 @@ 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 *devid)
+{
+	struct pca954x *data = i2c_mux_priv(devid);
+	struct i2c_client *client = data->client;
+	int i, ret, child_irq;
+	unsigned int nhandled = 0;
+
+	ret = i2c_smbus_read_byte(client);
+	if (ret < 0)
+		return IRQ_NONE;
+
+	for (i = 0; i < 2; i++) {
+		if (ret & BIT(PCA9543_IRQ_OFFSET+i)) {
+			child_irq = irq_find_mapping(data->irq_domain, i);
+			handle_nested_irq(child_irq);
+			nhandled++;
+		}
+	}
+
+	return (nhandled > 0) ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static void pca954x_irq_mask(struct irq_data *idata)
+{
+	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
+	struct pca954x *data = i2c_mux_priv(muxc);
+	unsigned int pos = idata->hwirq;
+
+	data->irq_mask &= ~BIT(pos);
+	if ((data->irq_mask & 0x3) != 0)
+		disable_irq(data->client->irq);
+}
+
+static void pca954x_irq_unmask(struct irq_data *idata)
+{
+	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
+	struct pca954x *data = i2c_mux_priv(muxc);
+	unsigned int pos = idata->hwirq;
+
+	data->irq_mask |= ~BIT(pos);
+	if ((data->irq_mask & 0x3) == 0x3)
+		enable_irq(data->client->irq);
+}
+
+static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
+{
+	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 pca953x_irq_domain_map(struct irq_domain *h, unsigned int irq,
+				 irq_hw_number_t hwirq)
+{
+	struct i2c_mux_core *muxc = h->host_data;
+
+	irq_set_chip_data(irq, muxc);
+	irq_set_chip_and_handler(irq, &pca954x_irq_chip, handle_level_irq);
+	return 0;
+}
+
+static const struct irq_domain_ops pca953x_irq_domain_ops = {
+	.map	= pca953x_irq_domain_map,
+	.xlate	= irq_domain_xlate_twocell,
+};
+
+static int pca953x_irq_setup(struct i2c_mux_core *muxc)
+{
+	struct pca954x *data = i2c_mux_priv(muxc);
+	struct i2c_client *client = data->client;
+	int i, ret;
+	int irq = client->irq;
+
+	if ((data->type != pca_9543) && (irq != -1))
+		return 0;
+
+	ret = devm_request_threaded_irq(&client->dev, irq, NULL,
+					pca954x_irq_handler,
+					IRQF_TRIGGER_HIGH | IRQF_ONESHOT |
+					   IRQF_SHARED,
+					dev_name(&client->dev), muxc);
+	if (ret) {
+		dev_err(&client->dev, "failed to request irq %d %d\n",
+			client->irq, ret);
+		return ret;
+	}
+
+	disable_irq(irq);
+
+	data->irq_domain = irq_domain_add_simple(client->dev.of_node,
+						 2, 0, &pca953x_irq_domain_ops,
+						 muxc);
+
+	for (i = 0; i < 2; i++)
+		irq_set_parent(irq_find_mapping(data->irq_domain, i), irq);
+
+	return 0;
+}
+
 /*
  * I2C init/probing/exit functions
  */
@@ -274,6 +385,10 @@ static int pca954x_probe(struct i2c_client *client,
 		}
 	}
 
+	ret = pca953x_irq_setup(muxc);
+	if (ret)
+		goto irq_setup_failed;
+
 	dev_info(&client->dev,
 		 "registered %d multiplexed busses for I2C %s %s\n",
 		 num, chips[data->type].muxtype == pca954x_ismux
@@ -281,6 +396,7 @@ static int pca954x_probe(struct i2c_client *client,
 
 	return 0;
 
+irq_setup_failed:
 virt_reg_failed:
 	i2c_mux_del_adapters(muxc);
 	return ret;
-- 
1.8.3.1

Re: [RFC PATCH 1/1] i2c: i2c-mux-pca954x: Add interrupt controller support.

[Peter Rosin]

On 2016-07-27 05:05, Phil Reid wrote:
> The pca9543 can aggregate multiple interrupts from each i2c bus.
> However it provides no ability to mask interrupts on each channel.
> So if one bus is asserting interrupts than it will continuously
> trigger the interrupts until another driver clears it. As a
> workaround don't enable interrupts until both irqs have been unmasked.
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 116 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 284e342..9ff2540 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -45,9 +45,14 @@
>  #include <linux/of.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/interrupt.h>

Please keep the includes sorted.

>  #define PCA954X_MAX_NCHANS 8
>  
> +#define PCA9543_IRQ_OFFSET 4

PCA9544 and PCA9545 also support interrupts, and they look similar.
It would be preferable if the changes accommodated the other chips
supported by the driver. They all look similar, and the irq bits
looks like they are always in the same place, but I think I would
like this offset to go into the chip_desc struct all the same.
At the very least the define should be renamed PCA954X_IRQ_OFFSET.

> +
>  enum pca_type {
>  	pca_9540,
>  	pca_9542,
> @@ -67,6 +72,8 @@ struct pca954x {
>  	struct i2c_client *client;
>  	bool idle_reset;
>  	struct gpio_desc *gpio;
> +	struct irq_domain *irq_domain;
> +	unsigned int irq_mask;
>  };
>  
>  struct chip_desc {
> @@ -194,6 +201,110 @@ 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 *devid)
> +{
> +	struct pca954x *data = i2c_mux_priv(devid);
> +	struct i2c_client *client = data->client;
> +	int i, ret, child_irq;
> +	unsigned int nhandled = 0;
> +
> +	ret = i2c_smbus_read_byte(client);
> +	if (ret < 0)
> +		return IRQ_NONE;
> +
> +	for (i = 0; i < 2; i++) {

Please use nchans instead of 2 here.

> +		if (ret & BIT(PCA9543_IRQ_OFFSET+i)) {
> +			child_irq = irq_find_mapping(data->irq_domain, i);
> +			handle_nested_irq(child_irq);
> +			nhandled++;
> +		}
> +	}
> +
> +	return (nhandled > 0) ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static void pca954x_irq_mask(struct irq_data *idata)
> +{
> +	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
> +	struct pca954x *data = i2c_mux_priv(muxc);
> +	unsigned int pos = idata->hwirq;
> +
> +	data->irq_mask &= ~BIT(pos);
> +	if ((data->irq_mask & 0x3) != 0)

The 0x3 mask should probably also go into struct chip_desc, then a non-
empty value in this field could be used as trigger for initing the irq
stuff.

> +		disable_irq(data->client->irq);
> +}
> +
> +static void pca954x_irq_unmask(struct irq_data *idata)
> +{
> +	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
> +	struct pca954x *data = i2c_mux_priv(muxc);
> +	unsigned int pos = idata->hwirq;
> +
> +	data->irq_mask |= ~BIT(pos);
> +	if ((data->irq_mask & 0x3) == 0x3)

This is what you mentioned in the commit message, but I don't get it.
Please explain further, and also think about how the same problem
could be handled should there be 4 incoming irq lines as in pca9544
and pca9545.

> +		enable_irq(data->client->irq);
> +}
> +
> +static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
> +{
> +	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 pca953x_irq_domain_map(struct irq_domain *h, unsigned int irq,
> +				 irq_hw_number_t hwirq)

This function should be named pca954x_irq_domain_map.

> +{
> +	struct i2c_mux_core *muxc = h->host_data;
> +
> +	irq_set_chip_data(irq, muxc);
> +	irq_set_chip_and_handler(irq, &pca954x_irq_chip, handle_level_irq);
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops pca953x_irq_domain_ops = {

This constant should be named pca954x_irq_domain_ops.

> +	.map	= pca953x_irq_domain_map,
> +	.xlate	= irq_domain_xlate_twocell,
> +};
> +
> +static int pca953x_irq_setup(struct i2c_mux_core *muxc)

This function should be named pca954x_irq_setup.

> +{
> +	struct pca954x *data = i2c_mux_priv(muxc);
> +	struct i2c_client *client = data->client;
> +	int i, ret;
> +	int irq = client->irq;
> +
> +	if ((data->type != pca_9543) && (irq != -1))
> +		return 0;

So here, please use the new irq mask value (0x3) that I proposed
above, instead of the explicitly check for pca9543.

> +	ret = devm_request_threaded_irq(&client->dev, irq, NULL,
> +					pca954x_irq_handler,
> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT |
> +					   IRQF_SHARED,
> +					dev_name(&client->dev), muxc);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to request irq %d %d\n",
> +			client->irq, ret);
> +		return ret;
> +	}
> +
> +	disable_irq(irq);
> +
> +	data->irq_domain = irq_domain_add_simple(client->dev.of_node,
> +						 2, 0, &pca953x_irq_domain_ops,
> +						 muxc);
> +
> +	for (i = 0; i < 2; i++)

nchans instead of 2, two instances I guess? I know very little about kernel
interrupt handling and can't really say anything about if your code interfacing
the irq subsystem is good to go...

> +		irq_set_parent(irq_find_mapping(data->irq_domain, i), irq);
> +
> +	return 0;
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -274,6 +385,10 @@ static int pca954x_probe(struct i2c_client *client,
>  		}
>  	}
>  
> +	ret = pca953x_irq_setup(muxc);
> +	if (ret)
> +		goto irq_setup_failed;
> +
>  	dev_info(&client->dev,
>  		 "registered %d multiplexed busses for I2C %s %s\n",
>  		 num, chips[data->type].muxtype == pca954x_ismux
> @@ -281,6 +396,7 @@ static int pca954x_probe(struct i2c_client *client,
>  
>  	return 0;
>  
> +irq_setup_failed:
>  virt_reg_failed:
>  	i2c_mux_del_adapters(muxc);
>  	return ret;
> 

Cheers,
Peter

Re: [RFC PATCH 1/1] i2c: i2c-mux-pca954x: Add interrupt controller support.

[Phil Reid]

G'day Peter,

Thanks for the feedback.
+linux-kernel@vger.kernel.org

On 27/07/2016 13:32, Peter Rosin wrote:
> On 2016-07-27 05:05, Phil Reid wrote:
>> The pca9543 can aggregate multiple interrupts from each i2c bus.
>> However it provides no ability to mask interrupts on each channel.
>> So if one bus is asserting interrupts than it will continuously
>> trigger the interrupts until another driver clears it. As a
>> workaround don't enable interrupts until both irqs have been unmasked.
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>  drivers/i2c/muxes/i2c-mux-pca954x.c | 116 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 116 insertions(+)
>>
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> index 284e342..9ff2540 100644
>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> @@ -45,9 +45,14 @@
>>  #include <linux/of.h>
>>  #include <linux/pm.h>
>>  #include <linux/slab.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/interrupt.h>
>
> Please keep the includes sorted.
>
>>  #define PCA954X_MAX_NCHANS 8
>>
>> +#define PCA9543_IRQ_OFFSET 4
>
> PCA9544 and PCA9545 also support interrupts, and they look similar.
> It would be preferable if the changes accommodated the other chips
> supported by the driver. They all look similar, and the irq bits
> looks like they are always in the same place, but I think I would
> like this offset to go into the chip_desc struct all the same.
> At the very least the define should be renamed PCA954X_IRQ_OFFSET.

No problem. I was intending to clean things up for that but the interrupt masking
was raising concerns with the whole patch.

>
>> +
>>  enum pca_type {
>>  	pca_9540,
>>  	pca_9542,
>> @@ -67,6 +72,8 @@ struct pca954x {
>>  	struct i2c_client *client;
>>  	bool idle_reset;
>>  	struct gpio_desc *gpio;
>> +	struct irq_domain *irq_domain;
>> +	unsigned int irq_mask;
>>  };
>>
>>  struct chip_desc {
>> @@ -194,6 +201,110 @@ 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 *devid)
>> +{
>> +	struct pca954x *data = i2c_mux_priv(devid);
>> +	struct i2c_client *client = data->client;
>> +	int i, ret, child_irq;
>> +	unsigned int nhandled = 0;
>> +
>> +	ret = i2c_smbus_read_byte(client);
>> +	if (ret < 0)
>> +		return IRQ_NONE;
>> +
>> +	for (i = 0; i < 2; i++) {
>
> Please use nchans instead of 2 here.
>
>> +		if (ret & BIT(PCA9543_IRQ_OFFSET+i)) {
>> +			child_irq = irq_find_mapping(data->irq_domain, i);
>> +			handle_nested_irq(child_irq);
>> +			nhandled++;
>> +		}
>> +	}
>> +
>> +	return (nhandled > 0) ? IRQ_HANDLED : IRQ_NONE;
>> +}
>> +
>> +static void pca954x_irq_mask(struct irq_data *idata)
>> +{
>> +	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
>> +	struct pca954x *data = i2c_mux_priv(muxc);
>> +	unsigned int pos = idata->hwirq;
>> +
>> +	data->irq_mask &= ~BIT(pos);
>> +	if ((data->irq_mask & 0x3) != 0)
>
> The 0x3 mask should probably also go into struct chip_desc, then a non-
> empty value in this field could be used as trigger for initing the irq
> stuff.
>
>> +		disable_irq(data->client->irq);
>> +}
>> +
>> +static void pca954x_irq_unmask(struct irq_data *idata)
>> +{
>> +	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
>> +	struct pca954x *data = i2c_mux_priv(muxc);
>> +	unsigned int pos = idata->hwirq;
>> +
>> +	data->irq_mask |= ~BIT(pos);
>> +	if ((data->irq_mask & 0x3) == 0x3)
>
> This is what you mentioned in the commit message, but I don't get it.
> Please explain further, and also think about how the same problem
> could be handled should there be 4 incoming irq lines as in pca9544
> and pca9545.

Yes this is the tricky point.

Consider this:

Host - pca954x + Dev1
                \ Dev2

In my case devs are ltc1760 that generate SMBALERT's (active low irq) which
are not maskable in the device. There is nothing that can be configured in the
device to prevent them assert an irq.

If just considering them as shared interrupts, ie pca954x is not an irq ctlr, just a wired and.
On boot Dev1 is registered and enable shared irq. If Dev2 is asserting SMBALERT
this results in irq being continuously triggered, trigger Dev1 irq handler which
can't clear the irq because it's being asserted by Dev2.

My system eventually boots but spends alot of time looping in the irq for dev1
until dev2 gets registered.



The same problem occurs with the pca954x driver present, unless I disable the irq
until both slaves have unmasked the irq. This is not a good generic solution as the
system may be used with only one irq active.

It's really a deficiency in the hardware design but I'm not sure I'll get that fixed.
Or I may be missing something obvious to deal with this situation

>
>> +		enable_irq(data->client->irq);
>> +}
>> +
>> +static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
>> +{
>> +	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 pca953x_irq_domain_map(struct irq_domain *h, unsigned int irq,
>> +				 irq_hw_number_t hwirq)
>
> This function should be named pca954x_irq_domain_map.
>
>> +{
>> +	struct i2c_mux_core *muxc = h->host_data;
>> +
>> +	irq_set_chip_data(irq, muxc);
>> +	irq_set_chip_and_handler(irq, &pca954x_irq_chip, handle_level_irq);
>> +	return 0;
>> +}
>> +
>> +static const struct irq_domain_ops pca953x_irq_domain_ops = {
>
> This constant should be named pca954x_irq_domain_ops.
>
>> +	.map	= pca953x_irq_domain_map,
>> +	.xlate	= irq_domain_xlate_twocell,
>> +};
>> +
>> +static int pca953x_irq_setup(struct i2c_mux_core *muxc)
>
> This function should be named pca954x_irq_setup.
>
>> +{
>> +	struct pca954x *data = i2c_mux_priv(muxc);
>> +	struct i2c_client *client = data->client;
>> +	int i, ret;
>> +	int irq = client->irq;
>> +
>> +	if ((data->type != pca_9543) && (irq != -1))
>> +		return 0;
>
> So here, please use the new irq mask value (0x3) that I proposed
> above, instead of the explicitly check for pca9543.
>
>> +	ret = devm_request_threaded_irq(&client->dev, irq, NULL,
>> +					pca954x_irq_handler,
>> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT |
>> +					   IRQF_SHARED,
>> +					dev_name(&client->dev), muxc);
>> +	if (ret) {
>> +		dev_err(&client->dev, "failed to request irq %d %d\n",
>> +			client->irq, ret);
>> +		return ret;
>> +	}
>> +
>> +	disable_irq(irq);
>> +
>> +	data->irq_domain = irq_domain_add_simple(client->dev.of_node,
>> +						 2, 0, &pca953x_irq_domain_ops,
>> +						 muxc);
>> +
>> +	for (i = 0; i < 2; i++)
>
> nchans instead of 2, two instances I guess? I know very little about kernel
> interrupt handling and can't really say anything about if your code interfacing
> the irq subsystem is good to go...

I've add linux-kernel@vger.kernel.org to see if anyone else to offer suggestions.

>
>> +		irq_set_parent(irq_find_mapping(data->irq_domain, i), irq);
>> +
>> +	return 0;
>> +}
>> +
>>  /*
>>   * I2C init/probing/exit functions
>>   */
>> @@ -274,6 +385,10 @@ static int pca954x_probe(struct i2c_client *client,
>>  		}
>>  	}
>>
>> +	ret = pca953x_irq_setup(muxc);
>> +	if (ret)
>> +		goto irq_setup_failed;
>> +
>>  	dev_info(&client->dev,
>>  		 "registered %d multiplexed busses for I2C %s %s\n",
>>  		 num, chips[data->type].muxtype == pca954x_ismux
>> @@ -281,6 +396,7 @@ static int pca954x_probe(struct i2c_client *client,
>>
>>  	return 0;
>>
>> +irq_setup_failed:
>>  virt_reg_failed:
>>  	i2c_mux_del_adapters(muxc);
>>  	return ret;
>>
>



-- 
Regards
Phil Reid

Re: [RFC PATCH 1/1] i2c: i2c-mux-pca954x: Add interrupt controller support.

[Peter Rosin]

On 2016-07-28 04:44, Phil Reid wrote:
> G'day Peter,
> 
> Thanks for the feedback.
> +linux-kernel@vger.kernel.org
> 
> On 27/07/2016 13:32, Peter Rosin wrote:
>> On 2016-07-27 05:05, Phil Reid wrote:
>>> +static void pca954x_irq_mask(struct irq_data *idata)
>>> +{
>>> +	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
>>> +	struct pca954x *data = i2c_mux_priv(muxc);
>>> +	unsigned int pos = idata->hwirq;
>>> +
>>> +	data->irq_mask &= ~BIT(pos);
>>> +	if ((data->irq_mask & 0x3) != 0)
>>
>> The 0x3 mask should probably also go into struct chip_desc, then a non-
>> empty value in this field could be used as trigger for initing the irq
>> stuff.

Ok, this comment just shows that I was not getting it. Please ignore it.
Some other (new) thing in chip_desc is needed to trigger irq init for the
chips that have irq support.

>>> +		disable_irq(data->client->irq);
>>> +}
>>> +
>>> +static void pca954x_irq_unmask(struct irq_data *idata)
>>> +{
>>> +	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
>>> +	struct pca954x *data = i2c_mux_priv(muxc);
>>> +	unsigned int pos = idata->hwirq;
>>> +
>>> +	data->irq_mask |= ~BIT(pos);
>>> +	if ((data->irq_mask & 0x3) == 0x3)
>>
>> This is what you mentioned in the commit message, but I don't get it.
>> Please explain further, and also think about how the same problem
>> could be handled should there be 4 incoming irq lines as in pca9544
>> and pca9545.
> 
> Yes this is the tricky point.
> 
> Consider this:
> 
> Host - pca954x + Dev1
>                 \ Dev2
> 
> In my case devs are ltc1760 that generate SMBALERT's (active low irq) which
> are not maskable in the device. There is nothing that can be configured in the
> device to prevent them assert an irq.
> 
> If just considering them as shared interrupts, ie pca954x is not an irq ctlr, just a wired and.
> On boot Dev1 is registered and enable shared irq. If Dev2 is asserting SMBALERT
> this results in irq being continuously triggered, trigger Dev1 irq handler which
> can't clear the irq because it's being asserted by Dev2.
> 
> My system eventually boots but spends alot of time looping in the irq for dev1
> until dev2 gets registered.
> 
> 
> 
> The same problem occurs with the pca954x driver present, unless I disable the irq
> until both slaves have unmasked the irq. This is not a good generic solution as the
> system may be used with only one irq active.
> 
> It's really a deficiency in the hardware design but I'm not sure I'll get that fixed.
> Or I may be missing something obvious to deal with this situation

Ok, I think I get the problem, but I too am at a loss and see no elegant solution.
One sad thing about your workaround is that it is not working at all unless there
is an irq user on all mux child segments that unmasks the corresponding irq. Right?

I think the default should be that the driver assumes sane HW, i.e. the irq inputs
are not asserted unless some driver is able to clear them. You can then add an
option to keep irqs disabled when the mask "is wrong". As a bonus, I think this
"is wrong" test should be configurable so that you specify a bitmask of irqs that
all has to be unmasked for the irq to be enabled (and use that instead of the 0x3
in the pca954x_irq_mask/pca954x_irq_unmask functions). That makes the workaround
more flexible.

Cheers,
Peter

Re: [RFC PATCH 1/1] i2c: i2c-mux-pca954x: Add interrupt controller support.

[Phil Reid]

On 29/07/2016 13:48, Peter Rosin wrote:
> On 2016-07-28 04:44, Phil Reid wrote:
>> G'day Peter,
>>
>> Thanks for the feedback.
>> +linux-kernel@vger.kernel.org
>>
>> On 27/07/2016 13:32, Peter Rosin wrote:
>>> On 2016-07-27 05:05, Phil Reid wrote:
>>>> +static void pca954x_irq_mask(struct irq_data *idata)
>>>> +{
>>>> +	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
>>>> +	struct pca954x *data = i2c_mux_priv(muxc);
>>>> +	unsigned int pos = idata->hwirq;
>>>> +
>>>> +	data->irq_mask &= ~BIT(pos);
>>>> +	if ((data->irq_mask & 0x3) != 0)
>>>
>>> The 0x3 mask should probably also go into struct chip_desc, then a non-
>>> empty value in this field could be used as trigger for initing the irq
>>> stuff.
>
> Ok, this comment just shows that I was not getting it. Please ignore it.
> Some other (new) thing in chip_desc is needed to trigger irq init for the
> chips that have irq support.
>
>>>> +		disable_irq(data->client->irq);
>>>> +}
>>>> +
>>>> +static void pca954x_irq_unmask(struct irq_data *idata)
>>>> +{
>>>> +	struct i2c_mux_core *muxc = irq_data_get_irq_chip_data(idata);
>>>> +	struct pca954x *data = i2c_mux_priv(muxc);
>>>> +	unsigned int pos = idata->hwirq;
>>>> +
>>>> +	data->irq_mask |= ~BIT(pos);
>>>> +	if ((data->irq_mask & 0x3) == 0x3)
>>>
>>> This is what you mentioned in the commit message, but I don't get it.
>>> Please explain further, and also think about how the same problem
>>> could be handled should there be 4 incoming irq lines as in pca9544
>>> and pca9545.
>>
>> Yes this is the tricky point.
>>
>> Consider this:
>>
>> Host - pca954x + Dev1
>>                 \ Dev2
>>
>> In my case devs are ltc1760 that generate SMBALERT's (active low irq) which
>> are not maskable in the device. There is nothing that can be configured in the
>> device to prevent them assert an irq.
>>
>> If just considering them as shared interrupts, ie pca954x is not an irq ctlr, just a wired and.
>> On boot Dev1 is registered and enable shared irq. If Dev2 is asserting SMBALERT
>> this results in irq being continuously triggered, trigger Dev1 irq handler which
>> can't clear the irq because it's being asserted by Dev2.
>>
>> My system eventually boots but spends alot of time looping in the irq for dev1
>> until dev2 gets registered.
>>
>>
>>
>> The same problem occurs with the pca954x driver present, unless I disable the irq
>> until both slaves have unmasked the irq. This is not a good generic solution as the
>> system may be used with only one irq active.
>>
>> It's really a deficiency in the hardware design but I'm not sure I'll get that fixed.
>> Or I may be missing something obvious to deal with this situation
>
> Ok, I think I get the problem, but I too am at a loss and see no elegant solution.
> One sad thing about your workaround is that it is not working at all unless there
> is an irq user on all mux child segments that unmasks the corresponding irq. Right?
>
> I think the default should be that the driver assumes sane HW, i.e. the irq inputs
> are not asserted unless some driver is able to clear them. You can then add an
> option to keep irqs disabled when the mask "is wrong". As a bonus, I think this
> "is wrong" test should be configurable so that you specify a bitmask of irqs that
> all has to be unmasked for the irq to be enabled (and use that instead of the 0x3
> in the pca954x_irq_mask/pca954x_irq_unmask functions). That makes the workaround
> more flexible.

Correct, It works for my use case at the moment. There would need to be a way to define a mask.
Via device tree for example.
I think sane devices that have irq masking don't really need the pca954x to be a irq source.
They could be configured with a shared IRQ as the pCA954x just ANDs then incoming lines together.
So does my use case and workaround justify the complexity added to the driver?

Thou there is possibly a performance benefit from reading the pca954x to dispatch the appropriate
irq, which would save alot of i2c transactions to probe each possible irq source device and assocaited
i2c mux switching.

WDYR?


-- 
Regards
Phil Reid

Re: [RFC PATCH 1/1] i2c: i2c-mux-pca954x: Add interrupt controller support.

[Peter Rosin]

On 2016-08-01 08:25, Phil Reid wrote:
> On 29/07/2016 13:48, Peter Rosin wrote:
>> Ok, I think I get the problem, but I too am at a loss and see no elegant solution.
>> One sad thing about your workaround is that it is not working at all unless there
>> is an irq user on all mux child segments that unmasks the corresponding irq. Right?
>>
>> I think the default should be that the driver assumes sane HW, i.e. the irq inputs
>> are not asserted unless some driver is able to clear them. You can then add an
>> option to keep irqs disabled when the mask "is wrong". As a bonus, I think this
>> "is wrong" test should be configurable so that you specify a bitmask of irqs that
>> all has to be unmasked for the irq to be enabled (and use that instead of the 0x3
>> in the pca954x_irq_mask/pca954x_irq_unmask functions). That makes the workaround
>> more flexible.
> 
> Correct, It works for my use case at the moment. There would need to be a way to define a mask.
> Via device tree for example.

Yes, some optional property in DT was my vision also.

> I think sane devices that have irq masking don't really need the pca954x to be a irq source.
> They could be configured with a shared IRQ as the pCA954x just ANDs then incoming lines together.

Right. And I suppose we don't know if this is already happening... AFAIU,
that approach would still work even if pca954x is made an interrupt source.
Or?

I mean, as you say, the pca954x functions as an electrical AND, and if
interrupt clients register with whatever the parent interrupt controller
is, they would be not be affected if there is also an interrupt controller
for pca954x?

Yes, such imaginary uses could have just wired all the irq lines together,
but we don't know if they actually did that or if they possibly went via
the irq routing in the pca954x for whatever reason...

> So does my use case and workaround justify the complexity added to the driver?
> 
> Thou there is possibly a performance benefit from reading the pca954x to dispatch the appropriate
> irq, which would save alot of i2c transactions to probe each possible irq source device and assocaited
> i2c mux switching.
> 
> WDYR?

It was the last bit that I also realized, and it would be nice to not have to
dig through all irq devices on the child side of the mux (and other clients
sharing the irq line with the pca954x for that matter). In theory there could
be quite a few...

So, yes, I think it might be worthwhile to make it an interrupt controller.
And then the tweak with your mask is no longer the big part of it...

BTW, you also need to change bindings docs in
Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt

I also noticed that you are using irq_domain_add_legacy which is marked as,
*tada* legacy, and that most drivers should use a linear domain. Sounds like
a linear domain fits the use case here, no?

And another note, the workaround for your limited hw is rather non-generic.
I mean, if the irq line from the pca954x to the parent interrupt controller
is shared with something else, then there would still be a flood even with
the workaround. Or am I misunderstanding that?

Cheers,
Peter

Re: [RFC PATCH 1/1] i2c: i2c-mux-pca954x: Add interrupt controller support.

[Phil Reid]

On 1/08/2016 16:22, Peter Rosin wrote:
> On 2016-08-01 08:25, Phil Reid wrote:
>> On 29/07/2016 13:48, Peter Rosin wrote:
>>> Ok, I think I get the problem, but I too am at a loss and see no elegant solution.
>>> One sad thing about your workaround is that it is not working at all unless there
>>> is an irq user on all mux child segments that unmasks the corresponding irq. Right?
>>>
>>> I think the default should be that the driver assumes sane HW, i.e. the irq inputs
>>> are not asserted unless some driver is able to clear them. You can then add an
>>> option to keep irqs disabled when the mask "is wrong". As a bonus, I think this
>>> "is wrong" test should be configurable so that you specify a bitmask of irqs that
>>> all has to be unmasked for the irq to be enabled (and use that instead of the 0x3
>>> in the pca954x_irq_mask/pca954x_irq_unmask functions). That makes the workaround
>>> more flexible.
>>
>> Correct, It works for my use case at the moment. There would need to be a way to define a mask.
>> Via device tree for example.
>
> Yes, some optional property in DT was my vision also.
>
>> I think sane devices that have irq masking don't really need the pca954x to be a irq source.
>> They could be configured with a shared IRQ as the pCA954x just ANDs then incoming lines together.
>
> Right. And I suppose we don't know if this is already happening... AFAIU,
> that approach would still work even if pca954x is made an interrupt source.
> Or?
>
> I mean, as you say, the pca954x functions as an electrical AND, and if
> interrupt clients register with whatever the parent interrupt controller
> is, they would be not be affected if there is also an interrupt controller
> for pca954x?
>
> Yes, such imaginary uses could have just wired all the irq lines together,
> but we don't know if they actually did that or if they possibly went via
> the irq routing in the pca954x for whatever reason...
>
>> So does my use case and workaround justify the complexity added to the driver?
>>
>> Thou there is possibly a performance benefit from reading the pca954x to dispatch the appropriate
>> irq, which would save alot of i2c transactions to probe each possible irq source device and assocaited
>> i2c mux switching.
>>
>> WDYR?
>
> It was the last bit that I also realized, and it would be nice to not have to
> dig through all irq devices on the child side of the mux (and other clients
> sharing the irq line with the pca954x for that matter). In theory there could
> be quite a few...
>
> So, yes, I think it might be worthwhile to make it an interrupt controller.
> And then the tweak with your mask is no longer the big part of it...
>
> BTW, you also need to change bindings docs in
> Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
Yeap, just wanted to nail down a direction before going thru that as well.
>
> I also noticed that you are using irq_domain_add_legacy which is marked as,
> *tada* legacy, and that most drivers should use a linear domain. Sounds like
> a linear domain fits the use case here, no?
I'll fix that.
>
> And another note, the workaround for your limited hw is rather non-generic.
> I mean, if the irq line from the pca954x to the parent interrupt controller
> is shared with something else, then there would still be a flood even with
> the workaround. Or am I misunderstanding that?
>

Yes there's still going to be a flood in that case.
My workaround is really specific to my hardware where I know that there is only
one irq source on each input the pca954x.

I'll start a patch with the irq support and then look at the how invasive my workaround is.

Thanks

-- 
Regards
Phil Reid