| Message ID | 20230530150413.12918-1-matthias.bgg@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: thermal: mediatek: Move auxdac binding to yaml | expand |
Hey Matthias, On Tue, May 30, 2023 at 05:04:12PM +0200, matthias.bgg@kernel.org wrote: > + 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. That double "also" bothers my OCD greatly, but it is a faithful conversion. > + "#thermal-sensor-cells": > + const: 1 > -- #thermal-sensor-cells : Should be 0. See Documentation/devicetree/bindings/thermal/thermal-sensor.yaml for a description. How come this has changed? I didn't see an explanation for it in either of the patches. Cheers, Conor.
On 31/05/2023 00:02, Conor Dooley wrote: > Hey Matthias, > > On Tue, May 30, 2023 at 05:04:12PM +0200, matthias.bgg@kernel.org wrote: > >> + 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. > > That double "also" bothers my OCD greatly, but it is a faithful > conversion. > >> + "#thermal-sensor-cells": >> + const: 1 > >> -- #thermal-sensor-cells : Should be 0. See Documentation/devicetree/bindings/thermal/thermal-sensor.yaml for a description. > > How come this has changed? I didn't see an explanation for it in either > of the patches. Yep. Please mention the deviations from pure conversion and if not obvious - justify them. Best regards, Krzysztof
On 30/05/2023 17:04, matthias.bgg@kernel.org wrote: > From: Matthias Brugger <matthias.bgg@gmail.com> > > Convert the older binding to yaml syntax. > > Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> > --- > > .../bindings/thermal/mediatek,thermal.yaml | 168 ++++++++++++++++++ > .../bindings/thermal/mediatek-thermal.txt | 52 ------ > 2 files changed, 168 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..7aa2bdc43567 > --- /dev/null > +++ b/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml > @@ -0,0 +1,168 @@ > +# 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: | > + 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. > + > +properties: > + compatible: > + oneOf: > + - enum: > + - mediatek,mt2701-thermal > + - mediatek,mt2712-thermal > + - mediatek,mt7622-thermal > + - mediatek,mt7986-thermal > + - mediatek,mt8173-thermal > + - mediatek,mt8183-thermal > + - mediatek,mt8365-thermal > + - items: > + - const: mediatek,mt7981-thermal > + - const: mediatek,mt7986-thermal > + - items: > + - const: mediatek,mt8516-thermal > + - const: mediatek,mt2701-thermal > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + minItems: 2 maxItems instead > + > + clock-names: > + items: > + - const: therm > + - const: auxadc > + > + resets: > + maxItems: 1 > + description: Reference to the reset controller controlling the thermal controller. You can drop description, it's obvious. > + > + reset-names: > + items: > + - const: therm > + > + nvmem-cells: > + items: > + - description: Calibration eFuse data. If unspecified default values are used. > + > + nvmem-cell-names: > + items: > + - const: calibration-data > + > + mediatek,auxadc: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + A phandle to the AUXADC which the thermal controller uses. > + > + mediatek,apmixedsys: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + A phandle to the APMIXEDSYS controller. > + > + "#thermal-sensor-cells": > + const: 1 > + > + bank0-supply: > + description: Regulator node supplying voltage to the first bank > + > + bank1-supply: > + description: Regulator node supplying voltage to the second bank These were not present before. Mention this in commit msg. Also drop "node". > + > + Just one blank line. > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - mediatek,auxadc > + - mediatek,apmixedsys > + - "#thermal-sensor-cells" > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/mt8173-clk.h> > + #include <dt-bindings/reset/mt8173-resets.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + auxadc: auxadc@11001000 { > + compatible = "mediatek,mt8173-auxadc"; > + reg = <0 0x11001000 0 0x1000>; > + clocks = <&pericfg CLK_PERI_AUXADC>; > + clock-names = "main"; > + #io-channel-cells = <1>; > + }; > + > + apmixedsys: clock-controller@10209000 { > + compatible = "mediatek,mt8173-apmixedsys"; > + reg = <0 0x10209000 0 0x1000>; > + #clock-cells = <1>; > + }; Drop both examples, not really relevant to thermal. It grows the example with code already documented somewhere else. > + > + thermal: thermal@1100b000 { > + #thermal-sensor-cells = <1>; > + compatible = "mediatek,mt8173-thermal"; > + reg = <0 0x1100b000 0 0x1000>; > + interrupts = <0 70 IRQ_TYPE_LEVEL_LOW>; > + clocks = <&pericfg CLK_PERI_THERM>, <&pericfg CLK_PERI_AUXADC>; > + clock-names = "therm", "auxadc"; > + resets = <&pericfg MT8173_PERI_THERM_SW_RST>; > + reset-names = "therm"; > + mediatek,auxadc = <&auxadc>; > + mediatek,apmixedsys = <&apmixedsys>; > + nvmem-cells = <&thermal_calibration_data>; > + nvmem-cell-names = "calibration-data"; > + }; > + > + thermal-zones { > + cpu_thermal: cpu-thermal { > + polling-delay-passive = <1000>; > + polling-delay = <1000>; > + > + thermal-sensors = <&thermal 0>; > + sustainable-power = <1500>; > + > + trips { > + threshold: trip-point0 { > + temperature = <68000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + > + target: trip-point1 { > + temperature = <85000>; > + hysteresis = <2000>; > + type = "passive"; > + }; > + > + cpu_crit: cpu_crit0 { No underscores in node names. Best regards, Krzysztof
On 31/05/2023 09:57, Krzysztof Kozlowski wrote: > On 30/05/2023 17:04, matthias.bgg@kernel.org wrote: >> From: Matthias Brugger <matthias.bgg@gmail.com> >> >> Convert the older binding to yaml syntax. >> >> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com> >> --- >> >> .../bindings/thermal/mediatek,thermal.yaml | 168 ++++++++++++++++++ >> .../bindings/thermal/mediatek-thermal.txt | 52 ------ >> 2 files changed, 168 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..7aa2bdc43567 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml >> @@ -0,0 +1,168 @@ >> +# 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: | >> + 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. >> + >> +properties: >> + compatible: >> + oneOf: >> + - enum: >> + - mediatek,mt2701-thermal >> + - mediatek,mt2712-thermal >> + - mediatek,mt7622-thermal >> + - mediatek,mt7986-thermal >> + - mediatek,mt8173-thermal >> + - mediatek,mt8183-thermal >> + - mediatek,mt8365-thermal >> + - items: >> + - const: mediatek,mt7981-thermal >> + - const: mediatek,mt7986-thermal >> + - items: >> + - const: mediatek,mt8516-thermal >> + - const: mediatek,mt2701-thermal >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> + clocks: >> + minItems: 2 > > maxItems instead > >> + >> + clock-names: >> + items: >> + - const: therm >> + - const: auxadc >> + >> + resets: >> + maxItems: 1 >> + description: Reference to the reset controller controlling the thermal controller. > > You can drop description, it's obvious. > >> + >> + reset-names: >> + items: >> + - const: therm >> + >> + nvmem-cells: >> + items: >> + - description: Calibration eFuse data. If unspecified default values are used. >> + >> + nvmem-cell-names: >> + items: >> + - const: calibration-data >> + >> + mediatek,auxadc: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + A phandle to the AUXADC which the thermal controller uses. >> + >> + mediatek,apmixedsys: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + A phandle to the APMIXEDSYS controller. >> + >> + "#thermal-sensor-cells": >> + const: 1 >> + >> + bank0-supply: >> + description: Regulator node supplying voltage to the first bank >> + >> + bank1-supply: >> + description: Regulator node supplying voltage to the second bank > > These were not present before. Mention this in commit msg. Also drop "node". > >> + >> + > > Just one blank line. > >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - clocks >> + - mediatek,auxadc >> + - mediatek,apmixedsys >> + - "#thermal-sensor-cells" >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/clock/mt8173-clk.h> >> + #include <dt-bindings/reset/mt8173-resets.h> >> + >> + soc { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + auxadc: auxadc@11001000 { >> + compatible = "mediatek,mt8173-auxadc"; >> + reg = <0 0x11001000 0 0x1000>; >> + clocks = <&pericfg CLK_PERI_AUXADC>; >> + clock-names = "main"; >> + #io-channel-cells = <1>; >> + }; >> + >> + apmixedsys: clock-controller@10209000 { >> + compatible = "mediatek,mt8173-apmixedsys"; >> + reg = <0 0x10209000 0 0x1000>; >> + #clock-cells = <1>; >> + }; > > Drop both examples, not really relevant to thermal. It grows the example > with code already documented somewhere else. > auxadc and apmixedsys are no examples. They are referenced by the thermal node as described in the binding. Without them the example won't be complete. Regards, Matthias >> + >> + thermal: thermal@1100b000 { >> + #thermal-sensor-cells = <1>; >> + compatible = "mediatek,mt8173-thermal"; >> + reg = <0 0x1100b000 0 0x1000>; >> + interrupts = <0 70 IRQ_TYPE_LEVEL_LOW>; >> + clocks = <&pericfg CLK_PERI_THERM>, <&pericfg CLK_PERI_AUXADC>; >> + clock-names = "therm", "auxadc"; >> + resets = <&pericfg MT8173_PERI_THERM_SW_RST>; >> + reset-names = "therm"; >> + mediatek,auxadc = <&auxadc>; >> + mediatek,apmixedsys = <&apmixedsys>; >> + nvmem-cells = <&thermal_calibration_data>; >> + nvmem-cell-names = "calibration-data"; >> + }; >> + >> + thermal-zones { >> + cpu_thermal: cpu-thermal { >> + polling-delay-passive = <1000>; >> + polling-delay = <1000>; >> + >> + thermal-sensors = <&thermal 0>; >> + sustainable-power = <1500>; >> + >> + trips { >> + threshold: trip-point0 { >> + temperature = <68000>; >> + hysteresis = <2000>; >> + type = "passive"; >> + }; >> + >> + target: trip-point1 { >> + temperature = <85000>; >> + hysteresis = <2000>; >> + type = "passive"; >> + }; >> + >> + cpu_crit: cpu_crit0 { > > No underscores in node names. > > > Best regards, > Krzysztof >
On 31/05/2023 14:42, Matthias Brugger wrote: >>> + soc { >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + >>> + auxadc: auxadc@11001000 { >>> + compatible = "mediatek,mt8173-auxadc"; >>> + reg = <0 0x11001000 0 0x1000>; >>> + clocks = <&pericfg CLK_PERI_AUXADC>; >>> + clock-names = "main"; >>> + #io-channel-cells = <1>; >>> + }; >>> + >>> + apmixedsys: clock-controller@10209000 { >>> + compatible = "mediatek,mt8173-apmixedsys"; >>> + reg = <0 0x10209000 0 0x1000>; >>> + #clock-cells = <1>; >>> + }; >> >> Drop both examples, not really relevant to thermal. It grows the example >> with code already documented somewhere else. >> > > auxadc and apmixedsys are no examples. They are referenced by the thermal node > as described in the binding. Without them the example won't be complete. > The example is complete, because you have phandles and that's enough. the auxadc and apmixedsys have their own examples in their own bindings - no need to duplicate them here. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml new file mode 100644 index 000000000000..7aa2bdc43567 --- /dev/null +++ b/Documentation/devicetree/bindings/thermal/mediatek,thermal.yaml @@ -0,0 +1,168 @@ +# 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: | + 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. + +properties: + compatible: + oneOf: + - enum: + - mediatek,mt2701-thermal + - mediatek,mt2712-thermal + - mediatek,mt7622-thermal + - mediatek,mt7986-thermal + - mediatek,mt8173-thermal + - mediatek,mt8183-thermal + - mediatek,mt8365-thermal + - items: + - const: mediatek,mt7981-thermal + - const: mediatek,mt7986-thermal + - items: + - const: mediatek,mt8516-thermal + - const: mediatek,mt2701-thermal + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + minItems: 2 + + clock-names: + items: + - const: therm + - const: auxadc + + resets: + maxItems: 1 + description: Reference to the reset controller controlling the thermal controller. + + reset-names: + items: + - const: therm + + nvmem-cells: + items: + - description: Calibration eFuse data. If unspecified default values are used. + + nvmem-cell-names: + items: + - const: calibration-data + + mediatek,auxadc: + $ref: /schemas/types.yaml#/definitions/phandle + description: + A phandle to the AUXADC which the thermal controller uses. + + mediatek,apmixedsys: + $ref: /schemas/types.yaml#/definitions/phandle + description: + A phandle to the APMIXEDSYS controller. + + "#thermal-sensor-cells": + const: 1 + + bank0-supply: + description: Regulator node supplying voltage to the first bank + + bank1-supply: + description: Regulator node supplying voltage to the second bank + + +required: + - compatible + - reg + - interrupts + - clocks + - mediatek,auxadc + - mediatek,apmixedsys + - "#thermal-sensor-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/clock/mt8173-clk.h> + #include <dt-bindings/reset/mt8173-resets.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + auxadc: auxadc@11001000 { + compatible = "mediatek,mt8173-auxadc"; + reg = <0 0x11001000 0 0x1000>; + clocks = <&pericfg CLK_PERI_AUXADC>; + clock-names = "main"; + #io-channel-cells = <1>; + }; + + apmixedsys: clock-controller@10209000 { + compatible = "mediatek,mt8173-apmixedsys"; + reg = <0 0x10209000 0 0x1000>; + #clock-cells = <1>; + }; + + thermal: thermal@1100b000 { + #thermal-sensor-cells = <1>; + compatible = "mediatek,mt8173-thermal"; + reg = <0 0x1100b000 0 0x1000>; + interrupts = <0 70 IRQ_TYPE_LEVEL_LOW>; + clocks = <&pericfg CLK_PERI_THERM>, <&pericfg CLK_PERI_AUXADC>; + clock-names = "therm", "auxadc"; + resets = <&pericfg MT8173_PERI_THERM_SW_RST>; + reset-names = "therm"; + mediatek,auxadc = <&auxadc>; + mediatek,apmixedsys = <&apmixedsys>; + nvmem-cells = <&thermal_calibration_data>; + nvmem-cell-names = "calibration-data"; + }; + + thermal-zones { + cpu_thermal: cpu-thermal { + polling-delay-passive = <1000>; + polling-delay = <1000>; + + thermal-sensors = <&thermal 0>; + sustainable-power = <1500>; + + trips { + threshold: trip-point0 { + temperature = <68000>; + hysteresis = <2000>; + type = "passive"; + }; + + target: trip-point1 { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + cpu_crit: cpu_crit0 { + temperature = <115000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + }; + }; + }; +... diff --git a/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt b/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt deleted file mode 100644 index ac39c7156fde..000000000000 --- a/Documentation/devicetree/bindings/thermal/mediatek-thermal.txt +++ /dev/null @@ -1,52 +0,0 @@ -* Mediatek Thermal - -This describes the device tree binding for the Mediatek thermal controller -which 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. - -Required properties: -- compatible: - - "mediatek,mt8173-thermal" : For MT8173 family of SoCs - - "mediatek,mt2701-thermal" : For MT2701 family of SoCs - - "mediatek,mt2712-thermal" : For MT2712 family of SoCs - - "mediatek,mt7622-thermal" : For MT7622 SoC - - "mediatek,mt7981-thermal", "mediatek,mt7986-thermal" : For MT7981 SoC - - "mediatek,mt7986-thermal" : For MT7986 SoC - - "mediatek,mt8183-thermal" : For MT8183 family of SoCs - - "mediatek,mt8365-thermal" : For MT8365 family of SoCs - - "mediatek,mt8516-thermal", "mediatek,mt2701-thermal : For MT8516 family of SoCs -- reg: Address range of the thermal controller -- interrupts: IRQ for the thermal controller -- clocks, clock-names: Clocks needed for the thermal controller. required - clocks are: - "therm": Main clock needed for register access - "auxadc": The AUXADC clock -- mediatek,auxadc: A phandle to the AUXADC which the thermal controller uses -- mediatek,apmixedsys: A phandle to the APMIXEDSYS controller. -- #thermal-sensor-cells : Should be 0. See Documentation/devicetree/bindings/thermal/thermal-sensor.yaml for a description. - -Optional properties: -- resets: Reference to the reset controller controlling the thermal controller. -- nvmem-cells: A phandle to the calibration data provided by a nvmem device. If - unspecified default values shall be used. -- nvmem-cell-names: Should be "calibration-data" - -Example: - - thermal: thermal@1100b000 { - #thermal-sensor-cells = <1>; - compatible = "mediatek,mt8173-thermal"; - reg = <0 0x1100b000 0 0x1000>; - interrupts = <0 70 IRQ_TYPE_LEVEL_LOW>; - clocks = <&pericfg CLK_PERI_THERM>, <&pericfg CLK_PERI_AUXADC>; - clock-names = "therm", "auxadc"; - resets = <&pericfg MT8173_PERI_THERM_SW_RST>; - reset-names = "therm"; - mediatek,auxadc = <&auxadc>; - mediatek,apmixedsys = <&apmixedsys>; - nvmem-cells = <&thermal_calibration_data>; - nvmem-cell-names = "calibration-data"; - };