On 01/09/2022 22:37, Larson, Bradley wrote: > On 9/1/22 12:20 AM, Krzysztof Kozlowski wrote: >> On 01/09/2022 02:01, Larson, Bradley wrote: >>>>> + is implemented by a sub-device reset-controller which accesses >>>>> + a CS0 control register. >>>>> + >>>>> +maintainers: >>>>> + - Brad Larson <blarson@xxxxxxx> >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + items: >>>>> + - enum: >>>>> + - amd,pensando-elbasr >>>>> + >>>>> + spi-max-frequency: >>>>> + description: Maximum SPI frequency of the device in Hz. >>>> No need for generic descriptions of common properties. >>> Changed to "spi-max-frequency: true" and moved to end of properties. >> Then you should rather reference spi-peripheral-props just like other >> SPI devices. > > > Will look at this dependent on the result of below > > >>>>> + >>>>> + reg: >>>>> + maxItems: 1 >>>>> + >>>>> + '#address-cells': >>>>> + const: 1 >>>>> + >>>>> + '#size-cells': >>>>> + const: 0 >>>>> + >>>>> + interrupts: >>>>> + maxItems: 1 >>>>> + >>>>> +required: >>>>> + - compatible >>>>> + - reg >>>>> + - spi-max-frequency >>>>> + >>>>> +patternProperties: >>>>> + '^reset-controller@[a-f0-9]+$': >>>>> + $ref: /schemas/reset/amd,pensando-elbasr-reset.yaml >>>>> + >>>>> +additionalProperties: false >>>>> + >>>>> +examples: >>>>> + - | >>>>> + #include <dt-bindings/interrupt-controller/arm-gic.h> >>>>> + >>>>> + spi { >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + num-cs = <4>; >>>>> + >>>>> + sysc: system-controller@0 { >>>>> + compatible = "amd,pensando-elbasr"; >>>>> + reg = <0>; >>>>> + #address-cells = <1>; >>>>> + #size-cells = <0>; >>>>> + spi-max-frequency = <12000000>; >>>>> + >>>>> + rstc: reset-controller@0 { >>>>> + compatible = "amd,pensando-elbasr-reset"; >>>>> + reg = <0>; >>>> What does 0 represent here? A register address within 'elbasr' device? >>> Removed, I recall a check threw a warning or error without reg. >>> >>>> Why do you need a child node for this? Are there other sub-devices and >>>> your binding is incomplete? Just put '#reset-cells' in the parent. >>> Without a reset-controller node and booting the function >>> __of_reset_control_get(...) fails to find a match in the list here >> That's not actually the answer to the question. There was no concerns >> whether you need or not reset controller. The question was why do you >> need a child device instead of elbasr being the reset controller. >> >> Your answer does not cover this at all, so again - why do you need a >> child for this? >> > > If the parent becomes a reset-controller compatible with > "amd,pensando-elbasr-reset" then the /dev node will not be created Why /dev node will not be created? How bindings affect having or not having something in /dev ? > as there is no match to "amd,pensando-elbasr" for cs0. For cs0 internal > to linux the reset-controller manages one register bit to hardware reset > the mmc device. A userspace application opens the device node to manage > transceiver leds, system leds, reporting temps to host, other reset > controls, etc. Looking at future requirements there likely will be other > child devices. You mean "amd,pensando-elbasr" will instantiate some more devices? Why you cannot add the binding for them now? This is actually important because earlier we agreed you remove unit address from children. > > Going down this path with one compatible should reset-elbasr.c just be > deleted and fold it into the parent driver pensando-elbasr.c? Then I > wonder if it even belongs in drivers/mfd and should just be modified > and put in drivers/spi. How is it related to bindings? Best regards, Krzysztof