Date:   Wed, 13 Apr 2022 13:27:27 -0500
From:   Rob Herring <robh@kernel.org>
To:     Jarrett Schultz <jaschultzms@gmail.com>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Mark Gross <markgross@kernel.org>,
        Maximilian Luz <luzmaximilian@gmail.com>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        Felipe Balbi <balbi@kernel.org>,
        Jarrett Schultz <jaschultz@microsoft.com>
Subject: Re: [PATCH v5 1/4] dt-bindings: platform: microsoft: Document
 surface xbl
Message-ID: <YlcWD3dnqotAEnfA@robh.at.kernel.org>
References: <20220405210750.619511-1-jaschultzMS@gmail.com>
 <20220405210750.619511-2-jaschultzMS@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220405210750.619511-2-jaschultzMS@gmail.com>
Precedence: bulk

On Tue, Apr 05, 2022 at 02:07:47PM -0700, Jarrett Schultz wrote:
> From: Jarrett Schultz <jaschultz@microsoft.com>
> 
> Introduce yaml for surface xbl driver.

From Bjorn's reply on v4, it sounds like the QCom folks need to work out 
how to describe 'imem' first. I'd think that would use 'mmio-sram' 
binding and then this could be a child of that. If it's DDR, then it 
belongs under /reserved-memory node. Either way, that's all kind of 
outside the scope of the binding unless there's something special about 
'imem'.

> 
> Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> 
> ---
> 
> Changed in v5:
> 
>  - Updated description
>  - Added child node for imem
>  - Simplified example
> 
> ---
> 
> Changes in v4:
>  - Addressed small formatting changes
>  - Removed unnecessary lines
> 
> ---
> 
> Changes in v3:
>  - Updated description to only pertain to the hardware
>  - Updated the required field to properly reflect the binding
>  - Removed the first example
>  - Fixed the size of the reg field in the second example
> 
> ---
> 
> Changes in v2:
>  - Removed json-schema dependence
>  - Elaborated on description of driver
>  - Updated example
> ---
>  .../platform/microsoft/surface-xbl.yaml       | 70 +++++++++++++++++++

That's not a binding path. Probably bindings/nvmem/.

>  MAINTAINERS                                   |  7 ++
>  2 files changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> new file mode 100644
> index 000000000000..648476670bbd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/platform/microsoft/surface-xbl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Surface Extensible Bootloader for Microsoft Surface Duo
> +
> +maintainers:
> +  - Jarrett Schultz <jaschultz@microsoft.com>
> +
> +description: |
> +  Defined to expose information that is used during production when
> +  the device is in manufacturing mode. Some of the information included
> +  in the imem section is -
> +    * board_id
> +    * battery_present
> +    * hw_init_retries
> +    * is_customer_mode
> +    * is_act_mode
> +    * pmic_reset_reason
> +    * touch_fw_version
> +    * ocp_error_location
> +
> +properties:
> +  compatible:
> +    const: simple-mfd

This schema will never be applied (your example would fail if it did) 
because we filter out matching on 'simple-mfd' as it is not valid on its 
own.

> +
> +  reg:
> +    maxItems: 1
> +
> +  child-node:

This literally means you have a node called 'child-node'.

> +    description: A description of the xbl space within imem
> +
> +    type: object
> +
> +    properties:
> +      compatible: 
> +        const: microsoft,sm8150-surface-duo-xbl

This should be moved up a level removing 'simple-mfd'.

> +
> +      reg:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - ranges

> +  - address-cells
> +  - size-cells

Not documented and also the wrong names (missing '#').

> +
> +examples:
> +  - |
> +    imem@146bf000 {
> +      compatible = "simple-mfd";
> +      reg = <0x146bf000 0x1000>;
> +      ranges = <0x0 0x146bf000 0x1000>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      xbl@a94 {
> +        compatible = "microsoft,sm8150-surface-duo-xbl";
> +        reg = <0xa94 0x100>;
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 13f9a84a617e..5d0ca2a98b57 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12649,6 +12649,13 @@ F:	Documentation/driver-api/surface_aggregator/clients/dtx.rst
>  F:	drivers/platform/surface/surface_dtx.c
>  F:	include/uapi/linux/surface_aggregator/dtx.h
>  
> +MICROSOFT SURFACE DUO XBL DRIVER
> +M:	Jarrett Schultz <jaschultz@microsoft.com>
> +L:	linux-arm-msm@vger.kernel.org
> +L:	platform-driver-x86@vger.kernel.org
> +S:	Supported
> +F:	Documentation/devicetree/bindings/platform/microsoft/surface-xbl.yaml
> +
>  MICROSOFT SURFACE GPE LID SUPPORT DRIVER
>  M:	Maximilian Luz <luzmaximilian@gmail.com>
>  L:	platform-driver-x86@vger.kernel.org
> -- 
> 2.25.1
> 
>