Re: [PATCH v3 1/2] can: m_can: add device tree binding documentation

[Varka Bhadram <varkabhadram@gmail.com>]

On 07/14/2014 08:54 AM, Dong Aisheng wrote:
> On Fri, Jul 11, 2014 at 04:11:03PM +0530, Varka Bhadram wrote:
>> On 07/11/2014 03:59 PM, Dong Aisheng wrote:
>>> add M_CAN device tree binding documentation
>>>
>>> Cc: Wolfgang Grandegger <wg@grandegger.com>
>>> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
>>> Cc: Varka Bhadram <varkabhadram@gmail.com>
>>> Signed-off-by: Dong Aisheng <b29396@freescale.com>
>>> ---
>>>   .../devicetree/bindings/net/can/m_can.txt          |   65 ++++++++++++++++++++
>>>   1 files changed, 65 insertions(+), 0 deletions(-)
>>>   create mode 100644 Documentation/devicetree/bindings/net/can/m_can.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
>>> new file mode 100644
>>> index 0000000..c4cb263
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>>> @@ -0,0 +1,65 @@
>>> +Bosch MCAN controller Device Tree Bindings
>>> +-------------------------------------------------
>>> +
>>> +Required properties:
>>> +- compatible		: Should be "bosch,m_can" for M_CAN controllers
>>> +- reg			: physical base address and size of the M_CAN
>>> +			  registers map and Message RAM
>>> +- reg-names		: Should be "m_can" and "message_ram"
>>> +- interrupts		: Should be the interrupt number of M_CAN interrupt
>>> +			  line 0 and line 1, could be same if sharing
>>> +			  the same interrupt.
>>> +- interrupt-names	: Should contain "int0" and "int1"
>>> +- clocks		: Clocks used by controller, should be host clock
>>> +			  and CAN clock.
>>> +- clock-names		: Should contain "hclk" and "cclk"
>>> +- pinctrl-<n>		: Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt
>> I think this should be pinctrl-0
>>
> First, this part is defined by pinctrl binding doc.
> Second i think it may be possible someone wants to add other pinctrl states
> when implement low power state in the future.
> So i just keep it as pinctrl-<n>.

Normally we will use pinctrl-0 for mentioning the pinctrl bindings.

If somebody going to add something to the pinctrl bindings they will
add separately with pinctrl-1: bla bla bla...

>>> +- pinctrl-names		: Names corresponding to the numbered pinctrl states
>> remove 1 tab space before :
>>
> It's a bit strange.
> Other line like pinctrl-<n> is also two tabs.
> And the code looks fine and already aligned.
> - pinctrl-<n>		: Pinctrl states as described in bindings/pinctrl/pinctrl-bindings.txt
> - pinctrl-names		: Names corresponding to the numbered pinctrl states
> Do you mean change line of pinctrl-names from two tabs to one space and a tab before :?
>
When i see in the patch the alignment is like this
pinctrl-<n>	:
pinctrl-name		:

>>> +- mram-cfg		: Message RAM configuration data.
>>> +  Multiple M_CAN instances can share the same Message RAM and each element(e.g
>>> +  Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable,
>>> +  so this property is telling driver how the shared or private Message RAM
>>> +  are used by this M_CAN controller.
>>> +
>> It may written like:
>> mram-cfg		: Message RAM configuration data
>> 			  Multiple M_CAN instances can share the same Message RAM and each element
>> 			  (e.g Rx FIFO or Tx Buffer and etc) number in Message RAM is also configurable,
>> 			  ...
>>
> I'm fine with that.
> The question is it's easy to over 80 columns if writing like that,
> is it ok?

When we follow the above format it would be more readable.

> Regards
> Dong Aisheng


-- 
Regards,
Varka Bhadram.