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

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

[Kelvin Zhang via B4 Relay]

Add support for Amlogic S4 PWM, including the driver and DTS.

Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
---
Changes in v8:
- Return dev_err_probe() in meson_pwm_init_channels_s4().
- Check the result of devm_add_action_or_reset().
- Link to v7: https://lore.kernel.org/r/20240605-s4-pwm-v7-0-e822b271d7b0@amlogic.com

Changes in v7:
- Put devm_add_action_or_reset() into the for loop.
- Remove the error handling of meson_pwm_init_channels_s4().
- Remove the repeated device node 'pwm-a-pins'.
- Some minor fixes and improvements.
- Link to v6: https://lore.kernel.org/r/20240529-s4-pwm-v6-0-270f63049f20@amlogic.com

Changes in v6:
- Rename 'pwm_meson_s4_data' to 'pwm_s4_data'.
- Rename 'meson_pwm_init_channels_meson_s4' to 'meson_pwm_init_channels_s4'.
- Adjust the order of the device nodes according to their unit addresses.
- Some minor improvements.
- Link to v5: https://lore.kernel.org/r/20240521-s4-pwm-v5-0-0c91f5fa32cd@amlogic.com

Changes in v5:
- Add devm_add_action_or_reset for free clk when unloading.
- Replace the underscores of node name with dashes.
- Link to v4: https://lore.kernel.org/r/20240424-s4-pwm-v4-0-ee22effd40d0@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 | 199 ++++++++++++++++++++++++++++++
 drivers/pwm/pwm-meson.c                   |  39 ++++++
 2 files changed, 238 insertions(+)
---
base-commit: 9d99040b1bc8dbf385a8aa535e9efcdf94466e19
change-id: 20240424-s4-pwm-2d709986caee

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

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

[Kelvin Zhang via B4 Relay]

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

Add 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 | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
index b2f97dfb01bb..98e6c1533312 100644
--- a/drivers/pwm/pwm-meson.c
+++ b/drivers/pwm/pwm-meson.c
@@ -460,6 +460,37 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
 	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
 }
 
+static void meson_pwm_s4_put_clk(void *data)
+{
+	struct clk *clk = data;
+
+	clk_put(clk);
+}
+
+static int meson_pwm_init_channels_s4(struct pwm_chip *chip)
+{
+	struct device *dev = pwmchip_parent(chip);
+	struct device_node *np = dev->of_node;
+	struct meson_pwm *meson = to_meson_pwm(chip);
+	int i, ret;
+
+	for (i = 0; i < MESON_NUM_PWMS; i++) {
+		meson->channels[i].clk = of_clk_get(np, i);
+		if (IS_ERR(meson->channels[i].clk))
+			return dev_err_probe(dev,
+					     PTR_ERR(meson->channels[i].clk),
+					     "Failed to get clk\n");
+
+		ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
+					       meson->channels[i].clk);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to add clk_put action\n");
+	}
+
+	return 0;
+}
+
 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,
