| Message ID | 1474447274-90821-2-git-send-email-preid@electromag.com.au |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
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
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.
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; }