[v5,1/6] I2C: i2c-smbus: add device tree support

Submitted by Phil Reid on Sept. 21, 2016, 8:41 a.m.

Details

Message ID 1474447274-90821-2-git-send-email-preid@electromag.com.au
State New
Headers show

Commit Message

Phil Reid Sept. 21, 2016, 8:41 a.m.
From: Andrea Merello <andrea.merello@gmail.com>

According to Documentation/i2c/smbus-protocol, a smbus controller driver
that wants to hook-in smbus extensions support, can call
i2c_setup_smbus_alert(). There are very few drivers that are currently
doing this.

However the i2c-smbus module can also work with any
smbus-extensions-unaware I2C controller, as long as we provide an extra
IRQ line connected to the I2C bus ALARM signal.

This patch makes it possible to go this way via DT. Note that the DT node
will indeed describe a (little) piece of HW, that is the routing of the
ALARM signal to an IRQ line (so it seems a fair DT use to me, but RFC).

Note that AFAICT, by design, i2c-smbus module pretends to be an I2C slave
with address 0x0C (that is the alarm response address), and IMHO this is
quite consistent with usage in the DT as a I2C child node.

Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 Documentation/devicetree/bindings/i2c/i2c-smbus.txt | 20 ++++++++++++++++++++
 drivers/i2c/i2c-smbus.c                             | 20 +++++++++++++++-----
 2 files changed, 35 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-smbus.txt

Comments

Mark Rutland Sept. 21, 2016, 11:34 a.m.
On Wed, Sep 21, 2016 at 04:41:09PM +0800, Phil Reid wrote:
> From: Andrea Merello <andrea.merello@gmail.com>
> 
> According to Documentation/i2c/smbus-protocol, a smbus controller driver
> that wants to hook-in smbus extensions support, can call
> i2c_setup_smbus_alert(). There are very few drivers that are currently
> doing this.
> 
> However the i2c-smbus module can also work with any
> smbus-extensions-unaware I2C controller, as long as we provide an extra
> IRQ line connected to the I2C bus ALARM signal.
> 
> This patch makes it possible to go this way via DT. Note that the DT node
> will indeed describe a (little) piece of HW, that is the routing of the
> ALARM signal to an IRQ line (so it seems a fair DT use to me, but RFC).

Which piece of hardware actually generates this IRQ? The I2C controller?
A slave SMBus device? Or something else?

I'm not at all familiar with I2C or SMBus, and a quick scan of
Documentation/i2c/smbus-protocol, has left me none-the-wiser on that
front.

> Note that AFAICT, by design, i2c-smbus module pretends to be an I2C slave
> with address 0x0C (that is the alarm response address), and IMHO this is
> quite consistent with usage in the DT as a I2C child node.
> 
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-smbus.txt | 20 ++++++++++++++++++++
>  drivers/i2c/i2c-smbus.c                             | 20 +++++++++++++++-----
>  2 files changed, 35 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-smbus.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-smbus.txt b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
> new file mode 100644
> index 0000000..da83127
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
> @@ -0,0 +1,20 @@
> +* i2c-smbus extensions
> +
> +Required Properties:
> +  - compatible: Must contain "smbus_alert"

Nit: s/_/-/ in compatible strings please.

> +  - interrupts: The irq line for smbus ALERT signal
> +  - reg: I2C slave address. Set to 0x0C (alert response address).
> +
> +Note: The i2c-smbus module registers itself as a slave I2C device. Whenever
> +a smbus controller directly support smbus extensions (and its driver supports
> +this), there is no need to add anything special to the DT. Otherwise, for using
> +i2c-smbus with any smbus-extensions-unaware I2C controllers, you need to
> +route the smbus ALARM signal to an extra IRQ line, thus you need to describe
> +this in the DT.

Bindings shouldn't mention driver details (e.g. the i2c-smbus module
behaviour). It feels like we're creating a virtual device for the sake
of a driver, rather than accurately capturing the hardware.

So as-is, I don't think this is the right way to describe this.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Reid Sept. 22, 2016, 2:24 a.m.
G'day Mark

