[PATCH v4 0/2] Add support for Amlogic S4 PWM

[PATCH v4 0/2] Add support for Amlogic S4 PWM

[Kelvin Zhang via B4 Relay]

Add support for Amlogic S4 PWM and related device nodes.

Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
---
Junyi Zhao (2):
      pwm: meson: Add support for Amlogic S4 PWM
      arm64: dts: amlogic: Add Amlogic S4 PWM

 arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 207 ++++++++++++++++++++++++++++++
 drivers/pwm/pwm-meson.c                   |  37 ++++++
 2 files changed, 244 insertions(+)
---
base-commit: a59668a9397e7245b26e9be85d23f242ff757ae8
change-id: 20240424-s4-pwm-2d709986caee

Best regards,
-- 
Kelvin Zhang <kelvin.zhang@amlogic.com>

[PATCH v4 1/2] pwm: meson: Add support for Amlogic S4 PWM

[Kelvin Zhang via B4 Relay]

From: Junyi Zhao <junyi.zhao@amlogic.com>

This patch adds support for Amlogic S4 PWM.

Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
---
 drivers/pwm/pwm-meson.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index ea96c5973488..6abc823745e4 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -462,6 +462,35 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
 	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
 }
 
+static int meson_pwm_init_channels_meson_s4(struct pwm_chip *chip)
+{
+	int i, ret;
+	struct device *dev = pwmchip_parent(chip);
+	struct device_node *np = dev->of_node;
+	struct meson_pwm *meson = to_meson_pwm(chip);
+	struct meson_pwm_channel *channel;
+
+	for (i = 0; i < MESON_NUM_PWMS; i++) {
+		channel = &meson->channels[i];
+		channel->clk = of_clk_get(np, i);
+		if (IS_ERR(channel->clk)) {
+			ret = PTR_ERR(channel->clk);
+			dev_err_probe(dev, ret, "Failed to get clk\n");
+			goto err;
+		}
+	}
+
+	return 0;
+
+err:
+	while (--i >= 0) {
+		channel = &meson->channels[i];
+		clk_put(channel->clk);
+	}
+
+	return ret;
+}
+
 static const struct meson_pwm_data pwm_meson8b_data = {
 	.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
 	.channels_init = meson_pwm_init_channels_meson8b_legacy,
@@ -500,6 +529,10 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
 	.channels_init = meson_pwm_init_channels_meson8b_v2,
 };
 
