Messages in this thread | | | Date | Mon, 9 Oct 2023 16:04:05 -0500 | Subject | Re: [PATCH 2/3] dt-bindings: mfd: sie,cronos-cpld: Add initial DT binding | From | Shawn Anastasio <> |
| |
On 10/3/23 4:22 AM, Krzysztof Kozlowski wrote: > On 03/10/2023 00:32, Shawn Anastasio wrote: >> The SIE Cronos Platform Controller CPLD is a multi-purpose platform > > What is SIE? Vendor prefix says sony. >
(Repeated from my response to your reply to patch 1) Sony Interactive Entertainment is a separate corporate entity and it's the one that created the hardware to which this driver pertains.
> What is Cronos? >
Sorry, I'll amend the description to clarify this. Cronos is an x86 server platform developed and deployed by SIE.
> >> controller that provides both a watchdog timer and an LED controller. As >> both functions are provided by the same CPLD, a multi-function device is >> exposed as the parent of both functions. > > A nit, subject: drop second/last, redundant "DT binding". The > "dt-bindings" prefix is already stating that these are bindings. >
Will do.
>> >> Add a DT binding for this device. >> >> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com> > > Except that this was clearly no tested... >
My apologies, it seems I didn't have all of the required dependencies installed locally to enable dt binding validation. Will fix.
>> --- > > ... > >> +properties: >> + compatible: >> + const: sie,cronos-cpld >> + >> + reg: >> + maxItems: 1 >> + >> + '#address-cells': >> + const: 1 > > Why do you need it? > >> + >> + '#size-cells': >> + const: 1 > > Also looks unneeded. >
These were inherited from an existing dt binding in the tree that I used as a reference. I'll drop them both at your request.
>> + >> + leds: >> + type: object >> + description: Cronos Platform Status LEDs > > Missing additionalProperties:false... but anyway this is just empty. No > resources? Drop the node. >
Having nodes for the leds and the watchdog allows the two independent functions to be enabled/disabled in the device tree by adding/removing the relevant object. Would it be more idiomatic to instead introduce properties to the parent sie,cronos-cpld object to toggle these functions?
>> + >> + properties: >> + compatible: >> + const: sie,cronos-leds >> + >> + watchdog: >> + type: object >> + description: Cronos Platform Watchdog Timer >> + >> + properties: >> + compatible: >> + const: sie,cronos-watchdog > > No resources? Drop the node. >
Same question as above.
> Best regards, > Krzysztof
Thanks, Shawn
| |