Re: [PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base address change

[Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>]

On Thu, 19 Mar 2015, Stephen Warren wrote:

> On 03/19/2015 10:34 AM, Paul Walmsley wrote:
> > On Thu, 19 Mar 2015, Stephen Warren wrote:
> > 
> > > On 03/19/2015 09:33 AM, Paul Walmsley wrote:
> > > > On Tue, 17 Mar 2015, Stephen Warren wrote:
> > > > 
> > > > > On 03/17/2015 02:32 AM, Paul Walmsley wrote:
> > > > > > For Tegra132 and later chips, we can now use the correct hardware
> > > > > > base
> > > > > > address for the Tegra AHB IP block in the DT data.  Update the DT
> > > > > > binding
> > > > > > documentation to reflect this change.
> > > > > 
> > > > > > diff --git
> > > > > > a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > > > b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > > > index 067c979..7692b4c 100644
> > > > > > ---
> > > > > > a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > > > +++
> > > > > > b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
> > > > > > @@ -2,10 +2,15 @@ NVIDIA Tegra AHB
> > > > > > 
> > > > > >     Required properties:
> > > > > >     - compatible : For Tegra20, must contain "nvidia,tegra20-ahb".
> > > > > > For
> > > > > > -  Tegra30, must contain "nvidia,tegra30-ahb".  Otherwise, must
> > > > > > contain
> > > > > > -  '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
> > > > > > tegra124,
> > > > > > -  tegra132, or tegra210.
> > > > > > -- reg : Should contain 1 register ranges(address and length)
> > > > > > +  Tegra30, must contain "nvidia,tegra30-ahb".  For Tegra114 and
> > > > > > Tegra124,
> > > > > > must
> > > > > > +  contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip>
> > > > > > is
> > > > > > tegra114
> > > > > > +  or tegra124.  For Tegra132, the compatible string must contain
> > > > > > +  "nvidia,tegra132-ahb".
> > > > > > +
> > > > > > +- reg : Should contain 1 register ranges(address and length).  On
> > > > > > Tegra20,
> > > > > > +  Tegra30, Tegra114, and Tegra124 chips, the low byte of the
> > > > > > physical
> > > > > > base
> > > > > > +  address of the IP block must end in 0x04.  On DT files for later
> > > > > > chips,
> > > > > > the
> > > > > > +  actual hardware base address of the IP block should be used.
> > > > > 
> > > > > A table-based approach rather than prose might make this more legible?
> > > > > 
> > > > > - compatible: Must contain the following:
> > > > >     Tegra20:  "nvidia,tegra20-ahb"
> > > > >     Tegra30:  "nvidia,tegra30-ahb"
> > > > >     Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
> > > > >     Tegra124: "nvidia,tegra124-ahb", "nvidia,tegra30-ahb"
> > > > >     Tegra132: "nvidia,tegra132-ahb"
> > > > >     Tegra210: "nvidia,tegra210-ahb", "nvidia,tegra132-ahb"
> > > > > 
> > > > > With any luck, we can extend that final item for future chips to be:
> > > > > 
> > > > >     Tegra210, TegraNNN:
> > > > >               "nvidia,tegra<chip>-ahb", "nvidia,tegra132-ahb"
> > > > > 
> > > > > Perhaps we format the 114/124 entry that way too.
> > > > 
> > > > I think I'm just going to drop this patch, since Russell prefers that
> > > > the
> > > > workaround is applied in the driver.
> > > > 
> > > > With regards to using tables rather than narrative descriptions: perhaps
> > > > consider a patch to
> > > > Documentation/devicetree/bindings/submitting-patches.txt ?  I don't know
> > > > what the DT binding documentation maintainers' future plans are with
> > > > regards to automated documentation reflow, etc., but submitting a patch
> > > > there would stimulate at least some coordination on the issue.
> > > 
> > > I don't think it's appropriate for that file to dictate that, in the same
> > > way
> > > that coding style documentation generally doesn't address that kind of
> > > detail
> > > regarding code structure.
> > 
> > We do indeed specify details like this in our documentation guidelines.
> > Here are two examples:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kernel-doc-nano-HOWTO.txt#n103
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/CodingStyle#n464
> 
> Perhaps I phrased my point slightly differently form the core of what I meant.
> 
> I'm not aware that review feedback can't address topics that are not already
> addressed by the documentation. Is there such a rule?

Not that I'm aware of, but I'm not sure that I understand the point you're 
making.  Care to rephrase to make it more explicit?

> Equally, I think if you want the documentation to address a particular point,
> it's appropriate for you to submit a patch to the documentation to update it,
> rather than ask the reviewer to do so before accepting the review feedback.

I guess my question is this: do you intend that the table-based 
documentation approach you describe should apply generally to other DT 
binding documents with similar per-chip support lists?  Or is there 
something about the Tegra AHB specifically that merits this format?

If the former was intended -- in other words, you are proposing a policy 
that should be followed in the general case -- then I would suggest that 
the documentation policy should be described in a shared DT binding 
CodingStyle or submitting-patches document, as we do elsewhere in the 
kernel.

For example, the guidance could read[*], using your earlier example:

--- 
If different values of a DT property are required for different chips 
or different situations, these should be listed in the binding 
documentation in the following format:

- compatible: Must contain the following:
    Tegra20:  "nvidia,tegra20-ahb"
    Tegra30:  "nvidia,tegra30-ahb"
    Tegra114: "nvidia,tegra114-ahb", "nvidia,tegra30-ahb"
(etc.)

Each line in the list should be indented from the start of the section 
describing the DT property by four spaces.  There should be no blank
lines between each list row.
---

That way, the community can align on a common format for this table-based 
format.  Any automated parsing tools that read the DT documentation can 
know what to expect; anyone who disagrees can speak up as the patch is 
being considered; and the issue no longer needs to be a matter of taste: 
it can be transformed into a matter of fact.

Once the documentation format becomes a matter of fact, then patch 
submitters have clear guidance to follow.  Submitters can get the patches 
right the first time and avoid wasting their time and reviewers' time.  
Otherwise, there is the (quite present) risk that 'n' different reviewers 
of the DT binding documentation could have 'n' different opinions about 
how the data should be formatted, with each opinion conveying 
minimal-to-no technical advantage over another.  This just results in a 
waste of time for everyone, time that is better spent on code.  In my 
view, every moment I spend reformatting documentation to standards that 
aren't shared is not only wasted, it's time that's subtracted from my 
ability to improve our actual upstream code and work on something that's 
actually useful.


- Paul

[*] I am neutral about the format or whether a narrative vs. a table 
approach is best.  Whatever it should be, it should just be common 
guidance.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html