On Sun, Jun 26, 2016 at 12:21 AM, Phil Reid [off-list ref] wrote:

G'day Rob

Couple of thoughts / question below.

On 25/06/2016 01:50, Rob Herring wrote:

quoted

On Wed, Jun 22, 2016 at 09:07:15PM +0200, Karl-Heinz Schneider wrote:

quoted

This patch adds device tree documentation for the sbs-manager

Reviewed-by: Phil Reid <redacted>
Signed-off-by: Karl-Heinz Schneider <redacted>
---
 .../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.

quoted

quoted

+- 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.

quoted

quoted

+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.

quoted

+        #address-cells = <1>;
+        #size-cells = <0>;
+        reg = <1>;
+
+        battery1@0b {

battery@b

quoted

+            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