Date:	Thu, 19 Mar 2015 16:27:15 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Paul Walmsley <paul@...an.com>
CC:	linux-tegra@...r.kernel.org, linux-arm-kernel@...r.kernel.org,
	Mark Rutland <mark.rutland@....com>,
	Alexandre Courbot <gnurou@...il.com>,
	Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	linux-kernel@...r.kernel.org,
	Eduardo Valentin <edubezval@...il.com>,
	devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Kumar Gala <galak@...eaurora.org>,
	Hiroshi DOYU <hdoyu@...dia.com>
Subject: Re: [PATCH 3/3] Documentation: DT bindings: Tegra AHB: note base
 address change

I guess pretend like I never made the suggestion.

On 03/19/2015 12:42 PM, Paul Walmsley wrote:
> 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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists