Re: [PATCH] dt-bindings: arm: Convert Marvell CP110 System Controller to DT schema

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Rob,

Thanks for the conversion!

On 14/10/2025 at 10:30:19 -05, "Rob Herring (Arm)" <robh@kernel.org> wrote:

> Convert the Marvell CP110 System Controller binding to DT schema
> format.
>
> There's not any specific compatible for the whole block which is a
> separate problem, so just the child nodes are documented. Only the
> pinctrl and clock child nodes need to be converted as the GPIO node
> already has a schema.
>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---

...

> -Those clocks can be referenced by other Device Tree nodes using two
> -cells:
> - - The first cell must be 0 or 1. 0 for the core clocks and 1 for the
> -   gateable clocks.
> - - The second cell identifies the particular core clock or gateable
> -   clocks.
> -
> -The following clocks are available:
> - - Core clocks
> -   - 0 0	APLL
> -   - 0 1	PPv2 core
> -   - 0 2	EIP
> -   - 0 3	Core
> -   - 0 4	NAND core
> -   - 0 5	SDIO core
> - - Gateable clocks
> -   - 1 0	Audio
> -   - 1 1	Comm Unit
> -   - 1 2	NAND
> -   - 1 3	PPv2
> -   - 1 4	SDIO
...

Why do you want to drop this information? Telling

    #clock-cells = <2>

is not enough IMO, we must tell people what is expected in these
cells. At the very least the cell values can be constrained to [0-1] for the first
one and [0-5] or [0-26] respectively for the second one.

But giving their meaning I think makes sense. I agree, these should have been
defined inside a shared header, that would have been a better way to
keep track of their meaning, but if we don't have that, I would propose
to keep the information here?

[...]

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,armada-7k-pinctrl.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/marvell,armada-7k-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Armada 7K/8K pin controller

...

> +        enum: [
> +          au, dev, ge, ge0, ge1, gpio, i2c0, i2c1, io, led, link,
> mii,

I don't think "io" has ever been a valid value, it probably comes from a
typo while sorting out all the possibilities ;-) (probably a left over
of a gpio, mdio or sdio string).

> +          mss_gpio0, mss_gpio1, mss_gpio2, mss_gpio3, mss_gpio4, mss_gpio5,
> +          mss_gpio6, mss_gpio7, mss_i2c, mss_spi, mss_uart, nf, pcie, pcie0,
> +          pcie1, pcie2, ptp, rei, sata0, sata1, sdio, sdio_cd, sdio_wp, sei,
> +          spi0, spi1, synce1, synce2, tdm, uart0, uart1, uart2, uart3, wakeup,
> +          xg

Rest LGTM otherwise.

Thanks,
Miquèl