| Message ID | 1460536591-12573-1-git-send-email-andrea.merello@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Apr 13, 2016 at 11:36 AM, Andrea Merello <andrea.merello@gmail.com> wrote: > 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. > --- a/drivers/i2c/i2c-smbus.c > +++ b/drivers/i2c/i2c-smbus.c > @@ -137,20 +138,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; Perhaps fwnode_handle ? > 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); > + }
On Wed, Apr 13, 2016 at 7:58 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Apr 13, 2016 at 11:36 AM, Andrea Merello > <andrea.merello@gmail.com> wrote: >> 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. > >> --- a/drivers/i2c/i2c-smbus.c >> +++ b/drivers/i2c/i2c-smbus.c > >> @@ -137,20 +138,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; > > Perhaps fwnode_handle ? Browsing the kernel tree it looks like using of_node is how almost all drivers do. Any specific reason to go for fwnode_handle here? Wouldn't this end up to just an extra is_of_node() and to_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); >> + } > > -- > With Best Regards, > Andy Shevchenko -- 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
On Wed, Apr 13, 2016 at 10:36:31AM +0200, Andrea Merello wrote: > 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> > > 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. Seems like you are designing the binding around how the module is currently designed and creating a virtual device that doesn't exist. A "smbalert-gpios" property in the controller node would be sufficient here. Alternatively, an interrupt and standardized interrupt-names value could be used. Another option would be add a boolean property to each child node smbalert capable and the childrens' interrupt properties would all be the shared interrupt. 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
On Thu, Apr 14, 2016 at 6:10 PM, Rob Herring <robh@kernel.org> wrote: > On Wed, Apr 13, 2016 at 10:36:31AM +0200, Andrea Merello wrote: >> 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> >> >> 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. > > Seems like you are designing the binding around how the module is > currently designed and creating a virtual device that doesn't exist. Yes, I actually stayed close to the original module design, but the current module design seems the result of reasonable rationale and it didn't appeared completely unreasonable for me even for DT.. Said that, my bindings didn't completely convinced me too, but while I agree that we have not a real I2C slave, I'm not completely convinced that there is no any real HW "device" to describe in the DT (see below). > A "smbalert-gpios" property in the controller node would be sufficient > here. Alternatively, an interrupt and standardized interrupt-names value > could be used. Hmm, IMHO this way is not closer to the real HW. We are adding an interrupt property inside the I2C controller node, but that's not how the HW really is: the I2C controller is completely unaware of it (and it isn't its driver that handles it). If the I2C controller had that wire, then its driver could hook the i2c-smbus module by itself, without any extra DT prop. I don't see the point here. IMHO it isn't definitely about the I2C controller. The smbus-alert thing here is exactly what the I2C controller has _not_ (and we are "emulating" it with external HW). > Another option would be add a boolean property to each child node > smbalert capable and the childrens' interrupt properties would all be > the shared interrupt. That's seems closer to the real HW, because at least the slave devices really have that wire.. However I'm unsure whether the boolean property is enough: maybe there is some I2C slave that has both a regular INT line (handled by the I2C slave driver) and a smbus ALERT line (handled by the common i2c-smbus module)... Adding another interrupt property in the slave node seems also not good to me, because the I2C slave drivers must be made aware of it (to get the right one). Maybe we could introduce a new property like "alert-interrupt" in the slave nodes, that is caught by common I2C layer code.. This could be nearly-OK to me, but there is a bunch of things because of which I'm still not 100% convinced that offloading this "smbalert" thing on every slave DT node is really right: - The alert line is not properly an interrupt. It's something that should go to the smalert pin of the I2C controller. But sometimes there is some HW (in my case a NOT and an OR logic, but it can be simply a wire) that _translates_ this signal to a interrupt signal. This can be as simple as a _connection_ but _logically_ this connection is changing the semantic. So I'd say we _have_ a (doesn't matter if it's as simple as a PCB track) piece of HW that is in charge of this (and to which our DT property may belongs).. (And you can see the the i2c-smbus module as its driver). - Let's consider this example: I change the I2C controller with one that has the ALERT pin and manages the i2c-smbus by itself.. So I will have to delete the "alert-interrupt" property from all slave nodes.. IMHO this smells like something not good, or at least weird, doesn't it ? - I admit that we often use the DT nodes to describe things that are not really about how a device _is_, rather are about how the device is connected (regular interrupts, for example). But at least in this case the driver for the device needs to know this information and it's reasonable to let it access from the device node (and none else needs this). In the i2c-smbus case, on the other hand, our property is not related to the slave device itself, it's maybe related to how the device is connected, but it's also about how the BUS _is_, and the slave's driver don't care about it. It's indeed a property that should be caught by the common I2C code (that will instantiate the i2c-smbus module). This IMHO could be a symptom that this property shouldn't be in the I2C slave nodes. I'd say I can't really see any strong point for which it has to stay in the slaves' DT nodes.. Does all this make any sense in your opinion? I'm thinking about a dedicated DT node for the i2c-smbus "extra hardware" with the interrupt property, and referenced by phandle in some way from the I2C bus... Andrea > 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
On Mon, Apr 18, 2016 at 8:35 AM, Andrea Merello <andrea.merello@gmail.com> wrote: > On Thu, Apr 14, 2016 at 6:10 PM, Rob Herring <robh@kernel.org> wrote: >> On Wed, Apr 13, 2016 at 10:36:31AM +0200, Andrea Merello wrote: >>> 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> >>> >>> 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. >> >> Seems like you are designing the binding around how the module is >> currently designed and creating a virtual device that doesn't exist. > > Yes, I actually stayed close to the original module design, but the current > module design seems the result of reasonable rationale and it didn't > appeared completely unreasonable for me even for DT.. Said that, my > bindings didn't completely convinced me too, but while I agree that we > have not a real I2C slave, I'm not completely convinced that there is no > any real HW "device" to describe in the DT (see below). > >> A "smbalert-gpios" property in the controller node would be sufficient >> here. Alternatively, an interrupt and standardized interrupt-names value >> could be used. > > Hmm, IMHO this way is not closer to the real HW. We are adding an > interrupt property inside the I2C controller node, but that's not how the > HW really is: the I2C controller is completely unaware of it (and it isn't > its driver that handles it). If the I2C controller had that wire, then its > driver could hook the i2c-smbus module by itself, without any extra DT > prop. > > I don't see the point here. IMHO it isn't definitely about the I2C controller. > The smbus-alert thing here is exactly what the I2C controller has _not_ > (and we are "emulating" it with external HW). Fair enough, I don't really disagree. There is some precedence doing things this way. SPI GPIO chip selects go in the SPI controller node. SD card detect GPIO goes in the SD host node. >> Another option would be add a boolean property to each child node >> smbalert capable and the childrens' interrupt properties would all be >> the shared interrupt. > > That's seems closer to the real HW, because at least the slave devices > really have that wire.. > > However I'm unsure whether the boolean property is enough: maybe > there is some I2C slave that has both a regular INT line (handled by the > I2C slave driver) and a smbus ALERT line (handled by the common > i2c-smbus module)... Adding another interrupt property in the slave node > seems also not good to me, because the I2C slave drivers must be made > aware of it (to get the right one). Maybe we could introduce a new > property like "alert-interrupt" in the slave nodes, that is caught by > common I2C layer code.. I could apply logic that having 2 signals is completely pointless as the whole point of ALERT is to save pins. But logic is often not applied to hardware design. If the device has 2 lines (IRQ and ALERT), then having the driver be aware of h/w properties such as that seems perfectly normal to me. I don't want to see "alert-interrupt" as a non-standard way to define an interrupt. Either it is an interrupt and we use the standard binding or it is not... IMO, it is an interrupt. > This could be nearly-OK to me, but there is a bunch of things because of > which I'm still not 100% convinced that offloading this "smbalert" thing on > every slave DT node is really right: > > - The alert line is not properly an interrupt. It's something that should > go to the smalert pin of the I2C controller. But sometimes there is some > HW (in my case a NOT and an OR logic, but it can be simply a wire) that > _translates_ this signal to a interrupt signal. This can be as simple as a > _connection_ but _logically_ this connection is changing the semantic. > So I'd say we _have_ a (doesn't matter if it's as simple as a PCB track) > piece of HW that is in charge of this (and to which our DT property may > belongs).. (And you can see the the i2c-smbus module as its driver). I don't really follow. My read of it is that it is simply a shared interrupt line with a standardized way to clear the interrupt. > - Let's consider this example: I change the I2C controller with one that has > the ALERT pin and manages the i2c-smbus by itself.. So I will have to > delete the "alert-interrupt" property from all slave nodes.. IMHO this smells > like something not good, or at least weird, doesn't it ? This is one reason to do things like my first suggestion if you want to avoid changing the child nodes. It is reasonable for child nodes to be different for a different controller and different h/w. The I2C core should see whether the I2C controller is managing the ALERT or not. If it is, then do nothing. If it is not, then the core needs to register the GPIO based handler. > - I admit that we often use the DT nodes to describe things that are > not really about how a device _is_, rather are about how the device is > connected (regular interrupts, for example). But at least in this case > the driver for the device needs to know this information and it's > reasonable to let it access from the device node (and none else needs > this). In the i2c-smbus case, on the other hand, our property is not related > to the slave device itself, it's maybe related to how the device is connected, > but it's also about how the BUS _is_, and the slave's driver don't care > about it. It's indeed a property that should be caught by the common I2C > code (that will instantiate the i2c-smbus module). This IMHO could be a > symptom that this property shouldn't be in the I2C slave nodes. I'd say I > can't really see any strong point for which it has to stay in the slaves' DT > nodes.. > > Does all this make any sense in your opinion? DT is ALL about how things are connected. That is what the parent-child relationships and phandles to other nodes convey. Primarily, only the compatible property conveys what the device is. The alert support is a property of the slaves though. There are only certain h/w devices that follow this acknowledgement protocol for the alert. > I'm thinking about a dedicated DT node for the i2c-smbus "extra > hardware" with the interrupt property, and referenced by phandle in some > way from the I2C bus... Sorry, I still don't agree. 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
On Mon, Apr 18, 2016 at 6:44 PM, Rob Herring <robh@kernel.org> wrote: > On Mon, Apr 18, 2016 at 8:35 AM, Andrea Merello > <andrea.merello@gmail.com> wrote: >> On Thu, Apr 14, 2016 at 6:10 PM, Rob Herring <robh@kernel.org> wrote: >>> On Wed, Apr 13, 2016 at 10:36:31AM +0200, Andrea Merello wrote: >>>> 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> >>>> >>>> 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. >>> >>> Seems like you are designing the binding around how the module is >>> currently designed and creating a virtual device that doesn't exist. >> >> Yes, I actually stayed close to the original module design, but the current >> module design seems the result of reasonable rationale and it didn't >> appeared completely unreasonable for me even for DT.. Said that, my >> bindings didn't completely convinced me too, but while I agree that we >> have not a real I2C slave, I'm not completely convinced that there is no >> any real HW "device" to describe in the DT (see below). >> >>> A "smbalert-gpios" property in the controller node would be sufficient >>> here. Alternatively, an interrupt and standardized interrupt-names value >>> could be used. >> >> Hmm, IMHO this way is not closer to the real HW. We are adding an >> interrupt property inside the I2C controller node, but that's not how the >> HW really is: the I2C controller is completely unaware of it (and it isn't >> its driver that handles it). If the I2C controller had that wire, then its >> driver could hook the i2c-smbus module by itself, without any extra DT >> prop. >> >> I don't see the point here. IMHO it isn't definitely about the I2C controller. >> The smbus-alert thing here is exactly what the I2C controller has _not_ >> (and we are "emulating" it with external HW). > > Fair enough, I don't really disagree. There is some precedence doing > things this way. SPI GPIO chip selects go in the SPI controller node. > SD card detect GPIO goes in the SD host node. Are they used inside the SD host driver itself, or by the MMC core ? >>> Another option would be add a boolean property to each child node >>> smbalert capable and the childrens' interrupt properties would all be >>> the shared interrupt. >> >> That's seems closer to the real HW, because at least the slave devices >> really have that wire.. >> >> However I'm unsure whether the boolean property is enough: maybe >> there is some I2C slave that has both a regular INT line (handled by the >> I2C slave driver) and a smbus ALERT line (handled by the common >> i2c-smbus module)... Adding another interrupt property in the slave node >> seems also not good to me, because the I2C slave drivers must be made >> aware of it (to get the right one). Maybe we could introduce a new >> property like "alert-interrupt" in the slave nodes, that is caught by >> common I2C layer code.. > > I could apply logic that having 2 signals is completely pointless as > the whole point of ALERT is to save pins. But logic is often not > applied to hardware design. If the device has 2 lines (IRQ and ALERT), > then having the driver be aware of h/w properties such as that seems > perfectly normal to me. > > I don't want to see "alert-interrupt" as a non-standard way to define > an interrupt. Either it is an interrupt and we use the standard > binding or it is not... IMO, it is an interrupt. > Hmm now I'm seeing you point. Yes, you are probably right: my whole point of thinking about the alert as something different than an interrupt is probably wrong.. >> This could be nearly-OK to me, but there is a bunch of things because of >> which I'm still not 100% convinced that offloading this "smbalert" thing on >> every slave DT node is really right: >> >> - The alert line is not properly an interrupt. It's something that should >> go to the smalert pin of the I2C controller. But sometimes there is some >> HW (in my case a NOT and an OR logic, but it can be simply a wire) that >> _translates_ this signal to a interrupt signal. This can be as simple as a >> _connection_ but _logically_ this connection is changing the semantic. >> So I'd say we _have_ a (doesn't matter if it's as simple as a PCB track) >> piece of HW that is in charge of this (and to which our DT property may >> belongs).. (And you can see the the i2c-smbus module as its driver). > > I don't really follow. My read of it is that it is simply a shared > interrupt line with a standardized way to clear the interrupt. > >> - Let's consider this example: I change the I2C controller with one that has >> the ALERT pin and manages the i2c-smbus by itself.. So I will have to >> delete the "alert-interrupt" property from all slave nodes.. IMHO this smells >> like something not good, or at least weird, doesn't it ? > > This is one reason to do things like my first suggestion if you want > to avoid changing the child nodes. It is reasonable for child nodes to > be different for a different controller and different h/w. > > The I2C core should see whether the I2C controller is managing the > ALERT or not. If it is, then do nothing. If it is not, then the core > needs to register the GPIO based handler. > >> - I admit that we often use the DT nodes to describe things that are >> not really about how a device _is_, rather are about how the device is >> connected (regular interrupts, for example). But at least in this case >> the driver for the device needs to know this information and it's >> reasonable to let it access from the device node (and none else needs >> this). In the i2c-smbus case, on the other hand, our property is not related >> to the slave device itself, it's maybe related to how the device is connected, >> but it's also about how the BUS _is_, and the slave's driver don't care >> about it. It's indeed a property that should be caught by the common I2C >> code (that will instantiate the i2c-smbus module). This IMHO could be a >> symptom that this property shouldn't be in the I2C slave nodes. I'd say I >> can't really see any strong point for which it has to stay in the slaves' DT >> nodes.. >> >> Does all this make any sense in your opinion? > > DT is ALL about how things are connected. That is what the > parent-child relationships and phandles to other nodes convey. > Primarily, only the compatible property conveys what the device is. > > The alert support is a property of the slaves though. There are only > certain h/w devices that follow this acknowledgement protocol for the > alert. That's indeed true. >> I'm thinking about a dedicated DT node for the i2c-smbus "extra >> hardware" with the interrupt property, and referenced by phandle in some >> way from the I2C bus... > > Sorry, I still don't agree. Ok, you almost convinced me. I'll probably come back with an updated patch that uses the bool prop in the slave nodes.. Unless in case that while I'm trying doing this I face something that makes me rethink the whole thing.. Andrea > 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
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 abb55d3..7f0a566 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; @@ -137,20 +138,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; @@ -158,7 +168,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; }
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> -- 1.9.1 -- 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