Re: [PATCH v2 2/2] dt-bindings: soc: add loongson2 guts

[Yinbo Zhu]

在 2022/10/26 下午10:10, Krzysztof Kozlowski 写道:
> On 26/10/2022 03:22, Yinbo Zhu wrote:
> > 在 2022/10/26 上午3:40, Krzysztof Kozlowski 写道:
> > > On 24/10/2022 23:51, Yinbo Zhu wrote:
> > > > Add the loongson2 soc guts driver binding with DT schema format
> > > > using json-schema.
> > > >
> > > > Signed-off-by: Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
> > > > ---
> > > >    .../soc/loongson/loongson,ls2k-guts.yaml      | 37 +++++++++++++++++++
> > >
> > > Looks like wrong location, although difficult to judge because you did
> > > not describe the hardware at all. If this is chipinfo-like device, then
> > > Documentation/devicetree/bindings/hwinfo/.
yes it is a chipinfo/socinfo device, I will following your advice.
> > My guts driver is refer fsl platform. It was was to manage and access
> > global utilities register block for SoC and it was only used in SoC
> > platform. when driver need use Soc ops to do some function the this
> > driver was needed.  the dcfg (device config) was a function in guts
> > (global utilities) block.
>
> I can barely understand it.
My description is about chipinfo/socinfo definition. and I have a look
/bindings/hwinfo/, I think move binding file to hwinfo that is okay for me.

Documentation/devicetree/bindings/hwinfo/ti,k3-socinfo.yaml

> > For these type of driver, other platforms were initially placed on
> > Documentation/devicetree/bindings/arm/   if it is arm/arm64
> > architecture. Later, move it to the soc directory.
> >
> > Documentation/devicetree/bindings/soc/fsl/fsl,layerscape-dcfg.yaml
>
> How is this related? This is Layerscape, not Loongson2. Describe the
> hardware you are adding bindings for.
The driver functions/type are the same, the driver was register a struct 
soc_device_attribute by soc_device_register then other peripheral driver
can call SoC ops, such as soc_device_match.

then layerscape guts module bindings are placed in
Documentation/devicetree/bindings/soc/, the loongson guts module 
bindings was follow that layerscape and are placed in
Documentation/devicetree/bindings/soc/

In a words,  It is a question about where the binding file should be 
placed.  I think move binding file to hwinfo that is okay for me.
> > So, do you still think it is inappropriate to place it in the soc dir?
> > > >    MAINTAINERS                                   |  1 +
> > > >    2 files changed, 38 insertions(+)
> > > >    create mode 100644 Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-guts.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-guts.yaml b/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-guts.yaml
> > > > new file mode 100644
> > > > index 000000000000..2502f8aeb74d
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/soc/loongson/loongson,ls2k-guts.yaml
> > > > @@ -0,0 +1,37 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/soc/loongson/loongson,ls2k-guts.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Loongson2 GUTS driver.
> > >
> > > Drop "driver." unless you refer to some hardware (like motor driver?).
> > this need refer hardware soc datasheet to gain soc register (global
> > utilities register block ).
> > so keep "driver" string that whether was more appropriate?
>
> What? I cannot parse it.
>
> Did you understand my comment? If yes, please point to Wikipedia article
> explaining this "Driver" you refer to.
I will remove the "driver" string.
> > > > +
> > > > +maintainers:
> > > > +  - Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
> > > > +
> > > > +description: |
> > > > +  GUTS driver was to manage and access global utilities block. Initially
> > >
> > > Drop "driver" and describe instead what is GUTS, including its acronym,
> > >
> > > > +  only reading SVR and registering soc device are supported.
> > >
> > > Entire sentence describe Linux driver - drop it. Instead describe the
> > > device, the hardware.
> > >
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: loongson,ls2k-guts
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  little-endian: true
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    guts: guts@1fe00000 {
> > >
> > > Node names should be generic.
> > dcfg/scfg (device cfg/ soc cfg)was the key function of guts (global
> > utilities) block. and guts name I was refer fsl soc driver.
> > "drivers/soc/fsl/guts.c"
> > this binding file was follows of fsl guts.
> > Documentation/devicetree/bindings/soc/fsl/fsl,layerscape-dcfg.yaml
> > Documentation/devicetree/bindings/soc/fsl/fsl,layerscape-scfg.yaml
> >
> > or, I was use scfg as node name, Do you think it's appropriate?
>
> No, these are not generic node names.
I was refer "ti,k3-socinfo.yaml",  Do you think it's appropriate that 
socinfo as node name?
> > > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
> Best regards,
> Krzysztof