Re: [RFC PATCH 2/5] Documentation: dt-bindings: add example DT binding document

[Matt Porter <mporter-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>]

On Fri, Aug 28, 2015 at 01:55:11PM -0500, Rob Herring wrote:
> On Fri, Aug 28, 2015 at 12:49 PM, Matt Porter <mporter-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> > On Fri, Aug 28, 2015 at 09:53:06AM -0500, Rob Herring wrote:
> >> On Fri, Aug 28, 2015 at 12:23 AM, Matt Porter <mporter-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> >> > Add a skeleton DT binding document that serves as the canonical
> >> > example for implementing YAML-based DT bindings documentation.
> >> > The skeleton binding illustrates use of all fields and variations
> >> > described in the dt-binding-format.txt documentation.
> >> >
> >> > Signed-off-by: Matt Porter <mporter-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >> > ---
> >> >  Documentation/devicetree/bindings/skeleton.yaml | 98 +++++++++++++++++++++++++
> >> >  1 file changed, 98 insertions(+)
> >> >  create mode 100644 Documentation/devicetree/bindings/skeleton.yaml
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/skeleton.yaml b/Documentation/devicetree/bindings/skeleton.yaml
> >> > new file mode 100644
> >> > index 0000000..175965f
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/skeleton.yaml
> >> > @@ -0,0 +1,98 @@
> >> > +%YAML 1.2
> >> > +---
> >> > +id: skel-device
> >> > +
> >> > +title: Skeleton Device
> >> > +
> >> > +maintainer:
> >> > +  - name: Skeleton Person <skel-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >>
> >> We'd want to tie this into get_maintainers.pl obviously.
> >
> > Right, I broke my rule of "no new tags for content we don't already
> > have" by adding this. It stems out of the discussion at LPC where
> > Mark suggested we could avoid the core bindings being moved by adding
> > maintainer info into the binding itself.
> 
> If you stated that rule, I missed it...

Mental list of rules I made for my initial cut at this. I was trying
really hard to have this be a cut and paste reformatting to start and
then see where comments take us.

> > This is an area where more docs are needed. Ideally, on the first pass
> > conversion we would not attempt to populate these. I had considered
> > that we *could* only add maintainer: tags on core bindings to and
> > adjust get_maintainers.pl to use that info to override the standard
> > directory-based info if it exists. I don't think it's necessary to add
> > a specific maintainer for all of the peripheral bindings if the default
> > is the subsystem maintainer and the dt list. We could at least start
> > directing core binding discussion to the -spec list which is where I
> > think you'd like it.
> 
> Yes, certainly this can solve my problem around handling core bindings.
> 
> Some subsystem maintainers require a driver have a maintainer. We
> could certainly enforce that too. We could probably automatically
> populate this with the original author as a starting point. OTOH,
> bindings should not change frequently.

Populating with the original author seems easy enough.

> >> > +description: >
> >> > +  The Skeleton Device binding represents the SK11 device produced by
> >> > +  the Skeleton Corporation. The binding can also support compatible
> >> > +  clones made by second source vendors.
> >> > +
> >> > +compatible:
> >> > +  - name: "skel,sk11"
> >> > +  - name: "faux,fx11"
> >>
> >> Is this an OR or AND? We need both.
> >
> > True, this only covers the OR case atm.
> >
> >> The complicated case is "one of {specific names} followed by {generic name}."
> >
> > I need to rethink these. I do have deprecated: tag for that case and
> > possibly "name:" gets split to "generic:" and "specific:" and we can
> > then do the right thing.
> >
> > For the above I would have:
> >
> > compatible:
> >   - specific: "skel,sk11"
> >   - specific: "faux,fx11"
> 
> This can still be an AND or OR relationship.

Ok, so this might need to be sequences of and: and or:
key value pairs to reflect the combinations.

> > and something like the Allwinner simple framebuffer would be:
> >
> > compatible:
> >   - generic: "simple-framebuffer"
> >   - specific: "allwinner,simple-framebuffer"
> >
> > where our validator would insist on seeing one of the specific: tags
> > along with the generic: tag. A lot of bindings, given current doc
> > patterns would just have two tags like above.
> 
> We could follow Pantelis' suggestion and do C style logic expressions:
> 
> name: ("chip1-ipblock" || "chip2-ipblock") && "generic-ipblock"

That might work. More specialized parsing, but it probably is needed
to handle property values ranges anyway.

> >> > +    description: A clone of the original sk11 device
> >> > +
> >> > +required:
> >> > +  - name: "reg"
> >>
> >> We definitely need type info from the start.
> >
> > Are you sure? Ugh.
> >
> > My big contention here is that we don't carry that content in the
> > docs today so we shouldn't try to add it in the initial conversion.
> 
> What? We have that all over the place. Boolean properties, phandles, etc.

I mean that it's not always explicitly called out in peripheral
DT bindings...it's inconsistently documented. I think that by properly
documenting this in the generic bindings as you are suggesting and
getting those properties by reference, the task of documenting these
won't be so bad.

> > I think you are right in that we should have it documented in the
> > schema but I'm concerned that we make the starting conversion
> > effort too large by adding type info to all converted docs.
> 
> How we do the conversion certainly needs to be worked out. However,
> first we need some sort of agreement that yes this looks promising
> before we spend too much time on it. We need to wait for all the YAML
> haters to come out of hiding. :) Once the direction is settled, then

I was expecting the YAML haters by now, I'll give it another few days.
:)

> we can iron out the details of the structure. Finally, then we can
> determine how to do the conversion. Just because we define how type
> information looks doesn't mean it has to be there day 1.

Agreed.
 
> > I think we gain a lot even without at the start, but I understand
> > why we need it and how it will help reduce the review firehose
> > with the associated validation tools.
> >
> >>
> >> > +    description: chip select address of skeleton device
> >> > +    reference: spi-slave
> >>
> >> I would like to not have to list properties if the inherited binding
> >> lists it. The problem is we need to say how many cells and the order
> >> (not a problem here, but for mmio devices).
> >
> > Yeah, make sense.
> >
> >> Perhaps we can list the reference at the top level for the node
> >> instead of for every property.
> >
> > That's a good point. I was wondering if per prop references would
> > get unwieldy once we actually add them into all the converted docs.
> > There's a huge number of existing docs without proper references.
> 
> Yes, in many cases they are implied. It should be simple enough to
> generate the list though whether they point to the doc file or just
> list the properties.

Ok.

> 
> >
> > Ok, I'll take a look at collecting references per group of properties.
> >
> >>
> >> > +  - name: "spi-max-frequency"
> >> > +    description: >
> >> > +      Maximum SPI clocking speed of skeleton device in Hz, must be
> >> > +      1000000
> >> > +    reference: spi-slave
> >>
> >> Rather than listing the property and having constraint in description,
> >> perhaps we could add constraints like this:
> >>
> >> - spi-max-frequency-range: 1000000 1000000
> >>
> >> Or groups of constraints:
> >>
> >> - spi-max-frequency-constraints:
> >>   range: 1000000 1000000
> >>   some-other-constraint: <value>
> >
> > I was hoping to avoid this to start due to my argument for keeping it
> > simple for the first pass at conversion. However, the latter looks
> > flexible enough. We have cases with enumerated values as well to handle
> > the require some thought.
> 
> We can always add constraints as a second step, but I'd like to define
> the structure at least.

Ok, this sounds reasonable, I'm goling to take a stab at reorganizing
the format based on the big areas of references, property types, and
property constraints.

-Matt