Re: [PATCH v3 1/2] ARM: at91/dt: shdwc binding: add new shutdown controller documentation

[Nicolas Ferre]

Le 03/10/2015 19:02, Alexandre Belloni a écrit :
> On 30/09/2015 at 18:22:00 +0200, Nicolas Ferre wrote :
>> +SHDWC Shutdown Controller (Alternative)
>> +
>> +1) shdwc node
>> +
>> +required properties:
>> +- compatible: should be "atmel,sama5d2-shdwc".
>> +- reg: should contain registers location and length
>> +- clocks: phandle to input clock.
>> +- #address-cells: should be one. The cell is the wake-up input index.
>> +- #size-cells: should be zero.
>> +
>> +optional properties:
>> +
>> +- atmel,wakeup-debouncer: minimum wake-up inputs debouncer period in
> 
> Shouldn't that property be called atmel,wakeup-debouncer-ms ?

Well, as it's microseconds, it should be "-us", according to the file:
Documentation/devicetree/bindings/property-units.txt.

Reworking on this I realised that a kind "generic" property exists for
this type: "debounce-delay-ms", without a vendor prefix.

So, for the DT people, what do you think if I change my private property
to this one that seems to be used in a bunch of bindings already in its
milliseconds variant:
gpio-matrix-keypad.txt, tc3589x.txt, extcon-palmas.txt or with a vendor
prefix in nvidia,tegra20-kbc.txt or lpc32xx-key.txt

and if I diversify it to a "debounce-delay-us" variant and use it in
this driver?

>> +
>> +2) input nodes
>> +
>> +Wake-up input nodes are usually described in the "board" part of the Device
>> +Tree. Note also that input 0 is linked to the wake-up pin and is frequently
>> +used.
>> +
>> +Required properties:
>> +- reg: should contain the wake-up input index [0 - 15].
>> +
>> +Optional properties:
>> +- atmel,wakeup-type: string, operation mode of the input described by the child
>> +  node. Supported values are: "high" or "low".
>> +
> 
> Maybe we could avoid parsing string and use an integer with a few defines

Yes, even a boolean should do the trick. What about:

- atmel,wakeup-active-high: boolean, the corresponding wake-up
  input described by the child, forces the wake-up of the core power
  supply on a high level. The default is to be active low.

Thanks for your insights. Best regards,
-- 
Nicolas Ferre