On 21/09/2016 19:34, Mark Rutland wrote:
> On Wed, Sep 21, 2016 at 04:41:09PM +0800, Phil Reid wrote:
>> From: Andrea Merello <andrea.merello@gmail.com>
>>
>> According to Documentation/i2c/smbus-protocol, a smbus controller driver
>> that wants to hook-in smbus extensions support, can call
>> i2c_setup_smbus_alert(). There are very few drivers that are currently
>> doing this.
>>
>> However the i2c-smbus module can also work with any
>> smbus-extensions-unaware I2C controller, as long as we provide an extra
>> IRQ line connected to the I2C bus ALARM signal.
>>
>> This patch makes it possible to go this way via DT. Note that the DT node
>> will indeed describe a (little) piece of HW, that is the routing of the
>> ALARM signal to an IRQ line (so it seems a fair DT use to me, but RFC).
>
> Which piece of hardware actually generates this IRQ? The I2C controller?
> A slave SMBus device? Or something else?
>
> I'm not at all familiar with I2C or SMBus, and a quick scan of
> Documentation/i2c/smbus-protocol, has left me none-the-wiser on that
> front.

The originating source is the smbus device. SMBALERT is an active low interrupt
with the addition that the SMBUS device can be polled by the controller to determine
which device is asserting by performing a read of i2c address 0xc.
Asserting devices will return their address.
See the datahsheet of the LTC1760: http://www.linear.com/docs/1958   Page 22

The i2c parport driver looks to be only controller setup for this at the moment.
However that has limitations to only poll the primary i2c segment.
If muxes are involved then each muxed bus needs to be queried.

My hardware has:

                 /- ltc1760
i2c_cntlr - mux +
                 \- ltc1760

The alert signal are routed to the mux (pca9543) irq inputs and then muxes combined
irq output to an fpga pin on an ALTERA SOC which I can route to a GPIO / IRQ

I've modified the pca954x driver to generate separate interrupts for each segment.
Submitted an RFC PATCH on this a while ago, but it needs a lot of cleaning up before
submission.

So I can create an virtual SMB_ALERT device per segment and it knows to just poll that segment.

>
>> Note that AFAICT, by design, i2c-smbus module pretends to be an I2C slave
>> with address 0x0C (that is the alarm response address), and IMHO this is
>> quite consistent with usage in the DT as a I2C child node.
>>
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>  Documentation/devicetree/bindings/i2c/i2c-smbus.txt | 20 ++++++++++++++++++++
>>  drivers/i2c/i2c-smbus.c                             | 20 +++++++++++++++-----
>>  2 files changed, 35 insertions(+), 5 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-smbus.txt
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-smbus.txt b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
>> new file mode 100644
>> index 0000000..da83127
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
>> @@ -0,0 +1,20 @@
>> +* i2c-smbus extensions
>> +
>> +Required Properties:
>> +  - compatible: Must contain "smbus_alert"
>
> Nit: s/_/-/ in compatible strings please.
>
>> +  - interrupts: The irq line for smbus ALERT signal
>> +  - reg: I2C slave address. Set to 0x0C (alert response address).
>> +
>> +Note: The i2c-smbus module registers itself as a slave I2C device. Whenever
>> +a smbus controller directly support smbus extensions (and its driver supports
>> +this), there is no need to add anything special to the DT. Otherwise, for using
>> +i2c-smbus with any smbus-extensions-unaware I2C controllers, you need to
>> +route the smbus ALARM signal to an extra IRQ line, thus you need to describe
>> +this in the DT.
>
> Bindings shouldn't mention driver details (e.g. the i2c-smbus module
> behaviour). It feels like we're creating a virtual device for the sake
> of a driver, rather than accurately capturing the hardware.
>
> So as-is, I don't think this is the right way to describe this.
>

Yes that seemed to be the discussion last time.

It's like a shared IRQ but has the extra feature of the polling to determine what device is active.
For my use case the polling is not necessary as there's only one smbalert device per segment.
So an irq would work, however that didn't seem to have much support either.

