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

[Sireesh Kodali]

On Thu May 12, 2022 at 4:32 PM IST, Krzysztof Kozlowski wrote:
> On 12/05/2022 11:32, Sireesh Kodali wrote:
> >>>>> +          - 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?
> > 
> > The code doesn't recognize "qcom,pronto", so perhaps the best solution
> > is to just remove that compatible from msm8916.dtsi?
>
> Eh, I don't know. You need to check, maybe also in downstream sources.
>

I just checked, it seems "qcom,pronto" is used by the wcnss driver in
/net. So both "qcom,pronto-v2-pil" and "qcom,pronto" need to be present,
but the latter wasn't documented.

> (...)
>
> >>>>
> >>>>> +
> >>>>> +  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".
> > 
> > RF like wifi and bluetooth. However there are wifi and bt subnodes in
> > the smd-edge subnode. Iris is just the antenna hardware if I understand
> > correctly. Also this is just a documentation of the existing nodes that
> > are present in msm8916.dtsi, but for whatever reason their documentation
> > was missing in the txt file. Without adding this node in the YAML
> > dtb_check fails.
>
> It seems commit fd52bdae9ab0 ("wcn36xx: Disable 5GHz for wcn3620")
>  added usage of "iris" property but did not document it in the bindings.
>
> You can fix it by documenting (separate patch) existing practice or
> document with changing the node name. I am not sure if it is worth the
> effort, so just new patch please.
>

I'll make a 2 separate patches, documenting the extra "qcom,pronto"
compatible, and the iris subnode.

Thanks,
Sireesh

> Best regards,
> Krzysztof