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

[Krzysztof Kozlowski]

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