Re: [PATCHv2 12/23] ARM: socfpga: dts: add a10 clock binding yaml

[Lothar Rubusch]

On Mon, Oct 21, 2024 at 9:05 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On Sun, Oct 20, 2024 at 07:40:17PM +0000, Lothar Rubusch wrote:
> > Convert content of the altera socfpga.txt to match clock bindings for
> > the Arria10 SoC devicetrees. Currently all altr,* bindings appear as
> > error at dtbs_check, since these bindings are only written in .txt
> > format.
> >
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with 'git log --oneline -- DIRECTORY_OR_FILE' on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>
> > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> > ---
> >  .../bindings/clock/altr,socfpga-a10.yaml      | 107 ++++++++++++++++++
> >  1 file changed, 107 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/altr,socfpga-a10.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/clock/altr,socfpga-a10.yaml b/Documentation/devicetree/bindings/clock/altr,socfpga-a10.yaml
> > new file mode 100644
> > index 000000000..795826f53
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/altr,socfpga-a10.yaml
> > @@ -0,0 +1,107 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/altr,socfpga-a10.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Device Tree Clock bindings for Altera's SoCFPGA platform
>
> This wasn't tested or you have some very, very old dtschema.
>
>
> > +
> > +maintainers:
> > +  - TODO
>
> We should not be taking unmaintained stuff.
>

This is just a trigger here. Basically, I have no probelm in placing
my own name here. AFAIR Mr. Dinh Nguyen has his name on the other
intel/altera related files, so I'm not sure who decides that. Please
let me know.

Basically this particular patch is related to my initial questions
(cover letter):
1.) Documentation/devicetree/bindings:
Executing the following find...
$ find ./Documentation/devicetree/bindings -name socfpga-\*.txt
...shows 4 text files describing "altr," bindings. I sketch-implemented
the clock binding and could reduce some of my dtbs_check warnings. So, my
questions is, if this is the right way? Shall I try to write .yaml files
for all 4 of them, too? Related to that, who will be maintainer?

2.) Some bindings, e.g. the Silabs clock generator seem to have no
driver, thus show up as warning:
    compatible = "silabs,si5338";
IMHO it is most likely rather to be probed/loaded in the SPL of the
bootloader. Is it problematic to keep those declarations (showing up as
warning in dtbs_check) or how to deal with them?

3.) Please, give me some feedback if the DT and binding adjustments are
going into total wrong direction, or where I may do better. If it is ok,
and acceptable, or what is still missing. I tried to split them, to
allow for better single integration / discussion let me know if this is
ok, too.


> > +
> > +description:
> > +  This binding uses the common clock binding[1].
> > +
> > +  [1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>
> Drop description or describe the hardware.

Ok (the description was taken as content from the corresponding .txt file)

> > +
> > +properties:
> > +  compatible:
> > +    description: |
> > +      shall be one of the following
> > +        - "altr,socfpga-a10-pll-clock" - for a PLL clock
> > +        - "altr,socfpga-a10-perip-clk" - The peripheral clock divided from the
> > +            PLL clock.
> > +        - "altr,socfpga-a10-gate-clk" - Clocks that directly feed peripherals
> > +            and can get gated.
>
> Drop description.

OK (dito)

> > +    enum:
> > +      - altr,socfpga-a10-pll-clock
> > +      - altr,socfpga-a10-perip-clk
> > +      - altr,socfpga-a10-gate-clk
>
> Why are you adding bindings per clock? Usually that's a no-go, you
> should be describing here clock controller unit.

as above, taken from .txt - So, I see there seems to be some interest
to convert the .txt files into .yaml files. I will have a look at the
other altr, ....txt -> .yaml cases, but will need to do more readings
first. This may become a different patch set, then.