I don't know what direction to go, to get this functionality into the upstream source.
Rob Herring Sept. 23, 2016, 7:42 p.m.
On Wed, Sep 21, 2016 at 04:41:09PM +0800, Phil Reid wrote:
> From: Andrea Merello <andrea.merello@gmail.com>
> 
> According to Documentation/i2c/smbus-protocol, a smbus controller driver
> that wants to hook-in smbus extensions support, can call
> i2c_setup_smbus_alert(). There are very few drivers that are currently
> doing this.
> 
> However the i2c-smbus module can also work with any
> smbus-extensions-unaware I2C controller, as long as we provide an extra
> IRQ line connected to the I2C bus ALARM signal.
> 
> This patch makes it possible to go this way via DT. Note that the DT node
> will indeed describe a (little) piece of HW, that is the routing of the
> ALARM signal to an IRQ line (so it seems a fair DT use to me, but RFC).
> 
> Note that AFAICT, by design, i2c-smbus module pretends to be an I2C slave
> with address 0x0C (that is the alarm response address), and IMHO this is
> quite consistent with usage in the DT as a I2C child node.
> 
> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-smbus.txt | 20 ++++++++++++++++++++
>  drivers/i2c/i2c-smbus.c                             | 20 +++++++++++++++-----
>  2 files changed, 35 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-smbus.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-smbus.txt b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
> new file mode 100644
> index 0000000..da83127
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
> @@ -0,0 +1,20 @@
> +* i2c-smbus extensions
> +
> +Required Properties:
> +  - compatible: Must contain "smbus_alert"
> +  - interrupts: The irq line for smbus ALERT signal
> +  - reg: I2C slave address. Set to 0x0C (alert response address).

This is not right. The 0xC address is special, not an actual device 
address. The bindings should just have the actual device's compatible 
string and address.

> +
> +Note: The i2c-smbus module registers itself as a slave I2C device. Whenever
> +a smbus controller directly support smbus extensions (and its driver supports
> +this), there is no need to add anything special to the DT. Otherwise, for using
> +i2c-smbus with any smbus-extensions-unaware I2C controllers, you need to
> +route the smbus ALARM signal to an extra IRQ line, thus you need to describe
> +this in the DT.

Now, I guess what you need in the kernel is a common handler for 
SMBALERT# and to know which interrupt line is SMBALERT#.

The drivers should know this. A given h/w device will or will not handle 
the "SMB Alert Response Address". So the drivers should register their 
interrupt with the I2C/SMBus core.

If a controller handles the SMBALERT, then it should make itself an 
interrupt controller and that's what slave devices 'interrupts' property 
will point to.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Reid Sept. 30, 2016, 7:55 a.m.
G'day Rob,

On 24/09/2016 03:42, Rob Herring wrote:
> On Wed, Sep 21, 2016 at 04:41:09PM +0800, Phil Reid wrote:
>> From: Andrea Merello <andrea.merello@gmail.com>
>>
>> According to Documentation/i2c/smbus-protocol, a smbus controller driver
>> that wants to hook-in smbus extensions support, can call
>> i2c_setup_smbus_alert(). There are very few drivers that are currently
>> doing this.
>>
>> However the i2c-smbus module can also work with any
>> smbus-extensions-unaware I2C controller, as long as we provide an extra
>> IRQ line connected to the I2C bus ALARM signal.
>>
>> This patch makes it possible to go this way via DT. Note that the DT node
>> will indeed describe a (little) piece of HW, that is the routing of the
>> ALARM signal to an IRQ line (so it seems a fair DT use to me, but RFC).
>>
>> Note that AFAICT, by design, i2c-smbus module pretends to be an I2C slave
>> with address 0x0C (that is the alarm response address), and IMHO this is
>> quite consistent with usage in the DT as a I2C child node.
>>
>> Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>  Documentation/devicetree/bindings/i2c/i2c-smbus.txt | 20 ++++++++++++++++++++
>>  drivers/i2c/i2c-smbus.c                             | 20 +++++++++++++++-----
>>  2 files changed, 35 insertions(+), 5 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-smbus.txt
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-smbus.txt b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
>> new file mode 100644
>> index 0000000..da83127
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
>> @@ -0,0 +1,20 @@
>> +* i2c-smbus extensions
>> +
>> +Required Properties:
>> +  - compatible: Must contain "smbus_alert"
>> +  - interrupts: The irq line for smbus ALERT signal
>> +  - reg: I2C slave address. Set to 0x0C (alert response address).
>
> This is not right. The 0xC address is special, not an actual device
> address. The bindings should just have the actual device's compatible
> string and address.
>
>> +
>> +Note: The i2c-smbus module registers itself as a slave I2C device. Whenever
>> +a smbus controller directly support smbus extensions (and its driver supports
>> +this), there is no need to add anything special to the DT. Otherwise, for using
>> +i2c-smbus with any smbus-extensions-unaware I2C controllers, you need to
>> +route the smbus ALARM signal to an extra IRQ line, thus you need to describe
>> +this in the DT.
>
> Now, I guess what you need in the kernel is a common handler for
> SMBALERT# and to know which interrupt line is SMBALERT#.
>
> The drivers should know this. A given h/w device will or will not handle
> the "SMB Alert Response Address". So the drivers should register their
> interrupt with the I2C/SMBus core.
>
> If a controller handles the SMBALERT, then it should make itself an
> interrupt controller and that's what slave devices 'interrupts' property
> will point to.
>