@@ -498,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_s4_data = {
+	.channels_init = meson_pwm_init_channels_s4,
+};
+
 static const struct of_device_id meson_pwm_matches[] = {
 	{
 		.compatible = "amlogic,meson8-pwm-v2",
@@ -536,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_s4_data
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(of, meson_pwm_matches);

-- 
2.37.1

[PATCH v8 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>
Reviewed-by: George Stark <gnstark@salutedevices.com>
Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
---
 arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 199 ++++++++++++++++++++++++++++++
 1 file changed, 199 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
index 10896f9df682..b686eacb9662 100644
--- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
@@ -312,6 +312,160 @@ 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_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";
+					};
+				};
+
+				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_c_hiz";
+					};
+				};
+
+				pwm_g_hiz_pins: pwm-g-hiz-pins {
+					mux {
+						groups = "pwm_g_hiz";
+						function = "pwm_g_hiz";
+					};
+				};
+
 				spicc0_pins_x: spicc0-pins_x {
 					mux {
 						groups = "spi_a_mosi_x",
@@ -399,6 +553,51 @@ spicc0: spi@50000 {
 				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";
+			};
+
 			i2c0: i2c@66000 {
 				compatible = "amlogic,meson-axg-i2c";
 				reg = <0x0 0x66000 0x0 0x20>;

-- 
2.37.1

Re: [DMARC error][DKIM error] [PATCH v8 1/2] pwm: meson: Add support for Amlogic S4 PWM

[George Stark]

If there's another series you can fix log messages and start it from 
low-case letter as in the rest of the file.

Either way
Reviewed-by: George Stark <gnstark@salutedevices.com>

On 6/13/24 14:46, Kelvin Zhang via B4 Relay wrote:
> From: Junyi Zhao <junyi.zhao@amlogic.com>
> 
> Add 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 | 39 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index b2f97dfb01bb..98e6c1533312 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -460,6 +460,37 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
>   	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
>   }
>   
> +static void meson_pwm_s4_put_clk(void *data)
> +{
> +	struct clk *clk = data;
> +
> +	clk_put(clk);
> +}
> +
> +static int meson_pwm_init_channels_s4(struct pwm_chip *chip)
> +{
> +	struct device *dev = pwmchip_parent(chip);
> +	struct device_node *np = dev->of_node;
> +	struct meson_pwm *meson = to_meson_pwm(chip);
> +	int i, ret;
> +
> +	for (i = 0; i < MESON_NUM_PWMS; i++) {
> +		meson->channels[i].clk = of_clk_get(np, i);
> +		if (IS_ERR(meson->channels[i].clk))
> +			return dev_err_probe(dev,
> +					     PTR_ERR(meson->channels[i].clk),
> +					     "Failed to get clk\n");
> +
> +		ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
> +					       meson->channels[i].clk);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to add clk_put action\n");
> +	}
> +
> +	return 0;
> +}
> +
>   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,
> @@ -498,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_s4_data = {
> +	.channels_init = meson_pwm_init_channels_s4,
> +};
> +
>   static const struct of_device_id meson_pwm_matches[] = {
>   	{
>   		.compatible = "amlogic,meson8-pwm-v2",
> @@ -536,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_s4_data
> +	},
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, meson_pwm_matches);
> 

-- 
Best regards
George

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

[Jerome Brunet]

On Thu 13 Jun 2024 at 19:46, Kelvin Zhang via B4 Relay <devnull+kelvin.zhang.amlogic.com@kernel.org> wrote:

> From: Junyi Zhao <junyi.zhao@amlogic.com>
>
> Add 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 | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>
> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index b2f97dfb01bb..98e6c1533312 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c
> @@ -460,6 +460,37 @@ static int meson_pwm_init_channels_meson8b_v2(struct pwm_chip *chip)
>  	return meson_pwm_init_clocks_meson8b(chip, mux_parent_data);
>  }
>  
> +static void meson_pwm_s4_put_clk(void *data)
> +{
> +	struct clk *clk = data;
> +
> +	clk_put(clk);
> +}
> +
> +static int meson_pwm_init_channels_s4(struct pwm_chip *chip)
> +{
> +	struct device *dev = pwmchip_parent(chip);
> +	struct device_node *np = dev->of_node;
> +	struct meson_pwm *meson = to_meson_pwm(chip);
> +	int i, ret;
> +
> +	for (i = 0; i < MESON_NUM_PWMS; i++) {
> +		meson->channels[i].clk = of_clk_get(np, i);
> +		if (IS_ERR(meson->channels[i].clk))
> +			return dev_err_probe(dev,
> +					     PTR_ERR(meson->channels[i].clk),
> +					     "Failed to get clk\n");

here it is ok but ..

> +
> +		ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
> +					       meson->channels[i].clk);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to add clk_put action\n");

... here, devm_add_action_or_reset cannot defer, so dev_err_probe is not useful.
dev_err() if you must print something. Just "return ret;" would be fine
by me

> +	}
> +
> +	return 0;
> +}
> +
>  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,
> @@ -498,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_s4_data = {
> +	.channels_init = meson_pwm_init_channels_s4,
> +};
> +
>  static const struct of_device_id meson_pwm_matches[] = {
>  	{
>  		.compatible = "amlogic,meson8-pwm-v2",
> @@ -536,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_s4_data
> +	},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, meson_pwm_matches);

-- 
Jerome

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

[Uwe Kleine-König]

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

Hello,

