Re: [PATCH 4/9] dt-bindings: remoteproc: qcom: wcnss: Convert to YAML

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 12/05/2022 08:50, Sireesh Kodali wrote:
> On Wed May 11, 2022 at 10:45 PM IST, Krzysztof Kozlowski wrote:
>> On 11/05/2022 18:15, Sireesh Kodali wrote:
>>> Convert the dt-bindings from txt to YAML. This is in preparation for
>>> including the relevant bindings for the MSM8953 platform's wcnss pil.
>>>
>>> Signed-off-by: Sireesh Kodali <sireeshkodali1@gmail.com>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>> Please use existing bindings or example-schema as a starting point. Half
>> of my review could be skipped if you just followed what we already have
>> in the tree.
>>
>> Some of these qcom specific properties already exist but you decided to
>> write them differently... please don't, rather reuse the code.
>>
> 
> Thank you for your review, I will make the chnages as appropriate in v2.
>> (...)
>>
>>> +
>>> +maintainers:
>>> +  - Bjorn Andersson <bjorn.andersson@linaro.org>
>>> +
>>> +description:
>>> +  This document defines the binding for a component that loads and boots
>>> +  firmware on the Qualcomm WCNSS core.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    oneOf:
>>> +      - items:
>>> +          - enum:
>>> +              - qcom,pronto-v2-pil
>>> +          - enum:
>>> +              - qcom,pronto
>>
>> This does not look correct. The fallback compatible should not change.
>> What is more, it was not documented in original binding, so this should
>> be done in separate patch.
>>
> 
> This was not a change to the fallback compatible. 

You made it an enum, so you expect it to use different fallback for
different cases.

> msm8916.dtsi's wcnss
> node has "qcom,pronto" as the compatible string, which is why this was
> added. It is however not documented in the txt file. Is it sufficient to
> add a note in the commit message, or should it be split into a separate
> commit?

Please split it, assuming that fallback is correct. Maybe the fallback
is wrong?

> 
>>> +      - items:
>>
>> No need for items, it's just one item.
>>
>>> +          - enum:
>>> +              - qcom,riva-pil
>>> +              - qcom,pronto-v1-pil
>>> +              - qcom,pronto-v2-pil
>>> +
>>> +  reg:
>>> +    description: must specify the base address and size of the CCU, DXE and PMU
>>> +      register blocks
>>
>> New line after "decription:", drop "must specify" and start with capital
>> letter.
>>
>> You need maxItems: 3
>>
> 
> Will fix in v2
>>
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: ccu
>>> +      - const: dxe
>>> +      - const: pmu
>>> +
>>> +  interrupts-extended:
>>> +    description:
>>> +      Interrupt lines
>>
>> Skip description, it's obvious.
>>
>> It should be only "interrupts", not extended.
>>
>>> +    minItems: 2
>>> +    maxItems: 5
>>> +
>>> +  interrupt-names:
>>> +    minItems: 2
>>> +    maxItems: 5
>>
>> Names should be clearly defined. They were BTW defined in original
>> bindings, so you should not remove them. This makes me wonder what else
>> did you remove from original bindings...
>>
>> Please document all deviations from pure conversion in the commit msg.
>> It's a second "hidden" difference.
>>
> 
> Sorry, this was meant to be a pure txt->YAML conversion. The missing
> interrupt names was accidental, and will be fixed in v2.
>>> +
>>> +  firmware-name:
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +    description: Relative firmware image path for the WCNSS core. Defaults to
>>> +      "wcnss.mdt".
>>
>>
>> Blank line after "description:". This applies to other places as well.
>>
>> Remove "Defailts to ..." and just add "default" schema.
>>
> 
> Will be fixed in v2
>>> +
>>> +  vddpx-supply:
>>> +    description: Reference to the PX regulator to be held on behalf of the
>>> +      booting of the WCNSS core
>>> +
>>> +  vddmx-supply:
>>> +    description: Reference to the MX regulator to be held on behalf of the
>>> +      booting of the WCNSS core.
>>> +
>>> +  vddcx-supply:
>>> +    description: Reference to the CX regulator to be held on behalf of the
>>> +      booting of the WCNSS core.
>>
>> s/Reference to the//
>>
>>> +
>>> +  power-domains:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    description: References to the power domains that need to be held on
>>> +      behalf of the booting WCNSS core
>>
>> 1. Ditto.
>> 2. No need for ref
>> 3. maxItems
>>
>>> +
>>> +  power-domain-names:
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>
>> No need for ref, skip description.
>>
>>> +    description: Names of the power domains
>>> +    items:
>>> +      - const: cx
>>> +      - const: mx
>>> +
>>> +  qcom,smem-states:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>>> +    description: States used by the AP to signal the WCNSS core that it should
>>> +      shutdown
>>> +    items:
>>> +      - description: Stop the modem
>>> +
>>> +  qcom,smem-state-names:
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>
>> No need for ref. Really, it does not appear in any of existing bindings
>> for smem-state-names, so how did you get it?
>>
> 
> The smem nodes were copied from /remoteproc/qcom,sdm845-adsp-pil.yaml

Hm, indeed, you're right. There are few files having here ref. I'll fix
these.

> 
>>> +    description: The names of the state bits used for SMP2P output
>>> +    items:
>>> +      - const: stop
>>> +
>>> +  memory-region:
>>> +    maxItems: 1
>>> +    description: Reference to the reserved-memory for the WCNSS core
>>> +
>>> +  smd-edge:
>>> +    type: object
>>> +    description:
>>> +      Qualcomm Shared Memory subnode which represents communication edge,
>>> +      channels and devices related to the ADSP.
>>
>> You should reference /schemas/soc/qcom/qcom,smd.yaml
> 
> Will be done in v2
>>
>>> +
>>> +  iris:
>>
>> Generic node name... what is "iris"?
>>
> Iris is the RF module, I'll make the description better

RF like wifi? Then the property name should be "wifi".



Best regards,
Krzysztof