Message-ID: <0852ffa5-9996-0f42-c5a8-d1fe9d39887e@linaro.org>
Date:   Thu, 8 Sep 2022 13:27:36 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     "Larson, Bradley" <Bradley.Larson@....com>,
        Rob Herring <robh@...nel.org>
Cc:     "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        "adrian.hunter@...el.com" <adrian.hunter@...el.com>,
        "alcooperx@...il.com" <alcooperx@...il.com>,
        "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "brijeshkumar.singh@....com" <brijeshkumar.singh@....com>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "gsomlo@...il.com" <gsomlo@...il.com>,
        "gerg@...ux-m68k.org" <gerg@...ux-m68k.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "yamada.masahiro@...ionext.com" <yamada.masahiro@...ionext.com>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "piotrs@...ence.com" <piotrs@...ence.com>,
        "p.yadav@...com" <p.yadav@...com>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "samuel@...lland.org" <samuel@...lland.org>,
        "fancer.lancer@...il.com" <fancer.lancer@...il.com>,
        "Suthikulpanit, Suravee" <Suravee.Suthikulpanit@....com>,
        "Lendacky, Thomas" <Thomas.Lendacky@....com>,
        "ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
        "will@...nel.org" <will@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v6 06/17] dt-bindings: mfd: amd,pensando-elbasr: Add AMD
 Pensando Elba System Resource chip

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@....com>
>>>>> +
>>>>> +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

Powered by blists - more mailing lists