Could a property be added to each i2c bus segment.
Something like the following.

   interrupt-parent = <&irqsrc>;
   interrupts = <0>;
   interrupt-names = "smbalert";

Then the i2c core code could look for this in each segment, either in the parent segment
or a mux segment and create the smbalert common handler for each segment as required.

Patch hide | download patch | download mbox

diff --git a/Documentation/devicetree/bindings/i2c/i2c-smbus.txt b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
new file mode 100644
index 0000000..da83127
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-smbus.txt
@@ -0,0 +1,20 @@ 
+* i2c-smbus extensions
+
+Required Properties:
+  - compatible: Must contain "smbus_alert"
+  - interrupts: The irq line for smbus ALERT signal
+  - reg: I2C slave address. Set to 0x0C (alert response address).
+
+Note: The i2c-smbus module registers itself as a slave I2C device. Whenever
+a smbus controller directly support smbus extensions (and its driver supports
+this), there is no need to add anything special to the DT. Otherwise, for using
+i2c-smbus with any smbus-extensions-unaware I2C controllers, you need to
+route the smbus ALARM signal to an extra IRQ line, thus you need to describe
+this in the DT.
+
+Example:
+	alert@0x0C {
+		reg = <0x0C>;
+		compatible = "smbus_alert";
+		interrupts = <0 36 IRQ_TYPE_LEVEL_LOW>;
+	};
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index b0d2679..5806db3 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -23,6 +23,7 @@ 
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/workqueue.h>
+#include <linux/of_irq.h>
 
 struct i2c_smbus_alert {
 	unsigned int		alert_edge_triggered:1;
@@ -139,20 +140,29 @@  static int smbalert_probe(struct i2c_client *ara,
 	struct i2c_smbus_alert_setup *setup = dev_get_platdata(&ara->dev);
 	struct i2c_smbus_alert *alert;
 	struct i2c_adapter *adapter = ara->adapter;
+	struct device_node *of_node = ara->dev.of_node;
 	int res;
+	int irq_type;
 
 	alert = devm_kzalloc(&ara->dev, sizeof(struct i2c_smbus_alert),
 			     GFP_KERNEL);
 	if (!alert)
 		return -ENOMEM;
 
-	alert->alert_edge_triggered = setup->alert_edge_triggered;
-	alert->irq = setup->irq;
+	if (setup) {
+		alert->alert_edge_triggered = setup->alert_edge_triggered;
+		alert->irq = setup->irq;
+	} else if (of_node) {
+		alert->irq = irq_of_parse_and_map(of_node, 0);
+		irq_type = irq_get_trigger_type(alert->irq);
+		alert->alert_edge_triggered = (irq_type & IRQ_TYPE_EDGE_BOTH);
+	}
+
 	INIT_WORK(&alert->alert, smbus_alert);
 	alert->ara = ara;
 
-	if (setup->irq > 0) {
-		res = devm_request_irq(&ara->dev, setup->irq, smbalert_irq,
+	if (alert->irq > 0) {
+		res = devm_request_irq(&ara->dev, alert->irq, smbalert_irq,
 				       0, "smbus_alert", alert);
 		if (res)
 			return res;
@@ -160,7 +170,7 @@  static int smbalert_probe(struct i2c_client *ara,
 
 	i2c_set_clientdata(ara, alert);
 	dev_info(&adapter->dev, "supports SMBALERT#, %s trigger\n",
-		 setup->alert_edge_triggered ? "edge" : "level");
+		 alert->alert_edge_triggered ? "edge" : "level");
 
 	return 0;
 }