On Thu, Jun 13, 2024 at 05:54:31PM +0200, Jerome Brunet wrote:
> > +	for (i = 0; i < MESON_NUM_PWMS; i++) {
> > +		meson->channels[i].clk = of_clk_get(np, i);
> > +		if (IS_ERR(meson->channels[i].clk))
> > +			return dev_err_probe(dev,
> > +					     PTR_ERR(meson->channels[i].clk),
> > +					     "Failed to get clk\n");
> 
> here it is ok but ..
> 
> > +
> > +		ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
> > +					       meson->channels[i].clk);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret,
> > +					     "Failed to add clk_put action\n");
> 
> ... here, devm_add_action_or_reset cannot defer, so dev_err_probe is not useful.
> dev_err() if you must print something. Just "return ret;" would be fine
> by me

I don't agree. dev_err_probe() has several purposes. Only one of them is
handling -EPROBE_DEFER. See also commit
532888a59505da2a3fbb4abac6adad381cedb374.

So yes, please use dev_err_probe() also to handle
devm_add_action_or_reset().

Best regards
Uwe

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

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

[Jerome Brunet]

On Thu 13 Jun 2024 at 22:46, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> Hello,
>
> On Thu, Jun 13, 2024 at 05:54:31PM +0200, Jerome Brunet wrote:
>> > +	for (i = 0; i < MESON_NUM_PWMS; i++) {
>> > +		meson->channels[i].clk = of_clk_get(np, i);
>> > +		if (IS_ERR(meson->channels[i].clk))
>> > +			return dev_err_probe(dev,
>> > +					     PTR_ERR(meson->channels[i].clk),
>> > +					     "Failed to get clk\n");
>> 
>> here it is ok but ..
>> 
>> > +
>> > +		ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
>> > +					       meson->channels[i].clk);
>> > +		if (ret)
>> > +			return dev_err_probe(dev, ret,
>> > +					     "Failed to add clk_put action\n");
>> 
>> ... here, devm_add_action_or_reset cannot defer, so dev_err_probe is not useful.
>> dev_err() if you must print something. Just "return ret;" would be fine
>> by me
>
> I don't agree. dev_err_probe() has several purposes. Only one of them is
> handling -EPROBE_DEFER. See also commit
> 532888a59505da2a3fbb4abac6adad381cedb374.

I was stuck on the -EPROBE_DEFER usage. Thanks for the heads up

>
> So yes, please use dev_err_probe() also to handle
> devm_add_action_or_reset().

My point here is also that devm_add_action_or_reset() can only fail on
memory allocation, like (devm_)kzalloc. Looking around the kernel, we
tend to not add messages for that and just return the error code,
presumably because those same 'out of memory' messages would proliferate
everywhere.

>
> Best regards
> Uwe

-- 
Jerome

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

[Uwe Kleine-König]

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

Hello Jerôme,

On Fri, Jun 14, 2024 at 09:24:28AM +0200, Jerome Brunet wrote:
> My point here is also that devm_add_action_or_reset() can only fail on
> memory allocation, like (devm_)kzalloc. Looking around the kernel, we
> tend to not add messages for that and just return the error code,
> presumably because those same 'out of memory' messages would proliferate
> everywhere.

I took your first message in this thread as opportunity to resend a
patch improving the situation here. See

	https://lore.kernel.org/lkml/3d1e308d45cddf67749522ca42d83f5b4f0b9634.1718311756.git.u.kleine-koenig@baylibre.com/

Best regards
Uwe

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

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

[Junyi Zhao]

