| Message ID | 1469433003-68564-2-git-send-email-preid@electromag.com.au (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid wrote: > This patch added irq support for the SMBALERT pin and notification of > the battery removal / insertion. The sbs manager would typically be > used with the corresponding sbs-battery driver that currently uses a > gpio input for battery presence and interrupt. To remain compatible with > that existing driver this patch implements GPIO inputs with interrupt > support. IRQ masking is performed in software as the hardware does not > support masking of notifications from each battery. > In addition a power_supply change notification is generated for the sbs > manager device when the AC present flag is changed. > Tested with LTC1760 and dual sbs compatible batteries. Please don't submit a binding and immediately turn around and add to it. While that is often preferred for drivers or kernel features, bindings should be complete as possible and not evolve. Combine this with your previous patch add sbs-mgr if that hasn't been accepted yet. IMO, the sbs-bat should just be a interrupt and making it and this binding a GPIO is overkill. Since batteries nodes using this will be new, there's no reason the driver can't be updated to support interrupts. Also, I'm not even convinced you need the sbs-mgr to be an interrupt-controller. Is this anything more that just wire-OR'ed interrupt lines which can be handled as shared irq? Does reading SBSM_CMD_BATSYSSTATE have a side effect for example? Rob -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Mittwoch, den 27.07.2016, 09:46 -0500 schrieb Rob Herring: > On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid wrote: > > This patch added irq support for the SMBALERT pin and notification of > > the battery removal / insertion. The sbs manager would typically be > > used with the corresponding sbs-battery driver that currently uses a > > gpio input for battery presence and interrupt. To remain compatible with > > that existing driver this patch implements GPIO inputs with interrupt > > support. IRQ masking is performed in software as the hardware does not > > support masking of notifications from each battery. > > In addition a power_supply change notification is generated for the sbs > > manager device when the AC present flag is changed. > > Tested with LTC1760 and dual sbs compatible batteries. > > Please don't submit a binding and immediately turn around and add to it. > While that is often preferred for drivers or kernel features, bindings > should be complete as possible and not evolve. Combine this with your > previous patch add sbs-mgr if that hasn't been accepted yet. > > IMO, the sbs-bat should just be a interrupt and making it and this > binding a GPIO is overkill. Since batteries nodes using this will be > new, there's no reason the driver can't be updated to support > interrupts. > > Also, I'm not even convinced you need the sbs-mgr to be an > interrupt-controller. Is this anything more that just wire-OR'ed > interrupt lines which can be handled as shared irq? Does reading > SBSM_CMD_BATSYSSTATE have a side effect for example? Yes, according to the Datasheed it clears SMBALERT#. > > Rob SMBALERT is electrically an active low line which is pulled down by one or more devices needing attention, isn't it? The I2C host driver or some other instance need to implement support for it, the device driver may register some sort of handler function (e.g by implementing struct i2c_driver.alert function) That's my understanding of the SMBALERT. I once tried to implement the alert function and see if it get's called. It didn't. At that point I didn't investigate further. So, do I miss something or is that part incomplete/broken? I can only find lm90 and ipmi_ssif which implement the callback.
G'day Karl / Rob, On 28/07/2016 05:08, Karl-Heinz Schneider wrote: > Am Mittwoch, den 27.07.2016, 09:46 -0500 schrieb Rob Herring: >> On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid wrote: >>> This patch added irq support for the SMBALERT pin and notification of >>> the battery removal / insertion. The sbs manager would typically be >>> used with the corresponding sbs-battery driver that currently uses a >>> gpio input for battery presence and interrupt. To remain compatible with >>> that existing driver this patch implements GPIO inputs with interrupt >>> support. IRQ masking is performed in software as the hardware does not >>> support masking of notifications from each battery. >>> In addition a power_supply change notification is generated for the sbs >>> manager device when the AC present flag is changed. >>> Tested with LTC1760 and dual sbs compatible batteries. >> >> Please don't submit a binding and immediately turn around and add to it. >> While that is often preferred for drivers or kernel features, bindings >> should be complete as possible and not evolve. Combine this with your >> previous patch add sbs-mgr if that hasn't been accepted yet. That was from Karl. I'm happy to have this combined if he is. >> >> IMO, the sbs-bat should just be a interrupt and making it and this >> binding a GPIO is overkill. Since batteries nodes using this will be >> new, there's no reason the driver can't be updated to support >> interrupts. >> >> Also, I'm not even convinced you need the sbs-mgr to be an >> interrupt-controller. Is this anything more that just wire-OR'ed >> interrupt lines which can be handled as shared irq? Does reading >> SBSM_CMD_BATSYSSTATE have a side effect for example? > Yes, according to the Datasheed it clears SMBALERT#. Yes you need to service the Ltc1760 for the irq to clear. reading the batteries does nothing. Even without any batteries present you may get an irq and need to service it. eg: I have a system with 2 ltc1760 with a total of 4 batteries. So if only one battery is present and AC power is connected then both ltc1760 will generate a notification on SMBALERT. One which has no batteries present. >> >> Rob > SMBALERT is electrically an active low line which is pulled down by one > or more devices needing attention, isn't it? The I2C host driver or some > other instance need to implement support for it, the device driver may > register some sort of handler function (e.g by implementing struct > i2c_driver.alert function) > > That's my understanding of the SMBALERT. > > I once tried to implement the alert function and see if it get's called. > It didn't. At that point I didn't investigate further. > > So, do I miss something or is that part incomplete/broken? I can only > find lm90 and ipmi_ssif which implement the callback. > I wasn't aware of that functionality. Most devices seem to use a GPIO to obtain irq support. Which I guess is the most flexible solution. I think SMBALERT also support a mechanism to query all devices on the bus to determine which device address is asserting the irq. Not sure how this would work with multiplexed address buses (eg dual ltc1760) and devices with the same address. Open to suggestions
G'day Rob, Just another thought. On 28/07/2016 10:11, Phil Reid wrote: > G'day Karl / Rob, > > On 28/07/2016 05:08, Karl-Heinz Schneider wrote: >> Am Mittwoch, den 27.07.2016, 09:46 -0500 schrieb Rob Herring: >>> On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid wrote: >>>> This patch added irq support for the SMBALERT pin and notification of >>>> the battery removal / insertion. The sbs manager would typically be >>>> used with the corresponding sbs-battery driver that currently uses a >>>> gpio input for battery presence and interrupt. To remain compatible with >>>> that existing driver this patch implements GPIO inputs with interrupt >>>> support. IRQ masking is performed in software as the hardware does not >>>> support masking of notifications from each battery. >>>> In addition a power_supply change notification is generated for the sbs >>>> manager device when the AC present flag is changed. >>>> Tested with LTC1760 and dual sbs compatible batteries. >>> >>> Please don't submit a binding and immediately turn around and add to it. >>> While that is often preferred for drivers or kernel features, bindings >>> should be complete as possible and not evolve. Combine this with your >>> previous patch add sbs-mgr if that hasn't been accepted yet. > That was from Karl. I'm happy to have this combined if he is. > >>> >>> IMO, the sbs-bat should just be a interrupt and making it and this >>> binding a GPIO is overkill. Since batteries nodes using this will be >>> new, there's no reason the driver can't be updated to support >>> interrupts. I did consider this and didn't go that way as: 1) the current sbs-battery driver didn't support it. 2) Then there's also the problem of reporting the presence of the battery to the battery driver. Without the GPIO support you resort to polling and looking for a NACK. Which didn't seem that nice. >>> >>> Also, I'm not even convinced you need the sbs-mgr to be an >>> interrupt-controller. Is this anything more that just wire-OR'ed >>> interrupt lines which can be handled as shared irq? Does reading >>> SBSM_CMD_BATSYSSTATE have a side effect for example? >> Yes, according to the Datasheed it clears SMBALERT#. > > Yes you need to service the Ltc1760 for the irq to clear. reading the batteries does nothing. > Even without any batteries present you may get an irq and need to service it. > eg: I have a system with 2 ltc1760 with a total of 4 batteries. > So if only one battery is present and AC power is connected then both ltc1760 will generate > a notification on SMBALERT. One which has no batteries present. > >>> >>> Rob >> SMBALERT is electrically an active low line which is pulled down by one >> or more devices needing attention, isn't it? The I2C host driver or some >> other instance need to implement support for it, the device driver may >> register some sort of handler function (e.g by implementing struct >> i2c_driver.alert function) >> >> That's my understanding of the SMBALERT. >> >> I once tried to implement the alert function and see if it get's called. >> It didn't. At that point I didn't investigate further. >> >> So, do I miss something or is that part incomplete/broken? I can only >> find lm90 and ipmi_ssif which implement the callback. >> > I wasn't aware of that functionality. Most devices seem to use a GPIO to > obtain irq support. Which I guess is the most flexible solution. > I think SMBALERT also support a mechanism to query all devices on the bus to determine > which device address is asserting the irq. Not sure how this would work with multiplexed > address buses (eg dual ltc1760) and devices with the same address. > > Open to suggestions >
On Thu, Jul 28, 2016 at 4:01 AM, Phil Reid <preid@electromag.com.au> wrote: > G'day Rob, > > Just another thought. > > On 28/07/2016 10:11, Phil Reid wrote: >> >> G'day Karl / Rob, >> >> On 28/07/2016 05:08, Karl-Heinz Schneider wrote: >>> >>> Am Mittwoch, den 27.07.2016, 09:46 -0500 schrieb Rob Herring: >>>> >>>> On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid wrote: >>>>> >>>>> This patch added irq support for the SMBALERT pin and notification of >>>>> the battery removal / insertion. The sbs manager would typically be >>>>> used with the corresponding sbs-battery driver that currently uses a >>>>> gpio input for battery presence and interrupt. To remain compatible >>>>> with >>>>> that existing driver this patch implements GPIO inputs with interrupt >>>>> support. IRQ masking is performed in software as the hardware does not >>>>> support masking of notifications from each battery. >>>>> In addition a power_supply change notification is generated for the sbs >>>>> manager device when the AC present flag is changed. >>>>> Tested with LTC1760 and dual sbs compatible batteries. >>>> >>>> >>>> Please don't submit a binding and immediately turn around and add to it. >>>> While that is often preferred for drivers or kernel features, bindings >>>> should be complete as possible and not evolve. Combine this with your >>>> previous patch add sbs-mgr if that hasn't been accepted yet. >> >> That was from Karl. I'm happy to have this combined if he is. Indeed it was. Still, it should be combined. You can leave the driver implementation as multiple patches, just combine all the binding to a single patch by itself. >>>> IMO, the sbs-bat should just be a interrupt and making it and this >>>> binding a GPIO is overkill. Since batteries nodes using this will be >>>> new, there's no reason the driver can't be updated to support >>>> interrupts. > > I did consider this and didn't go that way as: > 1) the current sbs-battery driver didn't support it. > 2) Then there's also the problem of reporting the presence of the battery > to the battery driver. > Without the GPIO support you resort to polling and looking for a NACK. Which > didn't seem that nice. The ability to read the current irq signal state with the irq api would be nice. Of course you can't really count on being able to on all irqs. Anyway, we shouldn't really design the binding around kernel limitations. As I understand how SMBALERT works, you have to poll the devices anyway to determine which device is asserting the irq and to ack the irq. Of course, here you are doing point to point connections, but you still need to know which batteries are present by reading the sbs-mgr registers. Wouldn't switching the driver to level irq allow you to get the state immediately without a gpio read? After all, the gpio read and irq handler are reading the same register. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Donnerstag, den 28.07.2016, 10:45 -0500 schrieb Rob Herring: > On Thu, Jul 28, 2016 at 4:01 AM, Phil Reid <preid@electromag.com.au> > wrote: > > > > G'day Rob, > > > > Just another thought. > > > > On 28/07/2016 10:11, Phil Reid wrote: > > > > > > > > > G'day Karl / Rob, > > > > > > On 28/07/2016 05:08, Karl-Heinz Schneider wrote: > > > > > > > > > > > > Am Mittwoch, den 27.07.2016, 09:46 -0500 schrieb Rob Herring: > > > > > > > > > > > > > > > On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid wrote: > > > > > > > > > > > > > > > > > > This patch added irq support for the SMBALERT pin and > > > > > > notification of > > > > > > the battery removal / insertion. The sbs manager would > > > > > > typically be > > > > > > used with the corresponding sbs-battery driver that > > > > > > currently uses a > > > > > > gpio input for battery presence and interrupt. To remain > > > > > > compatible > > > > > > with > > > > > > that existing driver this patch implements GPIO inputs with > > > > > > interrupt > > > > > > support. IRQ masking is performed in software as the > > > > > > hardware does not > > > > > > support masking of notifications from each battery. > > > > > > In addition a power_supply change notification is generated > > > > > > for the sbs > > > > > > manager device when the AC present flag is changed. > > > > > > Tested with LTC1760 and dual sbs compatible batteries. > > > > > > > > > > Please don't submit a binding and immediately turn around and > > > > > add to it. > > > > > While that is often preferred for drivers or kernel features, > > > > > bindings > > > > > should be complete as possible and not evolve. Combine this > > > > > with your > > > > > previous patch add sbs-mgr if that hasn't been accepted yet. > > > That was from Karl. I'm happy to have this combined if he is.Me > Indeed it was. Still, it should be combined. You can leave the driver > implementation as multiple patches, just combine all the binding to a > single patch by itself. Will do that. > > > > > > > > > > > > > > > > > > > IMO, the sbs-bat should just be a interrupt and making it and > > > > > this > > > > > binding a GPIO is overkill. Since batteries nodes using this > > > > > will be > > > > > new, there's no reason the driver can't be updated to support > > > > > interrupts. > > I did consider this and didn't go that way as: > > 1) the current sbs-battery driver didn't support it. > > 2) Then there's also the problem of reporting the presence of the > > battery > > to the battery driver. > > Without the GPIO support you resort to polling and looking for a > > NACK. Which > > didn't seem that nice. > The ability to read the current irq signal state with the irq api > would be nice. Of course you can't really count on being able to on > all irqs. Anyway, we shouldn't really design the binding around > kernel > limitations. > > As I understand how SMBALERT works, you have to poll the devices > anyway to determine which device is asserting the irq and to ack the > irq. Of course, here you are doing point to point connections, but > you > still need to know which batteries are present by reading the sbs-mgr > registers. The more interesting point for me is, that SMBALERT seems to provide an easy to use driver side interface. The code Registering the IRQ would vanish... Anyways I'm not very familiar with Linux IRQ handling, so I will follow your advise. > > Wouldn't switching the driver to level irq allow you to get the state > immediately without a gpio read? After all, the gpio read and irq > handler are reading the same register. > > Rob
On 31/07/2016 22:04, Karl-Heinz Schneider wrote: > Am Donnerstag, den 28.07.2016, 10:45 -0500 schrieb Rob Herring: >> On Thu, Jul 28, 2016 at 4:01 AM, Phil Reid <preid@electromag.com.au> >> wrote: >>> On 28/07/2016 10:11, Phil Reid wrote: >>>> G'day Karl / Rob, >>>> On 28/07/2016 05:08, Karl-Heinz Schneider wrote: >>>>> Am Mittwoch, den 27.07.2016, 09:46 -0500 schrieb Rob Herring: >>>>>> On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid wrote: >>>>>>> This patch added irq support for the SMBALERT pin and >>>>>>> notification of >>>>>>> the battery removal / insertion. The sbs manager would >>>>>>> typically be >>>>>>> used with the corresponding sbs-battery driver that >>>>>>> currently uses a >>>>>>> gpio input for battery presence and interrupt. To remain >>>>>>> compatible >>>>>>> with >>>>>>> that existing driver this patch implements GPIO inputs with >>>>>>> interrupt >>>>>>> support. IRQ masking is performed in software as the >>>>>>> hardware does not >>>>>>> support masking of notifications from each battery. >>>>>>> In addition a power_supply change notification is generated >>>>>>> for the sbs >>>>>>> manager device when the AC present flag is changed. >>>>>>> Tested with LTC1760 and dual sbs compatible batteries. >>>>>> >>>>>> Please don't submit a binding and immediately turn around and >>>>>> add to it. >>>>>> While that is often preferred for drivers or kernel features, >>>>>> bindings >>>>>> should be complete as possible and not evolve. Combine this >>>>>> with your >>>>>> previous patch add sbs-mgr if that hasn't been accepted yet. >>>> That was from Karl. I'm happy to have this combined if he is.Me >> Indeed it was. Still, it should be combined. You can leave the driver >> implementation as multiple patches, just combine all the binding to a >> single patch by itself. > Will do that. >>>>>> IMO, the sbs-bat should just be a interrupt and making it and >>>>>> this >>>>>> binding a GPIO is overkill. Since batteries nodes using this >>>>>> will be >>>>>> new, there's no reason the driver can't be updated to support >>>>>> interrupts. >>> I did consider this and didn't go that way as: >>> 1) the current sbs-battery driver didn't support it. >>> 2) Then there's also the problem of reporting the presence of the >>> battery >>> to the battery driver. >>> Without the GPIO support you resort to polling and looking for a >>> NACK. Which >>> didn't seem that nice. >> The ability to read the current irq signal state with the irq api >> would be nice. Of course you can't really count on being able to on >> all irqs. Anyway, we shouldn't really design the binding around >> kernel >> limitations. >> >> As I understand how SMBALERT works, you have to poll the devices >> anyway to determine which device is asserting the irq and to ack the >> irq. Of course, here you are doing point to point connections, but >> you >> still need to know which batteries are present by reading the sbs-mgr >> registers. > The more interesting point for me is, that SMBALERT seems to provide an > easy to use driver side interface. The code Registering the IRQ would > vanish... > Anyways I'm not very familiar with Linux IRQ handling, so I will follow > your advise. Just had a look at driver/i2c/i2c-smbus.c This appears to be a generic driver to handle smbalert. Currently used by the i2c-parport driver. Doesn't look to support device trees at the moment thou. Patch series "RFC: I2C: i2c-smbus: add device tree support" from Andrea Merello looked to be adding generic support for it. https://lkml.org/lkml/2016/4/13/157 Not sure where that's got to thou. >> >> Wouldn't switching the driver to level irq allow you to get the state >> immediately without a gpio read? After all, the gpio read and irq >> handler are reading the same register. If it's asserted continuously how do you stop the irq firing all the time? I guess you could register a high and low level irq and disable them alternatively. Unless I'm missing something. Keeping the gpio functionality also allows the battery detect to work without the irq connected to the sbsm thru polling.
Hi Phil, Hi Rob, Am Montag, den 01.08.2016, 14:52 +0800 schrieb Phil Reid: > On 31/07/2016 22:04, Karl-Heinz Schneider wrote: > > > > Am Donnerstag, den 28.07.2016, 10:45 -0500 schrieb Rob Herring: > > > > > > On Thu, Jul 28, 2016 at 4:01 AM, Phil Reid <preid@electromag.com. > > > au> > > > wrote: > > > > > > > > On 28/07/2016 10:11, Phil Reid wrote: > > > > > > > > > > G'day Karl / Rob, > > > > > On 28/07/2016 05:08, Karl-Heinz Schneider wrote: > > > > > > > > > > > > Am Mittwoch, den 27.07.2016, 09:46 -0500 schrieb Rob > > > > > > Herring: > > > > > > > > > > > > > > On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid > > > > > > > wrote: > > > > > > > > > > > > > > > > This patch added irq support for the SMBALERT pin and > > > > > > > > notification of > > > > > > > > the battery removal / insertion. The sbs manager would > > > > > > > > typically be > > > > > > > > used with the corresponding sbs-battery driver that > > > > > > > > currently uses a > > > > > > > > gpio input for battery presence and interrupt. To > > > > > > > > remain > > > > > > > > compatible > > > > > > > > with > > > > > > > > that existing driver this patch implements GPIO inputs > > > > > > > > with > > > > > > > > interrupt > > > > > > > > support. IRQ masking is performed in software as the > > > > > > > > hardware does not > > > > > > > > support masking of notifications from each battery. > > > > > > > > In addition a power_supply change notification is > > > > > > > > generated > > > > > > > > for the sbs > > > > > > > > manager device when the AC present flag is changed. > > > > > > > > Tested with LTC1760 and dual sbs compatible batteries. > > > > > > > Please don't submit a binding and immediately turn around > > > > > > > and > > > > > > > add to it. > > > > > > > While that is often preferred for drivers or kernel > > > > > > > features, > > > > > > > bindings > > > > > > > should be complete as possible and not evolve. Combine > > > > > > > this > > > > > > > with your > > > > > > > previous patch add sbs-mgr if that hasn't been accepted > > > > > > > yet. > > > > > That was from Karl. I'm happy to have this combined if he > > > > > is.Me > > > Indeed it was. Still, it should be combined. You can leave the > > > driver > > > implementation as multiple patches, just combine all the binding > > > to a > > > single patch by itself. > > Will do that. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > IMO, the sbs-bat should just be a interrupt and making it > > > > > > > and > > > > > > > this > > > > > > > binding a GPIO is overkill. Since batteries nodes using > > > > > > > this > > > > > > > will be > > > > > > > new, there's no reason the driver can't be updated to > > > > > > > support > > > > > > > interrupts. > > > > I did consider this and didn't go that way as: > > > > 1) the current sbs-battery driver didn't support it. > > > > 2) Then there's also the problem of reporting the presence of > > > > the > > > > battery > > > > to the battery driver. > > > > Without the GPIO support you resort to polling and looking for > > > > a > > > > NACK. Which > > > > didn't seem that nice. > > > The ability to read the current irq signal state with the irq api > > > would be nice. Of course you can't really count on being able to > > > on > > > all irqs. Anyway, we shouldn't really design the binding around > > > kernel > > > limitations. > > > > > > As I understand how SMBALERT works, you have to poll the devices > > > anyway to determine which device is asserting the irq and to ack > > > the > > > irq. Of course, here you are doing point to point connections, > > > but > > > you > > > still need to know which batteries are present by reading the > > > sbs-mgr > > > registers. > > The more interesting point for me is, that SMBALERT seems to > > provide an > > easy to use driver side interface. The code Registering the IRQ > > would > > vanish... > > Anyways I'm not very familiar with Linux IRQ handling, so I will > > follow > > your advise. > Just had a look at driver/i2c/i2c-smbus.c > This appears to be a generic driver to handle smbalert. Currently > used by the i2c-parport driver. > Doesn't look to support device trees at the moment thou. > Patch series "RFC: I2C: i2c-smbus: add device tree support" from > Andrea Merello looked to be adding generic support for it. > https://lkml.org/lkml/2016/4/13/157 > Not sure where that's got to thou. Did see this one. > > > > > > > > > > > > Wouldn't switching the driver to level irq allow you to get the > > > state > > > immediately without a gpio read? After all, the gpio read and irq > > > handler are reading the same register. > If it's asserted continuously how do you stop the irq firing all the > time? > I guess you could register a high and low level irq and disable them > alternatively. > Unless I'm missing something. > > Keeping the gpio functionality also allows the battery detect to work > without the irq > connected to the sbsm thru polling. > So what to do now? I can simply take Phil's patch to add IRQ and GPIO support to the sbsm driver. It needs some fixups though, at least Kconfig things. IMHO adding irq support adds complexity to the driver as well as to the device tree binding, which shouldn't be necessary. Could also be true for the GPIO part, though (sbsm could issue a call to .notify() for it's slaves). So anyway. Will take Phils patch and resubmit the sbs-manager patch series if making the sbsm an IRQ and GPIO controller is really desired.
On 24/08/2016 03:58, Karl-Heinz Schneider wrote: > Hi Phil, Hi Rob, > > Am Montag, den 01.08.2016, 14:52 +0800 schrieb Phil Reid: >> On 31/07/2016 22:04, Karl-Heinz Schneider wrote: >>> >>> Am Donnerstag, den 28.07.2016, 10:45 -0500 schrieb Rob Herring: >>>> >>>> On Thu, Jul 28, 2016 at 4:01 AM, Phil Reid <preid@electromag.com. >>>> au> >>>> wrote: >>>>> >>>>> On 28/07/2016 10:11, Phil Reid wrote: >>>>>> >>>>>> G'day Karl / Rob, >>>>>> On 28/07/2016 05:08, Karl-Heinz Schneider wrote: >>>>>>> >>>>>>> Am Mittwoch, den 27.07.2016, 09:46 -0500 schrieb Rob >>>>>>> Herring: >>>>>>>> >>>>>>>> On Mon, Jul 25, 2016 at 03:50:03PM +0800, Phil Reid >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> This patch added irq support for the SMBALERT pin and >>>>>>>>> notification of >>>>>>>>> the battery removal / insertion. The sbs manager would >>>>>>>>> typically be >>>>>>>>> used with the corresponding sbs-battery driver that >>>>>>>>> currently uses a >>>>>>>>> gpio input for battery presence and interrupt. To >>>>>>>>> remain >>>>>>>>> compatible >>>>>>>>> with >>>>>>>>> that existing driver this patch implements GPIO inputs >>>>>>>>> with >>>>>>>>> interrupt >>>>>>>>> support. IRQ masking is performed in software as the >>>>>>>>> hardware does not >>>>>>>>> support masking of notifications from each battery. >>>>>>>>> In addition a power_supply change notification is >>>>>>>>> generated >>>>>>>>> for the sbs >>>>>>>>> manager device when the AC present flag is changed. >>>>>>>>> Tested with LTC1760 and dual sbs compatible batteries. >>>>>>>> Please don't submit a binding and immediately turn around >>>>>>>> and >>>>>>>> add to it. >>>>>>>> While that is often preferred for drivers or kernel >>>>>>>> features, >>>>>>>> bindings >>>>>>>> should be complete as possible and not evolve. Combine >>>>>>>> this >>>>>>>> with your >>>>>>>> previous patch add sbs-mgr if that hasn't been accepted >>>>>>>> yet. >>>>>> That was from Karl. I'm happy to have this combined if he >>>>>> is.Me >>>> Indeed it was. Still, it should be combined. You can leave the >>>> driver >>>> implementation as multiple patches, just combine all the binding >>>> to a >>>> single patch by itself. >>> Will do that. >>>> >>>>> >>>>>> >>>>>>> >>>>>>>> >>>>>>>> IMO, the sbs-bat should just be a interrupt and making it >>>>>>>> and >>>>>>>> this >>>>>>>> binding a GPIO is overkill. Since batteries nodes using >>>>>>>> this >>>>>>>> will be >>>>>>>> new, there's no reason the driver can't be updated to >>>>>>>> support >>>>>>>> interrupts. >>>>> I did consider this and didn't go that way as: >>>>> 1) the current sbs-battery driver didn't support it. >>>>> 2) Then there's also the problem of reporting the presence of >>>>> the >>>>> battery >>>>> to the battery driver. >>>>> Without the GPIO support you resort to polling and looking for >>>>> a >>>>> NACK. Which >>>>> didn't seem that nice. >>>> The ability to read the current irq signal state with the irq api >>>> would be nice. Of course you can't really count on being able to >>>> on >>>> all irqs. Anyway, we shouldn't really design the binding around >>>> kernel >>>> limitations. >>>> >>>> As I understand how SMBALERT works, you have to poll the devices >>>> anyway to determine which device is asserting the irq and to ack >>>> the >>>> irq. Of course, here you are doing point to point connections, >>>> but >>>> you >>>> still need to know which batteries are present by reading the >>>> sbs-mgr >>>> registers. >>> The more interesting point for me is, that SMBALERT seems to >>> provide an >>> easy to use driver side interface. The code Registering the IRQ >>> would >>> vanish... >>> Anyways I'm not very familiar with Linux IRQ handling, so I will >>> follow >>> your advise. >> Just had a look at driver/i2c/i2c-smbus.c >> This appears to be a generic driver to handle smbalert. Currently >> used by the i2c-parport driver. >> Doesn't look to support device trees at the moment thou. >> Patch series "RFC: I2C: i2c-smbus: add device tree support" from >> Andrea Merello looked to be adding generic support for it. >> https://lkml.org/lkml/2016/4/13/157 >> Not sure where that's got to thou. > Did see this one. >> >>> >>>> >>>> >>>> Wouldn't switching the driver to level irq allow you to get the >>>> state >>>> immediately without a gpio read? After all, the gpio read and irq >>>> handler are reading the same register. >> If it's asserted continuously how do you stop the irq firing all the >> time? >> I guess you could register a high and low level irq and disable them >> alternatively. >> Unless I'm missing something. >> >> Keeping the gpio functionality also allows the battery detect to work >> without the irq >> connected to the sbsm thru polling. >> > So what to do now? I can simply take Phil's patch to add IRQ and GPIO > support to the sbsm driver. It needs some fixups though, at least > Kconfig things. > > IMHO adding irq support adds complexity to the driver as well as to the > device tree binding, which shouldn't be necessary. Could also be true > for the GPIO part, though (sbsm could issue a call to .notify() for > it's slaves). > > So anyway. Will take Phils patch and resubmit the sbs-manager patch > series if making the sbsm an IRQ and GPIO controller is really desired. > G'day Karl, I'm planing on looking at the .notify() solution for sbsm. Having the sbsm trigger an .notify() is an interesting solution. It could certainly reduce a fair bit of my code and probably address the biggest concern with the device tree. I'm all for getting the driver submitted as you have it and I'll sort something out later. I'm probably a couple of weeks away from getting time to look at this again.
diff --git a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt index f0e2253..624967e 100644 --- a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt @@ -3,6 +3,26 @@ Binding for sbs-manager Required properties: - compatible: should be "lltc,ltc1760" or use "sbs,sbs-manager" as fallback. - reg: integer, i2c address of the device. Should be <0xa>. +Optional properties: +- gpio-controller: Marks the port as GPIO controller. + See "gpio-specifier" in .../devicetree/bindings/gpio/gpio.txt. +- #gpio-cells: Should be <2>. The first cell is the pin number, the second cell + is used to specify optional parameters: + See "gpio-specifier" in .../devicetree/bindings/gpio/gpio.txt. +- interrupt-parent: Phandle for the interrupt controller that services + interrupts for this device. + See also .../devicetree/bindings/interrupt-controller/interrupts.txt. +- interrupts: Interrupt mapping for SMBALERT IRQ. + See also .../devicetree/bindings/interrupt-controller/interrupts.txt. +- #interrupt-cells: Should be <2>. The first cell is the GPIO number. The + second cell is used to specify flags. The following subset of flags is + supported: + - trigger type (bits[1:0]): + 3 = low-to-high or high-to-low edge triggered + Valid value is 3 + See also .../devicetree/bindings/interrupt-controller/interrupts.txt. +- interrupt-controller: Marks the device node as an interrupt controller. + See also .../devicetree/bindings/interrupt-controller/interrupts.txt. From OS view the device is basically an i2c-mux used to communicate with up to four smart battery devices at address 0xb. The driver actually implements this @@ -17,6 +37,15 @@ batman@0a { #address-cells = <1>; #size-cells = <0>; + gpio-controller; + #gpio-cells = <2>; + + interrupt-controller; + #interrupt-cells=<2>; + interrupt-parent = <&parent_irq>; + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; + interrupts-names = "irq"; + i2c@1 { #address-cells = <1>; #size-cells = <0>; diff --git a/drivers/power/sbs-manager.c b/drivers/power/sbs-manager.c index adf9e41..3911329 100644 --- a/drivers/power/sbs-manager.c +++ b/drivers/power/sbs-manager.c @@ -20,6 +20,8 @@ #include <linux/i2c.h> #include <linux/i2c-mux.h> #include <linux/power_supply.h> +#include <linux/gpio.h> +#include <linux/interrupt.h> #define SBSM_MAX_BATS 4 #define SBSM_RETRY_CNT 3 @@ -30,14 +32,22 @@ #define SBSM_CMD_BATSYSINFO 0x04 #define SBSM_CMD_LTC 0x3c +#define SBSM_BIT_AC_PRESENT BIT(0) + struct sbsm_data { struct i2c_client *client; struct i2c_mux_core *muxc; struct power_supply *psy; + struct gpio_chip chip; + int cur_chan; /* currently selected channel */ bool is_ltc1760; /* special capabilities */ + + unsigned int irq_mask; + unsigned int last_state; + unsigned int last_state_cont; }; static enum power_supply_property sbsm_props[] = { @@ -206,6 +216,149 @@ static const struct power_supply_desc sbsm_default_psy_desc = { .property_is_writeable = &sbsm_prop_is_writeable, }; +static int sbsm_gpio_get_value(struct gpio_chip *gc, unsigned off) +{ + struct sbsm_data *data = gpiochip_get_data(gc); + int ret; + + ret = sbsm_read_word(data->client, SBSM_CMD_BATSYSSTATE); + if (ret < 0) + return ret; + + return ret & BIT(off); +} + +/* + * This needs to be defined or the GPIO lib fails to register the pin. + * But the 'gpio' is always an input. + */ +static int sbsm_gpio_direction_input(struct gpio_chip *gc, unsigned off) +{ + return 0; +} + +static irqreturn_t sbsm_irq(int irq, void *data) +{ + struct sbsm_data *sbsm = data; + int ret, i, nhandled = 0, child_irq = 0, irq_bat = 0; + + ret = sbsm_read_word(sbsm->client, SBSM_CMD_BATSYSSTATE); + if (ret >= 0) + irq_bat = (ret ^ sbsm->last_state) & sbsm->irq_mask; + sbsm->last_state = ret; + + ret = sbsm_read_word(sbsm->client, SBSM_CMD_BATSYSSTATECONT); + if ((ret >= 0) && + ((ret ^ sbsm->last_state_cont) & SBSM_BIT_AC_PRESENT)) { + irq_bat |= sbsm->irq_mask; + power_supply_changed(sbsm->psy); + } + sbsm->last_state_cont = ret; + + for (i = 0; i < SBSM_MAX_BATS; i++) { + if (irq_bat & BIT(i)) { + child_irq = irq_find_mapping(sbsm->chip.irqdomain, i); + handle_nested_irq(child_irq); + nhandled++; + } + } + + return (nhandled > 0) ? IRQ_HANDLED : IRQ_NONE; +} + +static void sbsm_irq_mask(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct sbsm_data *sbsm = gpiochip_get_data(gc); + unsigned int pos = data->hwirq; + + sbsm->irq_mask &= ~BIT(pos); +} + +static void sbsm_irq_unmask(struct irq_data *data) +{ + struct gpio_chip *gc = irq_data_get_irq_chip_data(data); + struct sbsm_data *sbsm = gpiochip_get_data(gc); + unsigned int pos = data->hwirq; + + sbsm->irq_mask |= BIT(pos); +} + +static int sbsm_irq_set_type(struct irq_data *data, unsigned int type) +{ + if ((type & IRQ_TYPE_EDGE_BOTH) == IRQ_TYPE_EDGE_BOTH) + return 0; + + return -EINVAL; +} + +static struct irq_chip sbsm_irq_chip = { + .name = "sbs-manager", + .irq_mask = sbsm_irq_mask, + .irq_unmask = sbsm_irq_unmask, + .irq_set_type = sbsm_irq_set_type, +}; + +static int sbsm_gpio_setup(struct sbsm_data *data) +{ + struct gpio_chip *gc = &data->chip; + struct i2c_client *client = data->client; + struct device *dev = &client->dev; + struct device_node *of_node = client->dev.of_node; + unsigned long irqflags = IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_HIGH; + int irq = client->irq; + int ret; + + if (!of_get_property(of_node, "gpio-controller", NULL)) + return 0; + + ret = sbsm_read_word(client, SBSM_CMD_BATSYSSTATE); + if (ret < 0) + return ret; + data->last_state = ret; + + ret = sbsm_read_word(client, SBSM_CMD_BATSYSSTATECONT); + if (ret < 0) + return ret; + data->last_state_cont = ret; + + gc->get = sbsm_gpio_get_value; + gc->direction_input = sbsm_gpio_direction_input; + gc->can_sleep = true; + gc->base = -1; + gc->ngpio = SBSM_MAX_BATS; + gc->label = client->name; + gc->parent = dev; + gc->owner = THIS_MODULE; + + ret = devm_gpiochip_add_data(dev, gc, data); + if (ret) { + dev_err(dev, "devm_gpiochip_add_data failed: %d\n", ret); + return ret; + } + + if (irq < 0) + return 0; + + ret = devm_request_threaded_irq(dev, irq, NULL, sbsm_irq, + irqflags, dev_name(dev), data); + if (ret != 0) { + dev_err(dev, "unable to request IRQ%d: %d\n", irq, ret); + return ret; + } + + ret = gpiochip_irqchip_add(gc, &sbsm_irq_chip, 0, handle_simple_irq, + IRQ_TYPE_NONE); + if (ret) { + dev_err(dev, "gpiochip_irqchip_add failed: %d\n", ret); + return ret; + } + + gpiochip_set_chained_irqchip(gc, &sbsm_irq_chip, irq, NULL); + + return ret; +} + static int sbsm_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -283,6 +436,10 @@ static int sbsm_probe(struct i2c_client *client, goto err_psy; } + ret = sbsm_gpio_setup(data); + if (ret < 0) + goto err_psy; + dev_info(dev, "sbsm registered\n"); return 0;
This patch added irq support for the SMBALERT pin and notification of the battery removal / insertion. The sbs manager would typically be used with the corresponding sbs-battery driver that currently uses a gpio input for battery presence and interrupt. To remain compatible with that existing driver this patch implements GPIO inputs with interrupt support. IRQ masking is performed in software as the hardware does not support masking of notifications from each battery. In addition a power_supply change notification is generated for the sbs manager device when the AC present flag is changed. Tested with LTC1760 and dual sbs compatible batteries. Signed-off-by: Phil Reid <preid@electromag.com.au> --- .../devicetree/bindings/power/sbs,sbs-manager.txt | 29 ++++ drivers/power/sbs-manager.c | 157 +++++++++++++++++++++ 2 files changed, 186 insertions(+)