Re: [PATCH v2 1/3] dt-bindings: nvmem: syscon: Add syscon backed nvmem bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 17/05/2023 17:25, Marek Vasut wrote:
> Add trivial bindings for driver which permits exposing syscon backed
> register to userspace. This is useful e.g. to expose U-Boot boot
> counter on various platforms where the boot counter is stored in
> random volatile register, like STM32MP15xx TAMP_BKPxR register.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Cc: devicetree@vger.kernel.org
> Cc: kernel@dh-electronics.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> ---
> V2: Use generic syscon supernode
> ---
>  .../bindings/nvmem/nvmem-syscon.yaml          | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/nvmem-syscon.yaml
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem-syscon.yaml b/Documentation/devicetree/bindings/nvmem/nvmem-syscon.yaml
> new file mode 100644
> index 0000000000000..7c1173a1a6218
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/nvmem-syscon.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/nvmem-syscon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic syscon backed nvmem
> +
> +maintainers:
> +  - Marek Vasut <marex@denx.de>
> +
> +allOf:
> +  - $ref: "nvmem.yaml#"

Usual comment: drop quotes. We removed them everywhere, so you based
your work on some old tree.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - nvmem-syscon
> +
> +  reg:
> +    maxItems: 1

Rob's questions are not solved. The nvmem.yaml schema expects here to
allow children. This should not be created per-register, but per entire
block of registers.

OTOH, using nvmem for syscon (which are MMIO and writeable registers
usually) seems odd.

> +

missing nvmem cells

> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false

unevaluatedProps: false

> +
> +examples:
> +  - |
> +    syscon {
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        syscon@14c {

It's not really a syscon, but efuse, otp or nvmem.

> +            compatible = "nvmem-syscon";
> +            reg = <0x14c 0x4>;

Missing nvmem cells

> +        };
> +    };

Best regards,
Krzysztof