diff mbox series

[2/2] arm64: dts: mediatek: mt7988: add PWM controller

Message ID 20240213164633.25447-2-zajec5@gmail.com
State New
Headers
Series None |

Commit Message

Rafał Miłecki Feb. 13, 2024, 4:46 p.m. UTC 
  From: Rafał Miłecki <rafal@milecki.pl>

Add binding for on-SoC controller that can control up to 8 PWMs.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

Comments

AngeloGioacchino Del Regno Feb. 14, 2024, 9:09 a.m. UTC | #1 
Il 13/02/24 17:46, Rafał Miłecki ha scritto:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Add binding for on-SoC controller that can control up to 8 PWMs.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> index bba97de4fb44..67007626b5cd 100644
> --- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
> @@ -1,5 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0-only OR MIT
>   
> +#include <dt-bindings/clock/mediatek,mt7988-clk.h>
>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>   
>   / {
> @@ -78,7 +79,7 @@ gic: interrupt-controller@c000000 {
>   			#interrupt-cells = <3>;
>   		};
>   
> -		clock-controller@10001000 {
> +		infracfg: clock-controller@10001000 {
>   			compatible = "mediatek,mt7988-infracfg", "syscon";
>   			reg = <0 0x10001000 0 0x1000>;
>   			#clock-cells = <1>;
> @@ -103,6 +104,24 @@ clock-controller@1001e000 {
>   			#clock-cells = <1>;
>   		};
>   
> +		pwm@10048000 {
> +			compatible = "mediatek,mt7988-pwm";

I can't take this unless there's a driver that supports your device.

Regards,
Angelo

> +			reg = <0 0x10048000 0 0x1000>;
> +			clocks = <&infracfg CLK_INFRA_66M_PWM_BCK>,
> +				 <&infracfg CLK_INFRA_66M_PWM_HCK>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK1>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK2>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK3>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK4>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK5>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK6>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK7>,
> +				 <&infracfg CLK_INFRA_66M_PWM_CK8>;
> +			clock-names = "top", "main", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5", "pwm6",
> +				      "pwm7","pwm8";
> +			#pwm-cells = <2>;
> +		};
> +
>   		clock-controller@11f40000 {
>   			compatible = "mediatek,mt7988-xfi-pll";
>   			reg = <0 0x11f40000 0 0x1000>;
Rafał Miłecki Feb. 14, 2024, 9:24 a.m. UTC | #2 
On 14.02.2024 10:09, AngeloGioacchino Del Regno wrote:
> Il 13/02/24 17:46, Rafał Miłecki ha scritto:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Add binding for on-SoC controller that can control up to 8 PWMs.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>   arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>> index bba97de4fb44..67007626b5cd 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>> +++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>> @@ -1,5 +1,6 @@
>>   // SPDX-License-Identifier: GPL-2.0-only OR MIT
>> +#include <dt-bindings/clock/mediatek,mt7988-clk.h>
>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>>   / {
>> @@ -78,7 +79,7 @@ gic: interrupt-controller@c000000 {
>>               #interrupt-cells = <3>;
>>           };
>> -        clock-controller@10001000 {
>> +        infracfg: clock-controller@10001000 {
>>               compatible = "mediatek,mt7988-infracfg", "syscon";
>>               reg = <0 0x10001000 0 0x1000>;
>>               #clock-cells = <1>;
>> @@ -103,6 +104,24 @@ clock-controller@1001e000 {
>>               #clock-cells = <1>;
>>           };
>> +        pwm@10048000 {
>> +            compatible = "mediatek,mt7988-pwm";
> 
> I can't take this unless there's a driver that supports your device.

I'd argue you should rather look for a documented binding rather than a
(Linux?) driver. Otherwise you would refuse changes that are not
strictly Linux related. DTS files are meant to describe hardware in a
generic way and not be driven by Linux drivers / design.

Example:
We have bindings for "brcm,bcm6345-timer" and "bcm63138-timer" (see
commit e112f2de151b) and DTS files with those bindings.
There is no Linux driver for that hardware block as there is no need
for it.


In this context I'm explaining binding thing with Conor in discussion
on PATCH 1/1. So stay tuned :)
AngeloGioacchino Del Regno Feb. 14, 2024, 9:33 a.m. UTC | #3 
Il 14/02/24 10:24, Rafał Miłecki ha scritto:
> On 14.02.2024 10:09, AngeloGioacchino Del Regno wrote:
>> Il 13/02/24 17:46, Rafał Miłecki ha scritto:
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> Add binding for on-SoC controller that can control up to 8 PWMs.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>>   arch/arm64/boot/dts/mediatek/mt7988a.dtsi | 21 ++++++++++++++++++++-
>>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi 
>>> b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>>> index bba97de4fb44..67007626b5cd 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
>>> @@ -1,5 +1,6 @@
>>>   // SPDX-License-Identifier: GPL-2.0-only OR MIT
>>> +#include <dt-bindings/clock/mediatek,mt7988-clk.h>
>>>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>>>   / {
>>> @@ -78,7 +79,7 @@ gic: interrupt-controller@c000000 {
>>>               #interrupt-cells = <3>;
>>>           };
>>> -        clock-controller@10001000 {
>>> +        infracfg: clock-controller@10001000 {
>>>               compatible = "mediatek,mt7988-infracfg", "syscon";
>>>               reg = <0 0x10001000 0 0x1000>;
>>>               #clock-cells = <1>;
>>> @@ -103,6 +104,24 @@ clock-controller@1001e000 {
>>>               #clock-cells = <1>;
>>>           };
>>> +        pwm@10048000 {
>>> +            compatible = "mediatek,mt7988-pwm";
>>
>> I can't take this unless there's a driver that supports your device.
> 
> I'd argue you should rather look for a documented binding rather than a
> (Linux?) driver. Otherwise you would refuse changes that are not
> strictly Linux related. DTS files are meant to describe hardware in a
> generic way and not be driven by Linux drivers / design.
> 

Of course, devicetree describes hardware - that is pretty much globally known.

As I wrote in the bindings patch, I still anyway want to see the driver part
for this block before deciding if your description of this hardware is correct.

Regards,
Angelo
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
index bba97de4fb44..67007626b5cd 100644
--- a/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt7988a.dtsi
@@ -1,5 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0-only OR MIT
 
+#include <dt-bindings/clock/mediatek,mt7988-clk.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 
 / {
@@ -78,7 +79,7 @@  gic: interrupt-controller@c000000 {
 			#interrupt-cells = <3>;
 		};
 
-		clock-controller@10001000 {
+		infracfg: clock-controller@10001000 {
 			compatible = "mediatek,mt7988-infracfg", "syscon";
 			reg = <0 0x10001000 0 0x1000>;
 			#clock-cells = <1>;
@@ -103,6 +104,24 @@  clock-controller@1001e000 {
 			#clock-cells = <1>;
 		};
 
+		pwm@10048000 {
+			compatible = "mediatek,mt7988-pwm";
+			reg = <0 0x10048000 0 0x1000>;
+			clocks = <&infracfg CLK_INFRA_66M_PWM_BCK>,
+				 <&infracfg CLK_INFRA_66M_PWM_HCK>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK1>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK2>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK3>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK4>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK5>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK6>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK7>,
+				 <&infracfg CLK_INFRA_66M_PWM_CK8>;
+			clock-names = "top", "main", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5", "pwm6",
+				      "pwm7","pwm8";
+			#pwm-cells = <2>;
+		};
+
 		clock-controller@11f40000 {
 			compatible = "mediatek,mt7988-xfi-pll";
 			reg = <0 0x11f40000 0 0x1000>;