Re: [PATCH 2/3] arm64: dts: qcom: msm8916: Drop underscore in node name

[Stephan Gerhold <stephan@gerhold.net>]

On Tue, Sep 21, 2021 at 11:20:18AM -0700, Stephen Boyd wrote:
> Quoting Stephan Gerhold (2021-09-21 08:21:19)
> > Using underscores in device tree nodes is not very common.
> > Additionally, the _region suffix in "smem_region@..." is not really
> > useful since it's obvious that it describes a reserved memory region.
> >
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >  arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > index 5551dba2d5fd..95dea20cde75 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > @@ -41,7 +41,7 @@ tz-apps@86000000 {
> >                         no-map;
> >                 };
> >
> > -               smem_mem: smem_region@86300000 {
> > +               smem_mem: smem@86300000 {
> 
> Shouldn't that be smem_mem: memory@86300000? Node names should be
> generic.
> 

The way I read it, the DT schema [1] and device tree specification [2]
interprets the generic name recommendation a bit different here:

> Following the generic-names recommended practice, node names should
> reflect the purpose of the node (ie. "framebuffer" or "dma-pool").

"framebuffer" or "dma-pool" would also be "memory", yet they suggest
a name reflecting the purpose instead. The purpose of the node is
"smem", it's not just arbitrary "memory".

However, my main problem with using memory@ here is that it actually
causes new dtbs_check errors:

apq8016-sbc.dt.yaml: memory@86000000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)
apq8016-sbc.dt.yaml: memory@86300000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)
apq8016-sbc.dt.yaml: memory@86400000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)
apq8016-sbc.dt.yaml: memory@86500000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)
apq8016-sbc.dt.yaml: memory@86680000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)
apq8016-sbc.dt.yaml: memory@86700000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)
apq8016-sbc.dt.yaml: memory@867e0000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)
apq8016-sbc.dt.yaml: memory@86800000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)
apq8016-sbc.dt.yaml: memory@89300000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)
apq8016-sbc.dt.yaml: memory@89900000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)
apq8016-sbc.dt.yaml: memory@8ea00000: 'device_type' is a required property (From schema: dtschema/schemas/memory.yaml)

Looks like it thinks this is a definition of physical memory now.
I would rather not add more errors. :)

Stephan

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml
[2]: https://github.com/devicetree-org/devicetree-specification/blob/master/source/chapter3-devicenodes.rst#reserved-memory-child-nodes