Re: [Patch v3 3/4] power_supply: tps65090-charger: Add binding doc

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 03/13/2013 01:25 PM, Rhyland Klein wrote:
> On 3/12/2013 7:10 PM, Stephen Warren wrote:
>> On 03/12/2013 04:08 PM, Rhyland Klein wrote:
>>> This change adds the binding documentation for the tps65090-charger.
>>> diff --git
>>> a/Documentation/devicetree/bindings/power_supply/tps65090.txt
>>> b/Documentation/devicetree/bindings/power_supply/tps65090.txt
>>> +Example:
>>> +
>>> +    tps65090@48 {
>>> +        compatible = "ti,tps65090";
>>> +        reg = <0x48>;
>>> +        interrupts = <0 88 0x4>;
>>> +
>>> +        ti,enable-low-current-chrg;
>>> +
>>> +        regulators {
>>> +            ...
>>> +        };
>> I'm a little confused by this binding.
>>
>> What goes in the regulators sub-node; is that specified by another
>> binding file in bindings/regulator/tps65090.txt?
>>
>> I would expect one of the following:
>>
>> 1) A single binding file that describes absolutely everything in the
>> chip. In this case, the main TPS65909 node wouldn't have child nodes for
>> the MFD components, although the regulators sub-node, which in turn
>> contains children does still make sense.
>>
>> 2) A separate binding for each component block, and perhaps also some
>> top-level binding that indicates which child bindings can "plug into"
>> it. In this case, I'd expect each block to be represented as a sub-node
>> in DT. The overall regulator component might then still have a
>> regulators child DT node itself, to represent each regulator's
>> configuration. In this scenario, each binding document describes the
>> entirety of a single node.
>>
>> I think what you've got here is a hybrid; a single top-level node, but
>> different binding documents defining the various properties that are
>> relevant to each component block in the device. That seems odd to me.
> 
> Yes we started this discussion before and were discussing the proper
> arrangement of

(your message was wrapped far over 80 columns, please restrict to
something like 72-74 or so).

> documentation when dealing with devices like these. This is where the
> drivers/ directory
> naming in the binding docs might diverge a bit as it might make less
> sense to have
> a binding doc for each child component of an mfd.

I think your discussion swaps #1 and #2 relative to my list above? It
certainly makes more sense if I read it that way.

> I was thinking about moving this driver towards #1 above, and using a
> child node for
> the charger. I would then also move the regulators to a child node, and
> its structure would
> be very similar to the Palmas driver/dt representation. My only concern
> was that, from
> what I understood, separating out the child node implied that the child
> functionality
> could/might be used somewhere else. Say in this case, that the charger
> functionality might
> be duplicated in another pmic from ti.

Having separate nodes certainly makes it easy to /allow/ that. I don't
think it /requires/ that TI release (or plan to release) another chip
containing that IP block.

> I don't know how much that is the case with the
> tps65090 and so I am unsure if child nodes are the correct way to go.

Thinking more about this, separate child nodes for separate IP blocks do
start to make some sense. They definitely do allow easy
binding/driver/... re-use if future chips come out with the same
sub-block. Similarly, they neatly isolate each sub-block's binding from
the others, so (a) avoid any conflicts between what the different IP
blocks want to put in their nodes, like a list of regulators vs. using
sub-nodes for other purposes, such as an integrate I2C mux with I2C bus
child nodes, etc, and (b) the bindings can be defined separately thus
avoiding the "where do we put the binding file" issue.

It's certainly more work, but now I tend to think separate nodes are the
way to go.

> As for #2, This would also be fine with me, as logically we are talking
> about a single chip. I
> this the only concern here is where to place a single binding document
> in the bindings
> directory where it makes sense. Putting regulator documentation under
> charger or vice
> versa doesn't make sense. And then for some devices, they might also
> have an rtc, gpio
> controller, interrupt controller, etc.. If each of them had a driver and
> their own dt
> information, I don't know where a single core place for all that
> documentation would be
> right now.
> 
> Hence, I was hoping to continue this dicussion and see if we can decide
> on the most logical
> choice, whatever that may be.