Re: [PATCH 1/2] Documentation: Add sbs-manager device tree node documentation

[Karl-Heinz Schneider <karl-heinz@schneider-inet.de>]

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