| Message ID | 1466622436-27963-2-git-send-email-karl-heinz@schneider-inet.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, Jun 22, 2016 at 09:07:15PM +0200, Karl-Heinz Schneider wrote: > This patch adds device tree documentation for the sbs-manager > > Reviewed-by: Phil Reid <preid@electromag.com.au> > Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de> > --- > .../devicetree/bindings/power/sbs,sbs-manager.txt | 58 ++++++++++++++++++++++ > 1 file changed, 58 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/sbs,sbs-manager.txt > > diff --git a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt > new file mode 100644 > index 0000000..d52b466 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt > @@ -0,0 +1,58 @@ > +Binding for sbs-manager > + > +Required properties: > +- compatible: should be "sbs,sbs-manager" or "lltc,ltc1760" if device is a > + ltc1760. sbs is not a vendor. What chip is sbs-manager? I suspect you should drop it and only list specific chips. > +- reg: integer, i2c address of the device. Should be <0xa>. > + > +Optional properties: > +- sbsm,i2c-retry-count: integer, number of retries for trying to read or write > + to registers. Default: 1 Seems like a driver setting. Is having a retry in the driver a problem if the h/w works and never actually needs it? > + > +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 > +behaviour. So standard i2c-mux nodes can be used to register up to four slave > +batteries. Channels will be numerated as 1, 2, 4 and 8. > + > +Example: > + > +batman@0a { > + compatible = "sbs,sbs-manager"; > + reg = <0x0a>; > + sbsm,i2c-retry-count = <3>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + channel1@1 { channel@1 Do we have a standard node name for mux nodes? If not, we should. > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <1>; > + > + battery1@0b { battery@b > + compatible = "sbs-battery"; This should be an actual battery model. Or all this information is generic, you don't really need it in DT. > + reg = <0x0b>; > + }; > + }; > + > + channel2@2 { > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <2>; > + > + battery2@0b { > + compatible = "sbs-battery"; > + reg = <0x0b>; > + }; > + }; > + > + channel3@4 { > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <4>; > + > + battery3@0b { > + compatible = "sbs-battery"; > + reg = <0x0b>; > + }; > + }; > +}; > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 Couple of thoughts / question below. On 25/06/2016 01:50, Rob Herring wrote: > On Wed, Jun 22, 2016 at 09:07:15PM +0200, Karl-Heinz Schneider wrote: >> This patch adds device tree documentation for the sbs-manager >> >> Reviewed-by: Phil Reid <preid@electromag.com.au> >> Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de> >> --- >> .../devicetree/bindings/power/sbs,sbs-manager.txt | 58 ++++++++++++++++++++++ >> 1 file changed, 58 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/sbs,sbs-manager.txt >> >> diff --git a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt >> new file mode 100644 >> index 0000000..d52b466 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt >> @@ -0,0 +1,58 @@ >> +Binding for sbs-manager >> + >> +Required properties: >> +- compatible: should be "sbs,sbs-manager" or "lltc,ltc1760" if device is a >> + ltc1760. > > sbs is not a vendor. What chip is sbs-manager? I suspect you should drop > it and only list specific chips. This follows the interface to the existing paired sbs,sbs-battery driver defined in power/sbs-battery.c It implements a generic driver for the Smart Battery System Manager Specification. Spec available here: http://sbs-forum.org/specs/sbsm100b.pdf In addition the ltc1760 extends the spec. > >> +- reg: integer, i2c address of the device. Should be <0xa>. >> + >> +Optional properties: >> +- sbsm,i2c-retry-count: integer, number of retries for trying to read or write >> + to registers. Default: 1 > > Seems like a driver setting. Is having a retry in the driver a problem > if the h/w works and never actually needs it? Similarly the sbs-battery driver specifies the same same retry behaviour. And is a model for this implementation. I've found the ltc1760 and sbs batteries to be problematic when communicating to them. A lot of drivers (and the associated hardware) don't handle multiple bus masters well. The bus arbitation doesn't seem to work correctly. Retries where the only thing I could do to to get things to work reliably. Mostly means the driver needs fixing, but in one case the designware core hardware seemed to be the problem for me. > >> + >> +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 >> +behaviour. So standard i2c-mux nodes can be used to register up to four slave >> +batteries. Channels will be numerated as 1, 2, 4 and 8. >> + >> +Example: >> + >> +batman@0a { >> + compatible = "sbs,sbs-manager"; >> + reg = <0x0a>; >> + sbsm,i2c-retry-count = <3>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + channel1@1 { > > channel@1 > > Do we have a standard node name for mux nodes? If not, we should. > >> + #address-cells = <1>; >> + #size-cells = <0>; >> + reg = <1>; >> + >> + battery1@0b { > > battery@b > >> + compatible = "sbs-battery"; > > This should be an actual battery model. Or all this information is > generic, you don't really need it in DT. Do we really want to restrict to battery model? I have hardware where complete different sbs compliant batteries can be plugged in. Without the compatible flag here how do you get the sbs-battery driver to load and manage the battery itself? Or are you suggesting the manager should do this somehow? I'm still learning... > >> + reg = <0x0b>; >> + }; >> + }; >> + >> + channel2@2 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + reg = <2>; >> + >> + battery2@0b { >> + compatible = "sbs-battery"; >> + reg = <0x0b>; >> + }; >> + }; >> + >> + channel3@4 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + reg = <4>; >> + >> + battery3@0b { >> + compatible = "sbs-battery"; >> + reg = <0x0b>; >> + }; >> + }; >> +}; >> -- >> 1.9.1 >>
Hi Rob, sorry for resending, forgot the Cc. Am Freitag, den 24.06.2016, 12:50 -0500 schrieb Rob Herring: > On Wed, Jun 22, 2016 at 09:07:15PM +0200, Karl-Heinz Schneider wrote: > > This patch adds device tree documentation for the sbs-manager > > > > Reviewed-by: Phil Reid <preid@electromag.com.au> > > Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de> > > --- > > .../devicetree/bindings/power/sbs,sbs-manager.txt | 58 ++++++++++++++++++++++ > > 1 file changed, 58 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/power/sbs,sbs-manager.txt > > > > diff --git a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt > > new file mode 100644 > > index 0000000..d52b466 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt > > @@ -0,0 +1,58 @@ > > +Binding for sbs-manager > > + > > +Required properties: > > +- compatible: should be "sbs,sbs-manager" or "lltc,ltc1760" if device is a > > + ltc1760. > > sbs is not a vendor. What chip is sbs-manager? I suspect you should drop > it and only list specific chips. Yes it's not a specific chip. But it's the specification of the interface. See http://sbs-forum.org/specs/sbsm100b.pdf. One implementation (with special features) is the LTC1760. I choose this because the sbs-battery-driver did it the same way. You can find this in sbs-battery.c: static const struct of_device_id sbs_dt_ids[] = { { .compatible = "sbs,sbs-battery" }, { .compatible = "ti,bq20z75" }, { } }; Why not keep a generic compatibility string? > > > +- reg: integer, i2c address of the device. Should be <0xa>. > > + > > +Optional properties: > > +- sbsm,i2c-retry-count: integer, number of retries for trying to read or write > > + to registers. Default: 1 > > Seems like a driver setting. Is having a retry in the driver a problem > if the h/w works and never actually needs it? It could also be passed as a module parameter. But again the sbs-battery driver did it the same way, it just acted as template in this case... The retry problematic arises because the SBSM is itself an i2c master. And (at least the LTC) stretches hosts i2c clock if it does communicate with the connected batteries. This will likely disturb the hosts communication to any i2c chip connected on this bus sooner or later, including to the SBSM. > > > + > > +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 > > +behaviour. So standard i2c-mux nodes can be used to register up to four slave > > +batteries. Channels will be numerated as 1, 2, 4 and 8. > > + > > +Example: > > + > > +batman@0a { > > + compatible = "sbs,sbs-manager"; > > + reg = <0x0a>; > > + sbsm,i2c-retry-count = <3>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + channel1@1 { > > channel@1 > > Do we have a standard node name for mux nodes? If not, we should. I don't know. Other mux binding docs name the cannels just "i2c" or "port". Spec names them (the battery) "Smart Battery [A,B..]". Can at least drop the number in front of the @. > > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <1>; > > + > > + battery1@0b { > > battery@b > > > + compatible = "sbs-battery"; > > This should be an actual battery model. Or all this information is > generic, you don't really need it in DT. A solution could be to bind the sbs-battery driver within the sbs-manager driver for all detected channels per default, if not otherwise state in device tree? Current implementation doesn't bind anything by itself. Don't know what is the preferred behaviour? > > > + reg = <0x0b>; > > + }; > > + }; > > + > > + channel2@2 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <2>; > > + > > + battery2@0b { > > + compatible = "sbs-battery"; > > + reg = <0x0b>; > > + }; > > + }; > > + > > + channel3@4 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + reg = <4>; > > + > > + battery3@0b { > > + compatible = "sbs-battery"; > > + reg = <0x0b>; > > + }; > > + }; > > +}; > > -- > > 1.9.1 > > Tanks for review.
On Sun, Jun 26, 2016 at 12:21 AM, Phil Reid <preid@electromag.com.au> wrote: > G'day Rob > > Couple of thoughts / question below. > > On 25/06/2016 01:50, Rob Herring wrote: >> >> On Wed, Jun 22, 2016 at 09:07:15PM +0200, Karl-Heinz Schneider wrote: >>> >>> This patch adds device tree documentation for the sbs-manager >>> >>> Reviewed-by: Phil Reid <preid@electromag.com.au> >>> Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de> >>> --- >>> .../devicetree/bindings/power/sbs,sbs-manager.txt | 58 >>> ++++++++++++++++++++++ >>> 1 file changed, 58 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/power/sbs,sbs-manager.txt >>> >>> diff --git a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt >>> b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt >>> new file mode 100644 >>> index 0000000..d52b466 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt >>> @@ -0,0 +1,58 @@ >>> +Binding for sbs-manager >>> + >>> +Required properties: >>> +- compatible: should be "sbs,sbs-manager" or "lltc,ltc1760" if device is >>> a >>> + ltc1760. >> >> >> sbs is not a vendor. What chip is sbs-manager? I suspect you should drop >> it and only list specific chips. > > This follows the interface to the existing paired sbs,sbs-battery driver > defined in power/sbs-battery.c > It implements a generic driver for the Smart Battery System Manager > Specification. > Spec available here: http://sbs-forum.org/specs/sbsm100b.pdf > > In addition the ltc1760 extends the spec. Chips will always vary from specs in some way either on purpose or by accident. sbs,sbs-manager is fine as a fallback string, but there should always be a chip specific string first. >>> +- reg: integer, i2c address of the device. Should be <0xa>. >>> + >>> +Optional properties: >>> +- sbsm,i2c-retry-count: integer, number of retries for trying to read or >>> write >>> + to registers. Default: 1 >> >> >> Seems like a driver setting. Is having a retry in the driver a problem >> if the h/w works and never actually needs it? > > Similarly the sbs-battery driver specifies the same same retry behaviour. > And is a model for this implementation. > > I've found the ltc1760 and sbs batteries to be problematic when > communicating to them. > A lot of drivers (and the associated hardware) don't handle multiple bus > masters well. > The bus arbitation doesn't seem to work correctly. > Retries where the only thing I could do to to get things to work reliably. > Mostly means the driver needs fixing, but in one case the designware core > hardware seemed to be the problem for me. I'm not questioning the need for a retry. I'm questioning the need to limit the retries and tune per platform. What would be the issue if the driver hardcodes the number of retries to 10? This will work for any h/w that needs 0, 1, 2, ..., or 10 retries. The only issue would be how long until it errors out. And yes, I can confirm DW i2c h/w is a POS at least for some versions. >>> +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 >>> +behaviour. So standard i2c-mux nodes can be used to register up to four >>> slave >>> +batteries. Channels will be numerated as 1, 2, 4 and 8. >>> + >>> +Example: >>> + >>> +batman@0a { >>> + compatible = "sbs,sbs-manager"; >>> + reg = <0x0a>; >>> + sbsm,i2c-retry-count = <3>; >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + channel1@1 { >> >> >> channel@1 >> >> Do we have a standard node name for mux nodes? If not, we should. >> >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + reg = <1>; >>> + >>> + battery1@0b { >> >> >> battery@b >> >>> + compatible = "sbs-battery"; >> >> >> This should be an actual battery model. Or all this information is >> generic, you don't really need it in DT. > > Do we really want to restrict to battery model? You're not. It is just being explicit in case some battery needs special handling and you only find that out after writing the binding. The DT model is such that the kernel can be updated with fixes without updating the DT. Specific compatible strings are needed for that to work. > I have hardware where complete different sbs compliant batteries can be > plugged in. > Without the compatible flag here how do you get the sbs-battery driver to > load and manage the battery itself? > Or are you suggesting the manager should do this somehow? > I'm still learning... sbs,sbs-battery can still be a fall-back compatible string. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-06-24 19:50, Rob Herring wrote: > On Wed, Jun 22, 2016 at 09:07:15PM +0200, Karl-Heinz Schneider wrote: >> This patch adds device tree documentation for the sbs-manager *snip* >> + >> +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 >> +behaviour. So standard i2c-mux nodes can be used to register up to four slave >> +batteries. Channels will be numerated as 1, 2, 4 and 8. >> + >> +Example: >> + >> +batman@0a { >> + compatible = "sbs,sbs-manager"; >> + reg = <0x0a>; >> + sbsm,i2c-retry-count = <3>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + channel1@1 { > > channel@1 > > Do we have a standard node name for mux nodes? If not, we should. No name is enforced by the i2c mux support code, but I think "i2c" dominates, and quite possibly it is the only documented name? Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 26, 2016 at 5:35 PM, Peter Rosin <peda@axentia.se> wrote: > On 2016-06-24 19:50, Rob Herring wrote: >> On Wed, Jun 22, 2016 at 09:07:15PM +0200, Karl-Heinz Schneider wrote: >>> This patch adds device tree documentation for the sbs-manager > > *snip* > >>> + >>> +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 >>> +behaviour. So standard i2c-mux nodes can be used to register up to four slave >>> +batteries. Channels will be numerated as 1, 2, 4 and 8. >>> + >>> +Example: >>> + >>> +batman@0a { >>> + compatible = "sbs,sbs-manager"; >>> + reg = <0x0a>; >>> + sbsm,i2c-retry-count = <3>; >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + channel1@1 { >> >> channel@1 >> >> Do we have a standard node name for mux nodes? If not, we should. > > No name is enforced by the i2c mux support code, but I think "i2c" > dominates, and quite possibly it is the only documented name? The kernel generally doesn't care what node names are, but standard naming is convention. If "i2c" is most common, then go with that. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Montag, den 27.06.2016, 10:28 -0500 schrieb Rob Herring: > On Sun, Jun 26, 2016 at 5:35 PM, Peter Rosin <peda@axentia.se> wrote: > > On 2016-06-24 19:50, Rob Herring wrote: > >> On Wed, Jun 22, 2016 at 09:07:15PM +0200, Karl-Heinz Schneider wrote: > >>> This patch adds device tree documentation for the sbs-manager > > > > *snip* > > > >>> + > >>> +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 > >>> +behaviour. So standard i2c-mux nodes can be used to register up to four slave > >>> +batteries. Channels will be numerated as 1, 2, 4 and 8. > >>> + > >>> +Example: > >>> + > >>> +batman@0a { > >>> + compatible = "sbs,sbs-manager"; > >>> + reg = <0x0a>; > >>> + sbsm,i2c-retry-count = <3>; > >>> + #address-cells = <1>; > >>> + #size-cells = <0>; > >>> + > >>> + channel1@1 { > >> > >> channel@1 > >> > >> Do we have a standard node name for mux nodes? If not, we should. > > > > No name is enforced by the i2c mux support code, but I think "i2c" > > dominates, and quite possibly it is the only documented name? > > The kernel generally doesn't care what node names are, but standard > naming is convention. If "i2c" is most common, then go with that. Will do so. "channelx" will go "i2c" "batteryx" will go "battery" > > Rob Karl-Heinz -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Sonntag, den 26.06.2016, 09:05 -0500 schrieb Rob Herring: > On Sun, Jun 26, 2016 at 12:21 AM, Phil Reid <preid@electromag.com.au> wrote: > > G'day Rob > > > > Couple of thoughts / question below. > > > > On 25/06/2016 01:50, Rob Herring wrote: > >> > >> On Wed, Jun 22, 2016 at 09:07:15PM +0200, Karl-Heinz Schneider wrote: > >>> > >>> This patch adds device tree documentation for the sbs-manager > >>> > >>> Reviewed-by: Phil Reid <preid@electromag.com.au> > >>> Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de> > >>> --- > >>> .../devicetree/bindings/power/sbs,sbs-manager.txt | 58 > >>> ++++++++++++++++++++++ > >>> 1 file changed, 58 insertions(+) > >>> create mode 100644 > >>> Documentation/devicetree/bindings/power/sbs,sbs-manager.txt > >>> > >>> diff --git a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt > >>> b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt > >>> new file mode 100644 > >>> index 0000000..d52b466 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt > >>> @@ -0,0 +1,58 @@ > >>> +Binding for sbs-manager > >>> + > >>> +Required properties: > >>> +- compatible: should be "sbs,sbs-manager" or "lltc,ltc1760" if device is > >>> a > >>> + ltc1760. > >> > >> > >> sbs is not a vendor. What chip is sbs-manager? I suspect you should drop > >> it and only list specific chips. > > > > This follows the interface to the existing paired sbs,sbs-battery driver > > defined in power/sbs-battery.c > > It implements a generic driver for the Smart Battery System Manager > > Specification. > > Spec available here: http://sbs-forum.org/specs/sbsm100b.pdf > > > > In addition the ltc1760 extends the spec. > > Chips will always vary from specs in some way either on purpose or by > accident. sbs,sbs-manager is fine as a fallback string, but there > should always be a chip specific string first. All right. Will change compatible string to "lltc,ltc1760" and mention in the Required properties section that "sbs,sbs-manager" is usable as fallback. > > >>> +- reg: integer, i2c address of the device. Should be <0xa>. > >>> + > >>> +Optional properties: > >>> +- sbsm,i2c-retry-count: integer, number of retries for trying to read or > >>> write > >>> + to registers. Default: 1 > >> > >> > >> Seems like a driver setting. Is having a retry in the driver a problem > >> if the h/w works and never actually needs it? > > > > Similarly the sbs-battery driver specifies the same same retry behaviour. > > And is a model for this implementation. > > > > I've found the ltc1760 and sbs batteries to be problematic when > > communicating to them. > > A lot of drivers (and the associated hardware) don't handle multiple bus > > masters well. > > The bus arbitation doesn't seem to work correctly. > > Retries where the only thing I could do to to get things to work reliably. > > Mostly means the driver needs fixing, but in one case the designware core > > hardware seemed to be the problem for me. > > I'm not questioning the need for a retry. I'm questioning the need to > limit the retries and tune per platform. What would be the issue if > the driver hardcodes the number of retries to 10? This will work for > any h/w that needs 0, 1, 2, ..., or 10 retries. The only issue would > be how long until it errors out. > > And yes, I can confirm DW i2c h/w is a POS at least for some versions. > > >>> +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 > >>> +behaviour. So standard i2c-mux nodes can be used to register up to four > >>> slave > >>> +batteries. Channels will be numerated as 1, 2, 4 and 8. > >>> + > >>> +Example: > >>> + > >>> +batman@0a { > >>> + compatible = "sbs,sbs-manager"; > >>> + reg = <0x0a>; > >>> + sbsm,i2c-retry-count = <3>; > >>> + #address-cells = <1>; > >>> + #size-cells = <0>; > >>> + > >>> + channel1@1 { > >> > >> > >> channel@1 > >> > >> Do we have a standard node name for mux nodes? If not, we should. > >> > >>> + #address-cells = <1>; > >>> + #size-cells = <0>; > >>> + reg = <1>; > >>> + > >>> + battery1@0b { > >> > >> > >> battery@b > >> > >>> + compatible = "sbs-battery"; > >> > >> > >> This should be an actual battery model. Or all this information is > >> generic, you don't really need it in DT. > > > > Do we really want to restrict to battery model? > > You're not. It is just being explicit in case some battery needs > special handling and you only find that out after writing the binding. > The DT model is such that the kernel can be updated with fixes without > updating the DT. Specific compatible strings are needed for that to > work. > > > I have hardware where complete different sbs compliant batteries can be > > plugged in. > > Without the compatible flag here how do you get the sbs-battery driver to > > load and manage the battery itself? > > Or are you suggesting the manager should do this somehow? > > I'm still learning... > > sbs,sbs-battery can still be a fall-back compatible string. Consider this scenario: compatible = "ti,bq2060", "sbs,sbs-battery"; By now the there is no special "bq2060" driver. But my battery might be on of those. I code the DT this way everything should work find (sbs-battery driver will bind). If battery is removable and battery device changes everything should still work fine hence sbs-battery works on subset of registers of the standard. But what happens if a kernel update is done and a bq2060 driver appears which might want access to registers not part of the standard and the (now changed) device doesn't provide? > > Rob Karl-Heinz -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 28/06/2016 05:10, Karl-Heinz Schneider wrote: > Am Sonntag, den 26.06.2016, 09:05 -0500 schrieb Rob Herring: >> On Sun, Jun 26, 2016 at 12:21 AM, Phil Reid <preid@electromag.com.au> wrote: *Snip* >>>>> +- reg: integer, i2c address of the device. Should be <0xa>. >>>>> + >>>>> +Optional properties: >>>>> +- sbsm,i2c-retry-count: integer, number of retries for trying to read or >>>>> write >>>>> + to registers. Default: 1 >>>> >>>> >>>> Seems like a driver setting. Is having a retry in the driver a problem >>>> if the h/w works and never actually needs it? >>> >>> Similarly the sbs-battery driver specifies the same same retry behaviour. >>> And is a model for this implementation. >>> >>> I've found the ltc1760 and sbs batteries to be problematic when >>> communicating to them. >>> A lot of drivers (and the associated hardware) don't handle multiple bus >>> masters well. >>> The bus arbitation doesn't seem to work correctly. >>> Retries where the only thing I could do to to get things to work reliably. >>> Mostly means the driver needs fixing, but in one case the designware core >>> hardware seemed to be the problem for me. >> >> I'm not questioning the need for a retry. I'm questioning the need to >> limit the retries and tune per platform. What would be the issue if >> the driver hardcodes the number of retries to 10? This will work for >> any h/w that needs 0, 1, 2, ..., or 10 retries. The only issue would >> be how long until it errors out. >> >> And yes, I can confirm DW i2c h/w is a POS at least for some versions. >> So your suggesting we hardcode the retry value in the driver and not provide a configuration option in the binding? My only thought with allowing a dt setitng to customise the value is it allows the integrator to select how many retires they want to try before failing, which kind of limits the elapse time until a failure is reported.
On Tue, Jun 28, 2016 at 09:13:21AM +0800, Phil Reid wrote: > G'day Rob, > > On 28/06/2016 05:10, Karl-Heinz Schneider wrote: > >Am Sonntag, den 26.06.2016, 09:05 -0500 schrieb Rob Herring: > >>On Sun, Jun 26, 2016 at 12:21 AM, Phil Reid <preid@electromag.com.au> wrote: > > *Snip* > > > >>>>>+- reg: integer, i2c address of the device. Should be <0xa>. > >>>>>+ > >>>>>+Optional properties: > >>>>>+- sbsm,i2c-retry-count: integer, number of retries for trying to read or > >>>>>write > >>>>>+ to registers. Default: 1 > >>>> > >>>> > >>>>Seems like a driver setting. Is having a retry in the driver a problem > >>>>if the h/w works and never actually needs it? > >>> > >>>Similarly the sbs-battery driver specifies the same same retry behaviour. > >>>And is a model for this implementation. > >>> > >>>I've found the ltc1760 and sbs batteries to be problematic when > >>>communicating to them. > >>>A lot of drivers (and the associated hardware) don't handle multiple bus > >>>masters well. > >>>The bus arbitation doesn't seem to work correctly. > >>>Retries where the only thing I could do to to get things to work reliably. > >>>Mostly means the driver needs fixing, but in one case the designware core > >>>hardware seemed to be the problem for me. > >> > >>I'm not questioning the need for a retry. I'm questioning the need to > >>limit the retries and tune per platform. What would be the issue if > >>the driver hardcodes the number of retries to 10? This will work for > >>any h/w that needs 0, 1, 2, ..., or 10 retries. The only issue would > >>be how long until it errors out. > >> > >>And yes, I can confirm DW i2c h/w is a POS at least for some versions. > >> > > So your suggesting we hardcode the retry value in the driver and not provide a > configuration option in the binding? Yes. > My only thought with allowing a dt setitng to customise the value is it allows the > integrator to select how many retires they want to try before failing, which kind of > limits the elapse time until a failure is reported. It is easier to add later and can't be removed later. If we do want something like this, then it should be a common property for i2c devices/buses. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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/power/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt new file mode 100644 index 0000000..d52b466 --- /dev/null +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt @@ -0,0 +1,58 @@ +Binding for sbs-manager + +Required properties: +- compatible: should be "sbs,sbs-manager" or "lltc,ltc1760" if device is a + ltc1760. +- reg: integer, i2c address of the device. Should be <0xa>. + +Optional properties: +- sbsm,i2c-retry-count: integer, number of retries for trying to read or write + to registers. Default: 1 + +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 +behaviour. So standard i2c-mux nodes can be used to register up to four slave +batteries. Channels will be numerated as 1, 2, 4 and 8. + +Example: + +batman@0a { + compatible = "sbs,sbs-manager"; + reg = <0x0a>; + sbsm,i2c-retry-count = <3>; + #address-cells = <1>; + #size-cells = <0>; + + channel1@1 { + #address-cells = <1>; + #size-cells = <0>; + reg = <1>; + + battery1@0b { + compatible = "sbs-battery"; + reg = <0x0b>; + }; + }; + + channel2@2 { + #address-cells = <1>; + #size-cells = <0>; + reg = <2>; + + battery2@0b { + compatible = "sbs-battery"; + reg = <0x0b>; + }; + }; + + channel3@4 { + #address-cells = <1>; + #size-cells = <0>; + reg = <4>; + + battery3@0b { + compatible = "sbs-battery"; + reg = <0x0b>; + }; + }; +};