+static const struct meson_pwm_data pwm_meson_s4_data = {
+	.channels_init = meson_pwm_init_channels_meson_s4,
+};
+
 static const struct of_device_id meson_pwm_matches[] = {
 	{
 		.compatible = "amlogic,meson8-pwm-v2",
@@ -538,6 +571,10 @@ static const struct of_device_id meson_pwm_matches[] = {
 		.compatible = "amlogic,meson-g12a-ao-pwm-cd",
 		.data = &pwm_g12a_ao_cd_data
 	},
+	{
+		.compatible = "amlogic,meson-s4-pwm",
+		.data = &pwm_meson_s4_data
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, meson_pwm_matches);

-- 
2.37.1

[PATCH v4 2/2] arm64: dts: amlogic: Add Amlogic S4 PWM

[Kelvin Zhang via B4 Relay]

From: Junyi Zhao <junyi.zhao@amlogic.com>

Add device nodes for PWM_AB, PWM_CD, PWM_EF, PWM_GH and PWM_IJ
along with GPIO PIN configs of each channel.

Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
---
 arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 207 ++++++++++++++++++++++++++++++
 1 file changed, 207 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
index 10896f9df682..8165b263ab92 100644
--- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
@@ -292,6 +292,168 @@ mux {
 					};
 				};
 
+				pwm_a_pins1: pwm_a_pins1 {
+					mux {
+						groups = "pwm_a_d";
+						function = "pwm_a";
+					};
+				};
+
+				pwm_a_pins2: pwm_a_pins2 {
+					mux {
+						groups = "pwm_a_x";
+						function = "pwm_a";
+					};
+				};
+
+				pwm_a_pins: pwm_a_pins {
+					mux {
+						groups = "pwm_a_d";
+						function = "pwm_a";
+					};
+				};
+
+				pwm_b_pins1: pwm_b_pins1 {
+					mux {
+						groups = "pwm_b_d";
+						function = "pwm_b";
+					};
+				};
+
+				pwm_b_pins2: pwm_b_pins2 {
+					mux {
+						groups = "pwm_b_x";
+						function = "pwm_b";
+					};
+				};
+
+				pwm_c_pins1: pwm_c_pins1 {
+					mux {
+						groups = "pwm_c_d";
+						function = "pwm_c";
+					};
+				};
+
+				pwm_c_pins2: pwm_c_pins2 {
+					mux {
+						groups = "pwm_c_x";
+						function = "pwm_c";
+					};
+				};
+
+				pwm_d_pins1: pwm_d_pins1 {
+					mux {
+						groups = "pwm_d_d";
+						function = "pwm_d";
+					};
+				};
+
+				pwm_d_pins2: pwm_d_pins2 {
+					mux {
+						groups = "pwm_d_h";
+						function = "pwm_d";
+					};
+				};
+
+				pwm_e_pins1: pwm_e_pins1 {
+					mux {
+						groups = "pwm_e_x";
+						function = "pwm_e";
+						drive-strength-microamp = <500>;
+					};
+				};
+
+				pwm_e_pins2: pwm_e_pins2 {
+					mux {
+						groups = "pwm_e_z";
+						function = "pwm_e";
+					};
+				};
+
+				pwm_f_pins1: pwm_f_pins1 {
+					mux {
+						groups = "pwm_f_x";
+						function = "pwm_f";
+					};
+				};
+
+				pwm_f_pins2: pwm_f_pins2 {
+					mux {
+						groups = "pwm_f_z";
+						function = "pwm_f";
+					};
+				};
+
+				pwm_g_pins1: pwm_g_pins1 {
+					mux {
+						groups = "pwm_g_d";
+						function = "pwm_g";
+					};
+				};
+
+				pwm_g_pins2: pwm_g_pins2 {
+					mux {
+						groups = "pwm_g_z";
+						function = "pwm_g";
+					};
+				};
+
+				pwm_h_pins: pwm_h_pins {
+					mux {
+						groups = "pwm_h";
+						function = "pwm_h";
+					};
+				};
+
+				pwm_i_pins1: pwm_i_pins1 {
+					mux {
+						groups = "pwm_i_d";
+						function = "pwm_i";
+					};
+				};
+
+				pwm_i_pins2: pwm_i_pins2 {
+					mux {
+						groups = "pwm_i_h";
+						function = "pwm_i";
+					};
+				};
+
+				pwm_j_pins: pwm_j_pins {
+					mux {
+						groups = "pwm_j";
+						function = "pwm_j";
+					};
+				};
+
+				pwm_a_hiz_pins: pwm_a_hiz_pins {
+					mux {
+						groups = "pwm_a_hiz";
+						function = "pwm_a_hiz";
+					};
+				};
+
+				pwm_b_hiz_pins: pwm_b_hiz_pins {
+					mux {
+						groups = "pwm_b_hiz";
+						function = "pwm_b_hiz";
+					};
+				};
+
+				pwm_c_hiz_pins: pwm_c_hiz_pins {
+					mux {
+						groups = "pwm_c_hiz";
+						function = "pwm_b_hiz";
+					};
+				};
+
+				pwm_g_hiz_pins: pwm_g_hiz_pins {
+					mux {
+						groups = "pwm_g_hiz";
+						function = "pwm_g_hiz";
+					};
+				};
+
 				nand_pins: nand-pins {
 					mux {
 						groups = "emmc_nand_d0",
@@ -449,6 +611,51 @@ i2c4: i2c@6e000 {
 				status = "disabled";
 			};
 
+			pwm_ab: pwm@58000 {
+				compatible = "amlogic,meson-s4-pwm";
+				reg = <0x0 0x58000 0x0 0x24>;
+				clocks = <&clkc_periphs CLKID_PWM_A>,
+						<&clkc_periphs CLKID_PWM_B>;
+				#pwm-cells = <3>;
+				status = "disabled";
+			};
+
+			pwm_cd: pwm@5a000 {
+				compatible = "amlogic,meson-s4-pwm";
+				reg = <0x0 0x5a000 0x0 0x24>;
+				clocks = <&clkc_periphs CLKID_PWM_C>,
+						<&clkc_periphs CLKID_PWM_D>;
+				#pwm-cells = <3>;
+				status = "disabled";
+			};
+
+			pwm_ef: pwm@5c000 {
+				compatible = "amlogic,meson-s4-pwm";
+				reg = <0x0 0x5c000 0x0 0x24>;
+				clocks = <&clkc_periphs CLKID_PWM_E>,
+						<&clkc_periphs CLKID_PWM_F>;
+				#pwm-cells = <3>;
+				status = "disabled";
+			};
+
+			pwm_gh: pwm@5e000 {
+				compatible = "amlogic,meson-s4-pwm";
+				reg = <0x0 0x5e000 0x0 0x24>;
+				clocks = <&clkc_periphs CLKID_PWM_G>,
+						<&clkc_periphs CLKID_PWM_H>;
+				#pwm-cells = <3>;
+				status = "disabled";
+			};
+
+			pwm_ij: pwm@60000 {
+				compatible = "amlogic,meson-s4-pwm";
+				reg = <0x0 0x60000 0x0 0x24>;
+				clocks = <&clkc_periphs CLKID_PWM_I>,
+						<&clkc_periphs CLKID_PWM_J>;
+				#pwm-cells = <3>;
+				status = "disabled";
+			};
+
 			nand: nand-controller@8c800 {
 				compatible = "amlogic,meson-axg-nfc";
 				reg = <0x0 0x8c800 0x0 0x100>, <0x0 0x8c000 0x0 0x4>;

-- 
2.37.1

Re: [PATCH v4 1/2] pwm: meson: Add support for Amlogic S4 PWM

[Jerome Brunet]

On Wed 24 Apr 2024 at 18:28, Kelvin Zhang via B4 Relay <devnull+kelvin.zhang.amlogic.com@kernel.org> wrote:

> From: Junyi Zhao <junyi.zhao@amlogic.com>
>
> This patch adds support for Amlogic S4 PWM.
>
> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
> ---
>  drivers/pwm/pwm-meson.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index ea96c5973488..6abc823745e4 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -462,6 +462,35 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
>  	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
>  }
>  
> +static int meson_pwm_init_channels_meson_s4(struct pwm_chip *chip)
> +{
> +	int i, ret;
> +	struct device *dev = pwmchip_parent(chip);
> +	struct device_node *np = dev->of_node;
> +	struct meson_pwm *meson = to_meson_pwm(chip);
> +	struct meson_pwm_channel *channel;
> +
> +	for (i = 0; i < MESON_NUM_PWMS; i++) {
> +		channel = &meson->channels[i];
> +		channel->clk = of_clk_get(np, i);
> +		if (IS_ERR(channel->clk)) {
> +			ret = PTR_ERR(channel->clk);
> +			dev_err_probe(dev, ret, "Failed to get clk\n");
> +			goto err;
> +		}
> +	}
> +
> +	return 0;
> +
> +err:
> +	while (--i >= 0) {
> +		channel = &meson->channels[i];
> +		clk_put(channel->clk);

Fine on error but leaks on module unload.

Same as George,

Add the devm variant of of_clk_get() if you must.
Use devm_add_action_or_reset() otherwise

Could please synchronize this series with George and deal with all the
supported SoCs ? a1, s4, t7, c3 ...

> +	}
> +
> +	return ret;
> +}
> +
>  static const struct meson_pwm_data pwm_meson8b_data = {
>  	.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
>  	.channels_init = meson_pwm_init_channels_meson8b_legacy,
> @@ -500,6 +529,10 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
>  	.channels_init = meson_pwm_init_channels_meson8b_v2,
>  };
>  
> +static const struct meson_pwm_data pwm_meson_s4_data = {
> +	.channels_init = meson_pwm_init_channels_meson_s4,
> +};
> +
>  static const struct of_device_id meson_pwm_matches[] = {
>  	{
>  		.compatible = "amlogic,meson8-pwm-v2",
> @@ -538,6 +571,10 @@ static const struct of_device_id meson_pwm_matches[] = {
>  		.compatible = "amlogic,meson-g12a-ao-pwm-cd",
>  		.data = &pwm_g12a_ao_cd_data
>  	},
> +	{
> +		.compatible = "amlogic,meson-s4-pwm",
> +		.data = &pwm_meson_s4_data
> +	},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, meson_pwm_matches);


-- 
Jerome

Re: [PATCH v4 1/2] pwm: meson: Add support for Amlogic S4 PWM

[Junyi Zhao]

On 2024/4/24 18:32, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> On Wed 24 Apr 2024 at 18:28, Kelvin Zhang via B4 Relay <devnull+kelvin.zhang.amlogic.com@kernel.org> wrote:
> 
>> From: Junyi Zhao <junyi.zhao@amlogic.com>
>>
>> This patch adds support for Amlogic S4 PWM.
>>
>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>> ---
>>   drivers/pwm/pwm-meson.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index ea96c5973488..6abc823745e4 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -462,6 +462,35 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
>>        return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
>>   }
>>
>> +static int meson_pwm_init_channels_meson_s4(struct pwm_chip *chip)
>> +{
>> +     int i, ret;
>> +     struct device *dev = pwmchip_parent(chip);
>> +     struct device_node *np = dev->of_node;
>> +     struct meson_pwm *meson = to_meson_pwm(chip);
>> +     struct meson_pwm_channel *channel;
>> +
>> +     for (i = 0; i < MESON_NUM_PWMS; i++) {
>> +             channel = &meson->channels[i];
>> +             channel->clk = of_clk_get(np, i);
>> +             if (IS_ERR(channel->clk)) {
>> +                     ret = PTR_ERR(channel->clk);
>> +                     dev_err_probe(dev, ret, "Failed to get clk\n");
>> +                     goto err;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +
>> +err:
>> +     while (--i >= 0) {
>> +             channel = &meson->channels[i];
>> +             clk_put(channel->clk);
> 
> Fine on error but leaks on module unload.
> 
> Same as George,
> 
> Add the devm variant of of_clk_get() if you must.
> Use devm_add_action_or_reset() otherwise
Hi jerom，but we have discussed before.devm variant such as follows：
devm_clk_get_enable(struct device * dev, char * id)
struct clk *devm_clk_get(struct device *dev, const char *id)
struct clk *devm_clk_get_optional(struct device *dev, const char *id)

after i check api parm ,these api's 2rd parm "id" is string not index.
because dt binding have no name property. could we use devm？
> 
> Could please synchronize this series with George and deal with all the
> supported SoCs ? a1, s4, t7, c3 ...
> 
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>>   static const struct meson_pwm_data pwm_meson8b_data = {
>>        .parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
>>        .channels_init = meson_pwm_init_channels_meson8b_legacy,
>> @@ -500,6 +529,10 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
>>        .channels_init = meson_pwm_init_channels_meson8b_v2,
>>   };
>>
>> +static const struct meson_pwm_data pwm_meson_s4_data = {
>> +     .channels_init = meson_pwm_init_channels_meson_s4,
>> +};
>> +
>>   static const struct of_device_id meson_pwm_matches[] = {
>>        {
>>                .compatible = "amlogic,meson8-pwm-v2",
>> @@ -538,6 +571,10 @@ static const struct of_device_id meson_pwm_matches[] = {
>>                .compatible = "amlogic,meson-g12a-ao-pwm-cd",
>>                .data = &pwm_g12a_ao_cd_data
>>        },
>> +     {
>> +             .compatible = "amlogic,meson-s4-pwm",
>> +             .data = &pwm_meson_s4_data
>> +     },
>>        {},
>>   };
>>   MODULE_DEVICE_TABLE(of, meson_pwm_matches);
> 
> 
> --
> Jerome

Re: [PATCH v4 1/2] pwm: meson: Add support for Amlogic S4 PWM

[Uwe Kleine-König]

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

Hello,

On Wed, Apr 24, 2024 at 12:32:36PM +0200, Jerome Brunet wrote:
> > +err:
> > +	while (--i >= 0) {
> > +		channel = &meson->channels[i];
> > +		clk_put(channel->clk);
> 
> Fine on error but leaks on module unload.
> 
> Same as George,
> 
> Add the devm variant of of_clk_get() if you must.

There shouldn't be a reason to still use of_clk_get(). I'd expect that
devm_clk_get() should do the job and if not that's a bug.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v4 1/2] pwm: meson: Add support for Amlogic S4 PWM

[Jerome Brunet]

On Wed 24 Apr 2024 at 15:51, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> [[PGP Signed Part:Undecided]]
> Hello,
>
> On Wed, Apr 24, 2024 at 12:32:36PM +0200, Jerome Brunet wrote:
>> > +err:
>> > +	while (--i >= 0) {
>> > +		channel = &meson->channels[i];
>> > +		clk_put(channel->clk);
>> 
>> Fine on error but leaks on module unload.
>> 
>> Same as George,
>> 
>> Add the devm variant of of_clk_get() if you must.
>
> There shouldn't be a reason to still use of_clk_get(). I'd expect that
> devm_clk_get() should do the job and if not that's a bug.

Getting a clock ressource by index instead of by name is a reason.

>
> Best regards
> Uwe


-- 
Jerome

Re: [PATCH v4 1/2] pwm: meson: Add support for Amlogic S4 PWM

[George Stark]

Hello Jerome

On 4/24/24 13:32, Jerome Brunet wrote:
> 
> On Wed 24 Apr 2024 at 18:28, Kelvin Zhang via B4 Relay <devnull+kelvin.zhang.amlogic.com@kernel.org> wrote:
> 
>> From: Junyi Zhao <junyi.zhao@amlogic.com>
>>
>> This patch adds support for Amlogic S4 PWM.
>>
>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>> ---
>>   drivers/pwm/pwm-meson.c | 37 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 37 insertions(+)
>>
>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>> index ea96c5973488..6abc823745e4 100644
>> --- a/drivers/pwm/pwm-meson.c
>> +++ b/drivers/pwm/pwm-meson.c
>> @@ -462,6 +462,35 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
>>   	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
>>   }
>>   
>> +static int meson_pwm_init_channels_meson_s4(struct pwm_chip *chip)
>> +{
>> +	int i, ret;
>> +	struct device *dev = pwmchip_parent(chip);
>> +	struct device_node *np = dev->of_node;
>> +	struct meson_pwm *meson = to_meson_pwm(chip);
>> +	struct meson_pwm_channel *channel;
>> +
>> +	for (i = 0; i < MESON_NUM_PWMS; i++) {
>> +		channel = &meson->channels[i];
>> +		channel->clk = of_clk_get(np, i);
>> +		if (IS_ERR(channel->clk)) {
>> +			ret = PTR_ERR(channel->clk);
>> +			dev_err_probe(dev, ret, "Failed to get clk\n");
>> +			goto err;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +
>> +err:
>> +	while (--i >= 0) {
>> +		channel = &meson->channels[i];
>> +		clk_put(channel->clk);
> 
> Fine on error but leaks on module unload.
> 
> Same as George,
> 
> Add the devm variant of of_clk_get() if you must.
> Use devm_add_action_or_reset() otherwise
> 
> Could please synchronize this series with George and deal with all the
> supported SoCs ? a1, s4, t7, c3 ...

If the chipmaker eagers to support s4 himself we're ok :)
But since I sent my patch first I think it'd be fair if this
single patch have my tag:
Co-Developed-by: George Stark <gnstark@salutedevices.com>

I'll help to review the patch too.

Jerome could we split support for all mentioned socs into different series?
e.g.
1. Junyi finishes the driver's base patch and s4 dtsi patch
2. I send a1 dt-bindings and a1 dtsi patches
3. Someone later sends t7/c3 dt-bindings + dtsi

The reason is to apply what we have on hand now due to meson-pwm is 
under heavy development more than a year already.

> 
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   static const struct meson_pwm_data pwm_meson8b_data = {
>>   	.parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
>>   	.channels_init = meson_pwm_init_channels_meson8b_legacy,
>> @@ -500,6 +529,10 @@ static const struct meson_pwm_data pwm_meson8_v2_data = {
>>   	.channels_init = meson_pwm_init_channels_meson8b_v2,
>>   };
>>   
>> +static const struct meson_pwm_data pwm_meson_s4_data = {
>> +	.channels_init = meson_pwm_init_channels_meson_s4,
>> +};
>> +
>>   static const struct of_device_id meson_pwm_matches[] = {
>>   	{
>>   		.compatible = "amlogic,meson8-pwm-v2",
>> @@ -538,6 +571,10 @@ static const struct of_device_id meson_pwm_matches[] = {
>>   		.compatible = "amlogic,meson-g12a-ao-pwm-cd",
>>   		.data = &pwm_g12a_ao_cd_data
>>   	},
>> +	{
>> +		.compatible = "amlogic,meson-s4-pwm",
>> +		.data = &pwm_meson_s4_data
>> +	},
>>   	{},
>>   };
>>   MODULE_DEVICE_TABLE(of, meson_pwm_matches);
> 
> 

-- 
Best regards
George

Re: [PATCH v4 2/2] arm64: dts: amlogic: Add Amlogic S4 PWM

[Krzysztof Kozlowski]

On 24/04/2024 12:28, Kelvin Zhang via B4 Relay wrote:
> From: Junyi Zhao <junyi.zhao@amlogic.com>
> 
> Add device nodes for PWM_AB, PWM_CD, PWM_EF, PWM_GH and PWM_IJ
> along with GPIO PIN configs of each channel.
> 
> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
> ---
>  arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 207 ++++++++++++++++++++++++++++++
>  1 file changed, 207 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
> index 10896f9df682..8165b263ab92 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
> @@ -292,6 +292,168 @@ mux {
>  					};
>  				};
>  
> +				pwm_a_pins1: pwm_a_pins1 {

No underscores in node names.

Best regards,
Krzysztof

Re: [PATCH v4 1/2] pwm: meson: Add support for Amlogic S4 PWM

[George Stark]

Hello Junyi, Kelvin

I'm sorry for the ping. Do you have plans to finish these patches?

Here is sample code how devm could be used to manage clk objects created
by of_clk_get:

struct clk_set_devres {
	struct clk *clks[MESON_NUM_PWMS];
};

static void devm_clk_set_release(struct device *dev, void *res)
{
	struct clk_set_devres *devres = res;
	int num_clks = MESON_NUM_PWMS;

	while (--num_clks >= 0)
		clk_put(devres->clks[num_clks]);
}

static int meson_pwm_init_channels_s4(struct pwm_chip *chip)
{
	struct device *dev = pwmchip_parent(chip);
	struct meson_pwm *meson = to_meson_pwm(chip);
	struct clk_set_devres *devres;
	unsigned int i;
	int res;

	devres = devres_alloc(devm_clk_set_release,
			      sizeof(*devres), GFP_KERNEL);
	if (!devres)
		return -ENOMEM;

	devres_add(dev, devres);

	for (i = 0; i < MESON_NUM_PWMS; i++) {
		devres->clks[i] = of_clk_get(dev->of_node, i);
		if (IS_ERR(devres->clks[i])) {
			res = PTR_ERR(devres->clks[i]);
			dev_err_probe(dev, res, "Failed to get clk\n");
			return res;
		}
		meson->channels[i].clk = devres->clks[i];
	}
	return 0;
}

On 4/24/24 14:44, Junyi Zhao wrote:
> 
> 
> On 2024/4/24 18:32, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>> On Wed 24 Apr 2024 at 18:28, Kelvin Zhang via B4 Relay 
>> <devnull+kelvin.zhang.amlogic.com@kernel.org> wrote:
>>
>>> From: Junyi Zhao <junyi.zhao@amlogic.com>
>>>
>>> This patch adds support for Amlogic S4 PWM.
>>>
>>> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
>>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>>> ---
>>>   drivers/pwm/pwm-meson.c | 37 +++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
>>> index ea96c5973488..6abc823745e4 100644
>>> --- a/drivers/pwm/pwm-meson.c
>>> +++ b/drivers/pwm/pwm-meson.c
>>> @@ -462,6 +462,35 @@ static int 
>>> meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
>>>        return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
>>>   }
>>>
>>> +static int meson_pwm_init_channels_meson_s4(struct pwm_chip *chip)
>>> +{
>>> +     int i, ret;
>>> +     struct device *dev = pwmchip_parent(chip);
>>> +     struct device_node *np = dev->of_node;
>>> +     struct meson_pwm *meson = to_meson_pwm(chip);
>>> +     struct meson_pwm_channel *channel;
>>> +
>>> +     for (i = 0; i < MESON_NUM_PWMS; i++) {
>>> +             channel = &meson->channels[i];
>>> +             channel->clk = of_clk_get(np, i);
>>> +             if (IS_ERR(channel->clk)) {
>>> +                     ret = PTR_ERR(channel->clk);
>>> +                     dev_err_probe(dev, ret, "Failed to get clk\n");
>>> +                     goto err;
>>> +             }
>>> +     }
>>> +
>>> +     return 0;
>>> +
>>> +err:
>>> +     while (--i >= 0) {
>>> +             channel = &meson->channels[i];
>>> +             clk_put(channel->clk);
>>
>> Fine on error but leaks on module unload.
>>
>> Same as George,
>>
>> Add the devm variant of of_clk_get() if you must.
>> Use devm_add_action_or_reset() otherwise
> Hi jerom，but we have discussed before.devm variant such as follows：
> devm_clk_get_enable(struct device * dev, char * id)
> struct clk *devm_clk_get(struct device *dev, const char *id)
> struct clk *devm_clk_get_optional(struct device *dev, const char *id)
> 
> after i check api parm ,these api's 2rd parm "id" is string not index.
> because dt binding have no name property. could we use devm？
>>
>> Could please synchronize this series with George and deal with all the
>> supported SoCs ? a1, s4, t7, c3 ...
>>
>>> +     }
>>> +
>>> +     return ret;
>>> +}
>>> +
>>>   static const struct meson_pwm_data pwm_meson8b_data = {
>>>        .parent_names = { "xtal", NULL, "fclk_div4", "fclk_div3" },
>>>        .channels_init = meson_pwm_init_channels_meson8b_legacy,
>>> @@ -500,6 +529,10 @@ static const struct meson_pwm_data 
>>> pwm_meson8_v2_data = {
>>>        .channels_init = meson_pwm_init_channels_meson8b_v2,
>>>   };
>>>
>>> +static const struct meson_pwm_data pwm_meson_s4_data = {
>>> +     .channels_init = meson_pwm_init_channels_meson_s4,
>>> +};
>>> +
>>>   static const struct of_device_id meson_pwm_matches[] = {
>>>        {
>>>                .compatible = "amlogic,meson8-pwm-v2",
>>> @@ -538,6 +571,10 @@ static const struct of_device_id 
>>> meson_pwm_matches[] = {
>>>                .compatible = "amlogic,meson-g12a-ao-pwm-cd",
>>>                .data = &pwm_g12a_ao_cd_data
>>>        },
>>> +     {
>>> +             .compatible = "amlogic,meson-s4-pwm",
>>> +             .data = &pwm_meson_s4_data
>>> +     },
>>>        {},
>>>   };
>>>   MODULE_DEVICE_TABLE(of, meson_pwm_matches);
>>
>>
>> -- 
>> Jerome

-- 
Best regards
George