linux-kernel - Re: [PATCHv2 1/8] ARM: at91: Add at91sam9rl DT SoC support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 20 Feb 2014 09:59:23 +0100
From:	Boris BREZILLON <b.brezillon@...rkiz.com>
To:	Mark Rutland <mark.rutland@....com>,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>
CC:	Nicolas Ferre <nicolas.ferre@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Gregory Clement <gregory.clement@...e-electrons.com>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCHv2 1/8] ARM: at91: Add at91sam9rl DT SoC support

Hi Mark,

On 19/02/2014 18:54, Mark Rutland wrote:
> On Wed, Feb 19, 2014 at 05:31:42PM +0000, Alexandre Belloni wrote:
>> On 19/02/2014 at 17:00:20 +0000, Mark Rutland wrote :
>>> On Wed, Feb 19, 2014 at 03:32:24PM +0000, Alexandre Belloni wrote:
>>>> This adds preliminary DT support for the at91sam9rl.
>>>>
>>>> Signed-off-by: Alexandre Belloni <alexandre.belloni@...e-electrons.com>
>>>> ---
>>>>   arch/arm/boot/dts/at91sam9rl.dtsi | 628 ++++++++++++++++++++++++++++++++++++++
>>>>   arch/arm/mach-at91/at91sam9rl.c   |  16 +
>>>>   2 files changed, 644 insertions(+)
>>>>   create mode 100644 arch/arm/boot/dts/at91sam9rl.dtsi
>>>   
>>> [...]
>>>
>>>> +                       tcb0: timer@...a0000 {
>>>> +                               compatible = "atmel,at91rm9200-tcb";
>>>> +                               reg = <0xfffa0000 0x100>;
>>>> +                               interrupts = <16 IRQ_TYPE_LEVEL_HIGH 0
>>>> +                                             17 IRQ_TYPE_LEVEL_HIGH 0
>>>> +                                             18 IRQ_TYPE_LEVEL_HIGH 0>;
>>>> +                       };
>>> Nit: please bracket list entries individually. Also for other list
>>> properties like reg and (*-)gpio(s).
>>>
>> OK.
>>
>>> [...]
>>>
>>>> +                       adc0: adc@...d0000 {
>>>> +                               compatible = "atmel,at91sam9260-adc";
>>>> +                               reg = <0xfffd0000 0x100>;
>>>> +                               interrupts = <20 IRQ_TYPE_LEVEL_HIGH 0>;
>>>> +                               atmel,adc-use-external-triggers;
>>>> +                               atmel,adc-channels-used = <0xf>;
>>>> +                               atmel,adc-vref = <3300>;
>>>> +                               atmel,adc-num-channels = <4>;
>>>> +                               atmel,adc-startup-time = <15>;
>>>> +                               atmel,adc-channel-base = <0x30>;
>>>> +                               atmel,adc-drdy-mask = <0x10000>;
>>>> +                               atmel,adc-status-register = <0x1c>;
>>>> +                               atmel,adc-trigger-register = <0x04>;
>>>> +                               atmel,adc-res = <8 10>;
>>>> +                               atmel,adc-res-names = "lowres", "highres";
>>>> +                               atmel,adc-use-res = "highres";
>>>> +
>>>> +                               trigger@0 {
>>>> +                                       trigger-name = "timer-counter-0";
>>>> +                                       trigger-value = <0x1>;
>>>> +                               };
>>> A unit-address should go with a reg value. Either this needs a reg and
>>> the parent node needs #address-cells and #size-cells, or the
>>> unit-address should go, and the names made unique through other means.
>>>
>> OK, I guess I'll have to fix
>> Documentation/devicetree/bindings/arm/atmel-adc.txt too.
> Yes please.
>
>>> [...]
>>>
>>>> +                       pinctrl@...ff400 {
>>>> +                               #address-cells = <1>;
>>>> +                               #size-cells = <1>;
>>>> +                               compatible = "atmel,at91rm9200-pinctrl", "simple-bus";
>>> NAK. Either this is a atmel,at91rm9200-pinctrl node or a simple-bus. Not
>>> both; that doesn't make any sense.
>>>
>> Simply a copy paste, I'll fix that here and also the 6 other atmel
>> dtsi includes.
>>
>> What is your preference for those using:
>> compatible = "atmel,at91sam9x5-pinctrl", "atmel,at91rm9200-pinctrl", "simple-bus"; ?
> A node should by either a bus or a pinctrl node.
>
> If it has chidren then the simple-bus should be separated out into a
> separate node. If there are no children simple-bus should go.

Doing this clearly break backward compatibility (the current pinctrl 
drivers relies on
gpio controller being subnodes of the pinctrl node), but I'm interested 
in how you would
have represented this.

Could you give us an example ?


>
> Cheers,
> Mark.

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