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

[Rhyland Klein]

On 3/13/2013 4:41 PM, Stephen Warren wrote:
> 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).

Apparently line wrapping only works properly on thunderbird if you force 
it to plain text (which of course I think I had...) all better now.

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

Yep you're right, I swapped them.

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

I agree, I think this is definitely a much cleaner way of doing it, then 
each component gets its own documentation file and is clean.

-rhyland

--
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/