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@xxxxxxxxxxxxxxxxxxxxx> > > 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
