Re: [PATCH v6 06/17] dt-bindings: mfd: amd,pensando-elbasr: Add AMD Pensando Elba System Resource chip

[Krzysztof Kozlowski]

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.

> 
>>> +
>>> +  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?

Best regards,
Krzysztof