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

[Larson, Bradley]

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
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.

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.

Regards,
Brad