On 2024/6/14 15:24, Jerome Brunet wrote:
> [ EXTERNAL EMAIL ]
> 
> On Thu 13 Jun 2024 at 22:46, Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:
> 
>> Hello,
>>
>> On Thu, Jun 13, 2024 at 05:54:31PM +0200, Jerome Brunet wrote:
>>>> +  for (i = 0; i < MESON_NUM_PWMS; i++) {
>>>> +          meson->channels[i].clk = of_clk_get(np, i);
>>>> +          if (IS_ERR(meson->channels[i].clk))
>>>> +                  return dev_err_probe(dev,
>>>> +                                       PTR_ERR(meson->channels[i].clk),
>>>> +                                       "Failed to get clk\n");
>>>
>>> here it is ok but ..
>>>
>>>> +
>>>> +          ret = devm_add_action_or_reset(dev, meson_pwm_s4_put_clk,
>>>> +                                         meson->channels[i].clk);
>>>> +          if (ret)
>>>> +                  return dev_err_probe(dev, ret,
>>>> +                                       "Failed to add clk_put action\n");
>>>
>>> ... here, devm_add_action_or_reset cannot defer, so dev_err_probe is not useful.
>>> dev_err() if you must print something. Just "return ret;" would be fine
>>> by me
>>
>> I don't agree. dev_err_probe() has several purposes. Only one of them is
>> handling -EPROBE_DEFER. See also commit
>> 532888a59505da2a3fbb4abac6adad381cedb374.
> 
> I was stuck on the -EPROBE_DEFER usage. Thanks for the heads up
> 
>>
>> So yes, please use dev_err_probe() also to handle
>> devm_add_action_or_reset().
> 
> My point here is also that devm_add_action_or_reset() can only fail on
> memory allocation, like (devm_)kzalloc. Looking around the kernel, we
> tend to not add messages for that and just return the error code,
> presumably because those same 'out of memory' messages would proliferate
> everywhere.
> 
>>
>> Best regards
>> Uwe
> 
> --
> Jerome

Hi Uwe, I didnt get the clear point.
So, if we need "return ret" directly? or keep "dev_err_probe()" to print?

--
Best regards
Junyi

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

[Uwe Kleine-König]

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

Hello,

On Mon, Jun 17, 2024 at 04:44:13PM +0800, Junyi Zhao wrote:
> > > So yes, please use dev_err_probe() also to handle
> > > devm_add_action_or_reset().
> > 
> > My point here is also that devm_add_action_or_reset() can only fail on
> > memory allocation, like (devm_)kzalloc. Looking around the kernel, we
> > tend to not add messages for that and just return the error code,
> > presumably because those same 'out of memory' messages would proliferate
> > everywhere.
>
> Hi Uwe, I didnt get the clear point.
> So, if we need "return ret" directly? or keep "dev_err_probe()" to print?

Please keep the dev_err_probe(). There is a problem with that approach
(as Jerome pointed out), but that is about to be addressed in driver
core code.

Best regards
Uwe

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

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

[Kelvin Zhang]

On 2024/6/17 22:11, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Jun 17, 2024 at 04:44:13PM +0800, Junyi Zhao wrote:
>>>> So yes, please use dev_err_probe() also to handle
>>>> devm_add_action_or_reset().
>>> My point here is also that devm_add_action_or_reset() can only fail on
>>> memory allocation, like (devm_)kzalloc. Looking around the kernel, we
>>> tend to not add messages for that and just return the error code,
>>> presumably because those same 'out of memory' messages would proliferate
>>> everywhere.
>> Hi Uwe, I didnt get the clear point.
>> So, if we need "return ret" directly? or keep "dev_err_probe()" to print?
> Please keep the dev_err_probe(). There is a problem with that approach
> (as Jerome pointed out), but that is about to be addressed in driver
> core code.
> 
Hi Uwe,
For this patchset, is there anything that needs improvement?
Thanks!

> Best regards
> Uwe

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

[Uwe Kleine-König]

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

Hello Kelvin,

On Tue, Jun 25, 2024 at 05:33:22PM +0800, Kelvin Zhang wrote:
> On 2024/6/17 22:11, Uwe Kleine-König wrote:
> > On Mon, Jun 17, 2024 at 04:44:13PM +0800, Junyi Zhao wrote:
> > > > > So yes, please use dev_err_probe() also to handle
> > > > > devm_add_action_or_reset().
> > > > My point here is also that devm_add_action_or_reset() can only fail on
> > > > memory allocation, like (devm_)kzalloc. Looking around the kernel, we
> > > > tend to not add messages for that and just return the error code,
> > > > presumably because those same 'out of memory' messages would proliferate
> > > > everywhere.
> > > Hi Uwe, I didnt get the clear point.
> > > So, if we need "return ret" directly? or keep "dev_err_probe()" to print?
> > Please keep the dev_err_probe(). There is a problem with that approach
> > (as Jerome pointed out), but that is about to be addressed in driver
> > core code.

FTR, this is done in
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=2f3cfd2f4b7cf3026fe6b9b2a5320cc18f4c184e

> For this patchset, is there anything that needs improvement?

It's on my (not particularily short) todo list to review this patch. I'm
aware there are several people waiting for that one. I intend to work on
this one later this week.

Best regards
Uwe

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

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

[Uwe Kleine-König]

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

Hello,

On Thu, Jun 13, 2024 at 07:46:36PM +0800, 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>
> Reviewed-by: George Stark <gnstark@salutedevices.com>
> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
> ---
>  arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 199 ++++++++++++++++++++++++++++++
>  1 file changed, 199 insertions(+)

What is the merge plan for this patch? Technically it's independent from
driver support (i.e. patch #1 in this series). The obvious options are:

 - I pick it up together with patch #1 via pwm
 - You pick it up via arm-soc

Can I please get an Ack iff you prefer the first option?

Best regards
Uwe

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

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

[Kelvin Zhang]

On 2024/6/27 13:50, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Jun 13, 2024 at 07:46:36PM +0800, 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>
>> Reviewed-by: George Stark<gnstark@salutedevices.com>
>> Signed-off-by: Kelvin Zhang<kelvin.zhang@amlogic.com>
>> ---
>>   arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 199 ++++++++++++++++++++++++++++++
>>   1 file changed, 199 insertions(+)
> What is the merge plan for this patch? Technically it's independent from
> driver support (i.e. patch #1 in this series). The obvious options are:
> 
>   - I pick it up together with patch #1 via pwm
>   - You pick it up via arm-soc
> 
> Can I please get an Ack iff you prefer the first option?
> 
I understand that the DTS part is independent of the driver.

Hi Neil,
Are there any improvements needed for this patch?
If not, will you pick it up?
Thanks very much!

> Best regards
> Uwe

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

[Neil Armstrong]

Hi Uwe,

On 27/06/2024 07:50, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Jun 13, 2024 at 07:46:36PM +0800, 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>
>> Reviewed-by: George Stark <gnstark@salutedevices.com>
>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>> ---
>>   arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 199 ++++++++++++++++++++++++++++++
>>   1 file changed, 199 insertions(+)
> 
> What is the merge plan for this patch? Technically it's independent from
> driver support (i.e. patch #1 in this series). The obvious options are:
> 
>   - I pick it up together with patch #1 via pwm
>   - You pick it up via arm-soc
> 
> Can I please get an Ack iff you prefer the first option?

I can take but I was waiting for the driver part to be merged.

Anyway, I'll take, something less on the list!

Neil
> 
> Best regards
> Uwe

Re: (subset) [PATCH v8 0/2] Add support for Amlogic S4 PWM

[Neil Armstrong]

Hi,

On Thu, 13 Jun 2024 19:46:34 +0800, Kelvin Zhang wrote:
> Add support for Amlogic S4 PWM, including the driver and DTS.
> 
> 

Thanks, Applied to https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git (v6.11/arm64-dt)

[2/2] arm64: dts: amlogic: Add Amlogic S4 PWM
      https://git.kernel.org/amlogic/c/e227c1e14dfe06a54844014c076d11e9cdef87e4

These changes has been applied on the intermediate git tree [1].

The v6.11/arm64-dt branch will then be sent via a formal Pull Request to the Linux SoC maintainers
for inclusion in their intermediate git branches in order to be sent to Linus during
the next merge window, or sooner if it's a set of fixes.

In the cases of fixes, those will be merged in the current release candidate
kernel and as soon they appear on the Linux master branch they will be
backported to the previous Stable and Long-Stable kernels [2].

The intermediate git branches are merged daily in the linux-next tree [3],
people are encouraged testing these pre-release kernels and report issues on the
relevant mailing-lists.

If problems are discovered on those changes, please submit a signed-off-by revert
patch followed by a corrective changeset.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git
[2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git
[3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git

-- 
Neil

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

[Uwe Kleine-König]

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

Hello,

On Thu, Jun 13, 2024 at 07:46:35PM +0800, Kelvin Zhang via B4 Relay wrote:
> From: Junyi Zhao <junyi.zhao@amlogic.com>
> 
> Add support for Amlogic S4 PWM.
> 
> Signed-off-by: Junyi Zhao <junyi.zhao@amlogic.com>
> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>

Applied this patch with George Stark's Reviewed-by: to
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next
.

You signed your patch using gpg, that's fine. However I failed to find
your key to verify that signature. I suggest you to upload your key to
https://keys.openpgp.org/ (note you have to register your email
addresses there to make this useful) and read through
https://korg.docs.kernel.org/pgpkeys.html#submitting-keys-to-the-keyring
.

Best regards
Uwe

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