diff mbox series

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

Message ID 20240613-s4-pwm-v8-1-b5bd0a768282@amlogic.com (mailing list archive)
State Under Review
Headers show
Series Add support for Amlogic S4 PWM | expand

Commit Message

Kelvin Zhang via B4 Relay June 13, 2024, 11:46 a.m. UTC
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(+)

Comments

George Stark June 13, 2024, 3:03 p.m. UTC | #1
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);
>
Jerome Brunet June 13, 2024, 3:54 p.m. UTC | #2
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);
Uwe Kleine-König June 13, 2024, 8:46 p.m. UTC | #3
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
Jerome Brunet June 14, 2024, 7:24 a.m. UTC | #4
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
Uwe Kleine-König June 14, 2024, 9:02 a.m. UTC | #5
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
Junyi Zhao June 17, 2024, 8:44 a.m. UTC | #6
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
Uwe Kleine-König June 17, 2024, 2:11 p.m. UTC | #7
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
Kelvin Zhang June 25, 2024, 9:33 a.m. UTC | #8
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
Uwe Kleine-König June 25, 2024, 2:50 p.m. UTC | #9
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
Uwe Kleine-König June 27, 2024, 8:16 a.m. UTC | #10
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
diff mbox series

Patch

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);