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

[Rob Herring <robh@kernel.org>]

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