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

[Krzysztof Kozlowski]

On 30/09/2022 00:50, Larson, Bradley wrote:
> On 9/16/22 2:56 AM, Krzysztof Kozlowski wrote:
>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>
>> On 13/09/2022 22:57, Larson, Bradley wrote:
>>> On 9/8/22 4:27 AM, Krzysztof Kozlowski wrote:
>>>> 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:
>>>>>>
>> Wait, can we skip the driver entirely? I am not reviewing your driver 
>> and what it creates under /dev. 
> 
> Yes, see precise answer requested below.
> 
>>> In comparision, the pensando device is also on the other end of spi,
>>> four chip selects with /dev created for each for userspace control,
>>> and one child device on cs0 for hw reset emmc that the Linux block
>>> layer controls (single bit managed only by kernel).
>>>
>>> Pensando:
>>> &spi0 {
>>>           num-cs = <4>;
>>>           cs-gpios = <0>, <0>, <&porta 1 GPIO_ACTIVE_LOW>,
>>>                      <&porta 7 GPIO_ACTIVE_LOW>;
>>>           status = "okay";
>>>           system-controller@0 {
>>>                   compatible = "amd,pensando-elbasr";
>>>                   reg = <0>;
>>>                   #address-cells = <1>;
>>>                   #size-cells = <0>;
>>>                   spi-max-frequency = <12000000>;
>>>
>>>                   rstc: reset-controller {
>>>                           compatible = "amd,pensando-elbasr-reset";
>>>                           #reset-cells = <1>;
>>>                   };
>>>           };
>>>
>>>           system-controller@1 {
>>>                   compatible = "amd,pensando-elbasr";
>>>                   reg = <1>;
>>>                   spi-max-frequency = <12000000>;
>>>           };
>>>
>>>           system-controller@2 {
>>>                   compatible = "amd,pensando-elbasr";
>>>                   reg = <2>;
>>>                   spi-max-frequency = <12000000>;
>>>                   interrupt-parent = <&porta>;
>>>                   interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>>>           };
>>>
>>>           system-controller@3 {
>>>                   compatible = "amd,pensando-elbasr";
>>>                   reg = <3>;
>>>                   spi-max-frequency = <12000000>;
>>>           };
>>> };
>> You replied with quite a response of which 90% is unrelated talk about 
>> driver. Please be specific. We talk here only about hardware.
>> Your last DTS might be the answer, but you never explicitly wrote 
>> it... So let's check if I understand it correctly. Only some of elbasr 
>> block contain reset control?
> 
> Yes, only the elbasr block accessed on CS0 provides reset control.  The 
> other 3 blocks don't have any reset control and never will.

I see, that could explain the subnode. However:
1. You still do not use any resources in the subnode (it does not have
any in DT).

2. Your driver instantiates subdevice not based on existing of subnode
or characteristics of a device (e.g. compatible), but on hard-coded
chip-select line. The reset driver directly takes parent's regmap - no
other resources.

Therefore this does not look like dedicated piece of hardware and should
be just part of parent node.

> 
>> This however does not answer my questions before.... You keep ignoring 
>> them. So please answer yes or no: "Are there other sub-devices?"
> 
> No
> 
>> " and your binding is incomplete?"
> 
> No
> 
>> and a new question: "Is reset block (amd,pensando-elbasr-reset) 
>> re-usable so it will appear in different device (not in 
>> amd,pensando-elbasr)?"
> 
> No its not re-usable

So squash it with parent node.

Best regards,
Krzysztof