Re: [PATCH v2 1/2] dt-bindings: thermal: mediatek: Move auxdac binding to yaml

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3349 bytes --]

On Thu, Jun 01, 2023 at 03:44:24PM +0200, matthias.bgg@kernel.org wrote:
> From: Matthias Brugger <matthias.bgg@gmail.com>
> 
> Convert the older binding to yaml syntax.
> The thermal IP has several sensors, to reflect that
> thermal-sensors-cells is set to '1'. Apart optional regulator
> for bank supply wasn't part of the old binding description,
> this patch adds them.
> 
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> 
> ---
> 
> Changes in v2:
> - drop auxadc and apmixedsys nodes
> - maxItems for clocks
> - drop obvious description of reset
> - mention optional regulators in commit message
> - mention thermal-sensors-cells change in commit message
> 
>  .../bindings/thermal/mediatek,thermal.yaml    | 152 ++++++++++++++++++
>  .../bindings/thermal/mediatek-thermal.txt     |  52 ------
>  2 files changed, 152 insertions(+), 52 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml
>  delete mode 100644 Documentation/devicetree/bindings/thermal/mediatek-thermal.txt
> 
> diff --git a/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml
> new file mode 100644
> index 000000000000..bbc9c2935da0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml
> @@ -0,0 +1,152 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/mediatek,thermal.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Thermal Sensor
> +
> +maintainers:
> +  - Matthias Brugger <matthias.bgg@gmail.com>
> +
> +description: |
                ^
You don't need this btw, unless you have formatting to preserve.

> +  The MediaTek thermal controller measures the on-SoC temperatures.
> +  This device does not have its own ADC, instead it directly controls
> +  the AUXADC via AHB bus accesses. For this reason this device needs
> +  phandles to the AUXADC. Also it controls a mux in the apmixedsys
> +  register space via AHB bus accesses, so a phandle to the APMIXEDSYS
> +  is also needed.

> +  reset-names:
> +    items:
> +      - const: therm

> +  nvmem-cell-names:
> +    items:
> +      - const: calibration-data

You don't need the "items: - const:", "const:" alone is sufficient.

> +
> +  mediatek,auxadc:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      A phandle to the AUXADC which the thermal controller uses...
...to read the on-SoC temperatures as it does not have its own ADC.

Moving the information out of the description down here seems like it
would be better than what the original text binding does. Sorry for not
point that out last time around.

> +
> +  mediatek,apmixedsys:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      A phandle to the APMIXEDSYS controller...

Ideally the information about the purpose of the mux would go here, but
I don't know the purpose of it :)

> +  "#thermal-sensor-cells":
> +    const: 1
> +
> +  bank0-supply:
> +    description: Regulator supplying voltage to the first bank

Since I am an eejit, first bank of what? Sensors?

Otherwise, LGTM.

Cheers,
Conor.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]