[PATCH v1 0/6] pwm: tegra: Cleanups and .get_state()

[PATCH v1 0/6] pwm: tegra: Cleanups and .get_state()

[Uwe Kleine-König]

Hello,

the tegra PWM driver annoyed me for some time. This patch set addresses
some of the concerns. The implementation of .get_state() is the check if
I understood the hardware correctly. If so, the next step is to fix the
rounding in .apply() and/or convert to the waveform callbacks.

The first 5 patches should be fine (unless my understanding of
pm_runtime is wrong, which is quite possible), for the 6th patch I'd
like someone to test it before it gets applied.

Best regards
Uwe

Uwe Kleine-König (6):
  pwm: tegra: Check for match_data being NULL
  pwm: tegra: Make use of dev_err_probe()
  pwm: tegra: Use devm function for pm_runtime_enable()
  pwm: tegra: Simplify using
    devm_reset_control_get_exclusive_deasserted()
  pwm: tegra: Simplify using devm_pwmchip_add()
  pwm: tegra: Implement .get_state()

 drivers/pwm/pwm-tegra.c | 123 ++++++++++++++++++++++++++++------------
 1 file changed, 86 insertions(+), 37 deletions(-)


base-commit: 82dcd68f7246eeacbc2ef614b82e284a50b2b8bb
-- 
2.55.0.11.g153666a7d9bb

[PATCH v1 1/6] pwm: tegra: Check for match_data being NULL

[Uwe Kleine-König]

It's unlikely but not impossible that of_device_get_match_data() returns
NULL. Handle this case instead of triggering a NULL pointer exception.

Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
---
 drivers/pwm/pwm-tegra.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 5cdbe120ba2d..53743f83869a 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -322,6 +322,13 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	int ret;
 
 	soc = of_device_get_match_data(&pdev->dev);
+	if (!soc)
+		/*
+		 * This can only happen if pdev was matched via pdev->name
+		 * (which should not happen today) or in combination with a
+		 * driver override.
+		 */
+		return dev_err_probe(dev, -ENODEV, "Unsupported device\n");
 
 	chip = devm_pwmchip_alloc(&pdev->dev, soc->num_channels, sizeof(*pc));
 	if (IS_ERR(chip))
-- 
2.55.0.11.g153666a7d9bb

Re: [PATCH v1 1/6] pwm: tegra: Check for match_data being NULL

[Mikko Perttunen]

On Tuesday, July 14, 2026 9:02 PM Uwe Kleine-König wrote:
> It's unlikely but not impossible that of_device_get_match_data() returns
> NULL. Handle this case instead of triggering a NULL pointer exception.
> 
> Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
> ---
>  drivers/pwm/pwm-tegra.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 5cdbe120ba2d..53743f83869a 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -322,6 +322,13 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	int ret;
>  
>  	soc = of_device_get_match_data(&pdev->dev);
> +	if (!soc)

Very subjective, but my preference is to have curly braces whenever the
if block is more than one line, for clarity.

> +		/*
> +		 * This can only happen if pdev was matched via pdev->name
> +		 * (which should not happen today) or in combination with a
> +		 * driver override.
> +		 */

I feel like driver_override falls in the realm of 'root can mess with
the system as they feel like but if they don't know what they're doing
they get to keep the pieces'. So adding a check in every driver, or
in practice having a random mix of drivers with and without the check,
doesn't seem necessary to me.

If we actually want to check for this condition, could it be done
centrally instead? I.e. don't call probe if there's no match data and
the driver's match table implies it requires it.

> +		return dev_err_probe(dev, -ENODEV, "Unsupported device\n");

'dev' is not defined (yet).

Thank you
Mikko

>  
>  	chip = devm_pwmchip_alloc(&pdev->dev, soc->num_channels, sizeof(*pc));
>  	if (IS_ERR(chip))
> -- 
> 2.55.0.11.g153666a7d9bb
> 
> 

[PATCH v1 2/6] pwm: tegra: Make use of dev_err_probe()

[Uwe Kleine-König]

Usage of dev_err_probe() is more compact than dev_err()'s, emits the
error code and handles -ENOMEM and -EPROBE_DEFER properly. Benefit from
these improvements.

Also add a few messages in error paths that lacked an output before.

Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
---
 drivers/pwm/pwm-tegra.c | 44 ++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 53743f83869a..dba9a05675e3 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -316,6 +316,7 @@ static const struct pwm_ops tegra_pwm_ops = {
 
 static int tegra_pwm_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct pwm_chip *chip;
 	struct tegra_pwm_chip *pc;
 	const struct tegra_pwm_soc *soc;
@@ -330,7 +331,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 		 */
 		return dev_err_probe(dev, -ENODEV, "Unsupported device\n");
 
-	chip = devm_pwmchip_alloc(&pdev->dev, soc->num_channels, sizeof(*pc));
+	chip = devm_pwmchip_alloc(dev, soc->num_channels, sizeof(*pc));
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 	pc = to_tegra_pwm_chip(chip);
@@ -339,27 +340,36 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 
 	pc->regs = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pc->regs))
+		/*
+		 * devm_platform_ioremap_resource() already emits an error
+		 * message with CONFIG_HAS_IOMEM, so don't emit another message
+		 * here.
+		 */
 		return PTR_ERR(pc->regs);
 
 	platform_set_drvdata(pdev, chip);
 
-	pc->clk = devm_clk_get(&pdev->dev, NULL);
+	pc->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(pc->clk))
-		return PTR_ERR(pc->clk);
+		return dev_err_probe(dev, PTR_ERR(pc->clk), "Failed to get clock\n");
 
-	ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
+	ret = devm_tegra_core_dev_init_opp_table_common(dev);
 	if (ret)
+		/*
+		 * devm_tegra_core_dev_init_opp_table_common() emits an error
+		 * message most of the time, so don't add another.
+		 */
 		return ret;
 
-	pm_runtime_enable(&pdev->dev);
-	ret = pm_runtime_resume_and_get(&pdev->dev);
+	pm_runtime_enable(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
-		return ret;
+		return dev_err_probe(dev, ret, "Failed to runtime resume device\n");
 
 	/* Set maximum frequency of the IP */
-	ret = dev_pm_opp_set_rate(&pdev->dev, ULONG_MAX);
+	ret = dev_pm_opp_set_rate(dev, ULONG_MAX);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "Failed to set max frequency: %d\n", ret);
+		dev_err_probe(dev, ret, "Failed to set max frequency\n");
 		goto put_pm;
 	}
 
@@ -370,8 +380,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	 */
 	pc->clk_rate = clk_get_rate(pc->clk);
 	if (pc->clk_rate < TEGRA_PWM_DEPTH) {
-		dev_err(&pdev->dev, "clock maximum frequency out of range\n");
-		ret = -ERANGE;
+		ret = dev_err_probe(dev, -ERANGE, "Clock maximum frequency out of range\n");
 		goto put_pm;
 	}
 
@@ -379,10 +388,9 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	pc->min_period_ns =
 	    (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1;
 
-	pc->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
+	pc->rst = devm_reset_control_get_exclusive(dev, "pwm");
 	if (IS_ERR(pc->rst)) {
-		ret = PTR_ERR(pc->rst);
-		dev_err(&pdev->dev, "Reset control is not found: %d\n", ret);
+		ret = dev_err_probe(dev, PTR_ERR(pc->rst), "Failed to get reset control\n");
 		goto put_pm;
 	}
 
@@ -392,17 +400,17 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 
 	ret = pwmchip_add(chip);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+		dev_err_probe(dev, ret, "Adding pwmchip failed\n");
 		reset_control_assert(pc->rst);
 		goto put_pm;
 	}
 
-	pm_runtime_put(&pdev->dev);
+	pm_runtime_put(dev);
 
 	return 0;
 put_pm:
-	pm_runtime_put_sync_suspend(&pdev->dev);
-	pm_runtime_force_suspend(&pdev->dev);
+	pm_runtime_put_sync_suspend(dev);
+	pm_runtime_force_suspend(dev);
 	return ret;
 }
 
-- 
2.55.0.11.g153666a7d9bb

Re: [PATCH v1 2/6] pwm: tegra: Make use of dev_err_probe()

[Mikko Perttunen]

On Tuesday, July 14, 2026 9:02 PM Uwe Kleine-König wrote:
> Usage of dev_err_probe() is more compact than dev_err()'s, emits the
> error code and handles -ENOMEM and -EPROBE_DEFER properly. Benefit from
> these improvements.
> 
> Also add a few messages in error paths that lacked an output before.
> 
> Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
> ---
>  drivers/pwm/pwm-tegra.c | 44 ++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 53743f83869a..dba9a05675e3 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -316,6 +316,7 @@ static const struct pwm_ops tegra_pwm_ops = {
>  
>  static int tegra_pwm_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct pwm_chip *chip;
>  	struct tegra_pwm_chip *pc;
>  	const struct tegra_pwm_soc *soc;
> @@ -330,7 +331,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  		 */
>  		return dev_err_probe(dev, -ENODEV, "Unsupported device\n");
>  
> -	chip = devm_pwmchip_alloc(&pdev->dev, soc->num_channels, sizeof(*pc));
> +	chip = devm_pwmchip_alloc(dev, soc->num_channels, sizeof(*pc));
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
>  	pc = to_tegra_pwm_chip(chip);
> @@ -339,27 +340,36 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  
>  	pc->regs = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(pc->regs))
> +		/*
> +		 * devm_platform_ioremap_resource() already emits an error
> +		 * message with CONFIG_HAS_IOMEM, so don't emit another message
> +		 * here.
> +		 */
>  		return PTR_ERR(pc->regs);

Same as 1/6, my preference is for curlies with multiline blocks.

Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>

>  
>  	platform_set_drvdata(pdev, chip);
>  
> -	pc->clk = devm_clk_get(&pdev->dev, NULL);
> +	pc->clk = devm_clk_get(dev, NULL);
>  	if (IS_ERR(pc->clk))
> -		return PTR_ERR(pc->clk);
> +		return dev_err_probe(dev, PTR_ERR(pc->clk), "Failed to get clock\n");
>  
> -	ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
> +	ret = devm_tegra_core_dev_init_opp_table_common(dev);
>  	if (ret)
> +		/*
> +		 * devm_tegra_core_dev_init_opp_table_common() emits an error
> +		 * message most of the time, so don't add another.
> +		 */
>  		return ret;
>  
> -	pm_runtime_enable(&pdev->dev);
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	pm_runtime_enable(dev);
> +	ret = pm_runtime_resume_and_get(dev);
>  	if (ret)
> -		return ret;
> +		return dev_err_probe(dev, ret, "Failed to runtime resume device\n");
>  
>  	/* Set maximum frequency of the IP */
> -	ret = dev_pm_opp_set_rate(&pdev->dev, ULONG_MAX);
> +	ret = dev_pm_opp_set_rate(dev, ULONG_MAX);
>  	if (ret < 0) {
> -		dev_err(&pdev->dev, "Failed to set max frequency: %d\n", ret);
> +		dev_err_probe(dev, ret, "Failed to set max frequency\n");
>  		goto put_pm;
>  	}
>  
> @@ -370,8 +380,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	 */
>  	pc->clk_rate = clk_get_rate(pc->clk);
>  	if (pc->clk_rate < TEGRA_PWM_DEPTH) {
> -		dev_err(&pdev->dev, "clock maximum frequency out of range\n");
> -		ret = -ERANGE;
> +		ret = dev_err_probe(dev, -ERANGE, "Clock maximum frequency out of range\n");
>  		goto put_pm;
>  	}
>  
> @@ -379,10 +388,9 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	pc->min_period_ns =
>  	    (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1;
>  
> -	pc->rst = devm_reset_control_get_exclusive(&pdev->dev, "pwm");
> +	pc->rst = devm_reset_control_get_exclusive(dev, "pwm");
>  	if (IS_ERR(pc->rst)) {
> -		ret = PTR_ERR(pc->rst);
> -		dev_err(&pdev->dev, "Reset control is not found: %d\n", ret);
> +		ret = dev_err_probe(dev, PTR_ERR(pc->rst), "Failed to get reset control\n");
>  		goto put_pm;
>  	}
>  
> @@ -392,17 +400,17 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  
>  	ret = pwmchip_add(chip);
>  	if (ret < 0) {
> -		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> +		dev_err_probe(dev, ret, "Adding pwmchip failed\n");
>  		reset_control_assert(pc->rst);
>  		goto put_pm;
>  	}
>  
> -	pm_runtime_put(&pdev->dev);
> +	pm_runtime_put(dev);
>  
>  	return 0;
>  put_pm:
> -	pm_runtime_put_sync_suspend(&pdev->dev);
> -	pm_runtime_force_suspend(&pdev->dev);
> +	pm_runtime_put_sync_suspend(dev);
> +	pm_runtime_force_suspend(dev);
>  	return ret;
>  }
>  
> -- 
> 2.55.0.11.g153666a7d9bb
> 
> 

[PATCH v1 3/6] pwm: tegra: Use devm function for pm_runtime_enable()

[Uwe Kleine-König]

This simplifies the error paths as pwm_runtime_disable() is called
automatatically by the driver core.

Note that pwm_runtime_disable() is the right function to undo
pm_runtime_enable(); pm_runtime_force_suspend() "should only be used
during system-wide PM transitions to sleep states".

Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
---
 drivers/pwm/pwm-tegra.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index dba9a05675e3..e99e1c5b18c3 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -361,7 +361,10 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 		 */
 		return ret;
 
-	pm_runtime_enable(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
+
 	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to runtime resume device\n");
@@ -410,7 +413,6 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	return 0;
 put_pm:
 	pm_runtime_put_sync_suspend(dev);
-	pm_runtime_force_suspend(dev);
 	return ret;
 }
 
@@ -422,8 +424,6 @@ static void tegra_pwm_remove(struct platform_device *pdev)
 	pwmchip_remove(chip);
 
 	reset_control_assert(pc->rst);
-
-	pm_runtime_force_suspend(&pdev->dev);
 }
 
 static int __maybe_unused tegra_pwm_runtime_suspend(struct device *dev)
-- 
2.55.0.11.g153666a7d9bb

Re: [PATCH v1 3/6] pwm: tegra: Use devm function for pm_runtime_enable()

[Mikko Perttunen]

On Tuesday, July 14, 2026 9:02 PM Uwe Kleine-König wrote:
> This simplifies the error paths as pwm_runtime_disable() is called
> automatatically by the driver core.

Typo, s/ta//

> 
> Note that pwm_runtime_disable() is the right function to undo
> pm_runtime_enable(); pm_runtime_force_suspend() "should only be used
> during system-wide PM transitions to sleep states".
> 
> Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
> ---
>  drivers/pwm/pwm-tegra.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index dba9a05675e3..e99e1c5b18c3 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -361,7 +361,10 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  		 */
>  		return ret;
>  
> -	pm_runtime_enable(dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
>  	ret = pm_runtime_resume_and_get(dev);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to runtime resume device\n");
> @@ -410,7 +413,6 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	return 0;
>  put_pm:
>  	pm_runtime_put_sync_suspend(dev);
> -	pm_runtime_force_suspend(dev);
>  	return ret;
>  }
>  
> @@ -422,8 +424,6 @@ static void tegra_pwm_remove(struct platform_device *pdev)
>  	pwmchip_remove(chip);
>  
>  	reset_control_assert(pc->rst);
> -
> -	pm_runtime_force_suspend(&pdev->dev);

I'm guessing the pm_runtime_force_suspend was there to runtime_suspend
the device if it was on. I agree that pm_runtime_force_suspend is not
the right way to do that, but some replacement would be needed.

Thank you
Mikko

>  }
>  
>  static int __maybe_unused tegra_pwm_runtime_suspend(struct device *dev)
> -- 
> 2.55.0.11.g153666a7d9bb
> 
> 

[PATCH v1 4/6] pwm: tegra: Simplify using devm_reset_control_get_exclusive_deasserted()

[Uwe Kleine-König]

This function ensures the reset is already deasserted at probe time and
asserted at unbind. So the remove function and the error paths in the
probe function can be simplified accordingly.

Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
---
 drivers/pwm/pwm-tegra.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index e99e1c5b18c3..d7f4baa4cd9b 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -108,7 +108,6 @@ struct tegra_pwm_soc {
 
 struct tegra_pwm_chip {
 	struct clk *clk;
-	struct reset_control*rst;
 
 	unsigned long clk_rate;
 	unsigned long min_period_ns;
@@ -319,6 +318,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct pwm_chip *chip;
 	struct tegra_pwm_chip *pc;
+	struct reset_control*rst;
 	const struct tegra_pwm_soc *soc;
 	int ret;
 
@@ -391,20 +391,17 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	pc->min_period_ns =
 	    (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1;
 
-	pc->rst = devm_reset_control_get_exclusive(dev, "pwm");
-	if (IS_ERR(pc->rst)) {
-		ret = dev_err_probe(dev, PTR_ERR(pc->rst), "Failed to get reset control\n");
+	rst = devm_reset_control_get_exclusive_deasserted(dev, "pwm");
+	if (IS_ERR(rst)) {
+		ret = dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset control\n");
 		goto put_pm;
 	}
 
-	reset_control_deassert(pc->rst);
-
 	chip->ops = &tegra_pwm_ops;
 
 	ret = pwmchip_add(chip);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "Adding pwmchip failed\n");
-		reset_control_assert(pc->rst);
 		goto put_pm;
 	}
 
@@ -419,11 +416,8 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 static void tegra_pwm_remove(struct platform_device *pdev)
 {
 	struct pwm_chip *chip = platform_get_drvdata(pdev);
-	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
 
 	pwmchip_remove(chip);
-
-	reset_control_assert(pc->rst);
 }
 
 static int __maybe_unused tegra_pwm_runtime_suspend(struct device *dev)
-- 
2.55.0.11.g153666a7d9bb

Re: [PATCH v1 4/6] pwm: tegra: Simplify using devm_reset_control_get_exclusive_deasserted()

[Philipp Zabel]

On Di, 2026-07-14 at 14:02 +0200, Uwe Kleine-König wrote:
> This function ensures the reset is already deasserted at probe time and
> asserted at unbind. So the remove function and the error paths in the
> probe function can be simplified accordingly.
> 
> Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
> ---
>  drivers/pwm/pwm-tegra.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index e99e1c5b18c3..d7f4baa4cd9b 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -108,7 +108,6 @@ struct tegra_pwm_soc {
>  
>  struct tegra_pwm_chip {
>  	struct clk *clk;
> -	struct reset_control*rst;
>  
>  	unsigned long clk_rate;
>  	unsigned long min_period_ns;
> @@ -319,6 +318,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct pwm_chip *chip;
>  	struct tegra_pwm_chip *pc;
> +	struct reset_control*rst;

You could use this opportunity to add a space between reset_control and
*rst.

>  	const struct tegra_pwm_soc *soc;
>  	int ret;
>  
> @@ -391,20 +391,17 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	pc->min_period_ns =
>  	    (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1;
>  
> -	pc->rst = devm_reset_control_get_exclusive(dev, "pwm");
> -	if (IS_ERR(pc->rst)) {
> -		ret = dev_err_probe(dev, PTR_ERR(pc->rst), "Failed to get reset control\n");
> +	rst = devm_reset_control_get_exclusive_deasserted(dev, "pwm");
> +	if (IS_ERR(rst)) {
> +		ret = dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset control\n");
>  		goto put_pm;
>  	}
>  
> -	reset_control_deassert(pc->rst);
> -
>  	chip->ops = &tegra_pwm_ops;
>  
>  	ret = pwmchip_add(chip);
>  	if (ret < 0) {
>  		dev_err_probe(dev, ret, "Adding pwmchip failed\n");
> -		reset_control_assert(pc->rst);

With this change, pm_runtime_put_sync_suspend() and
pm_runtime_force_suspend() are called before the reset control is
asserted again in the error case. Is this safe?

regards
Philipp

Re: [PATCH v1 4/6] pwm: tegra: Simplify using devm_reset_control_get_exclusive_deasserted()

[Uwe Kleine-König]

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

On Tue, Jul 14, 2026 at 02:12:38PM +0200, Philipp Zabel wrote:
> On Di, 2026-07-14 at 14:02 +0200, Uwe Kleine-König wrote:
> > This function ensures the reset is already deasserted at probe time and
> > asserted at unbind. So the remove function and the error paths in the
> > probe function can be simplified accordingly.
> > 
> > Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
> > ---
> >  drivers/pwm/pwm-tegra.c | 14 ++++----------
> >  1 file changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > index e99e1c5b18c3..d7f4baa4cd9b 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -108,7 +108,6 @@ struct tegra_pwm_soc {
> >  
> >  struct tegra_pwm_chip {
> >  	struct clk *clk;
> > -	struct reset_control*rst;
> >  
> >  	unsigned long clk_rate;
> >  	unsigned long min_period_ns;
> > @@ -319,6 +318,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	struct pwm_chip *chip;
> >  	struct tegra_pwm_chip *pc;
> > +	struct reset_control*rst;
> 
> You could use this opportunity to add a space between reset_control and
> *rst.

Oh indeed. I thought I called checkpatch, but there are two more
warnings in this series that I'm not aware of, so it seems I didn't
check before sending :-o

> >  	const struct tegra_pwm_soc *soc;
> >  	int ret;
> >  
> > @@ -391,20 +391,17 @@ static int tegra_pwm_probe(struct platform_device *pdev)
> >  	pc->min_period_ns =
> >  	    (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1;
> >  
> > -	pc->rst = devm_reset_control_get_exclusive(dev, "pwm");
> > -	if (IS_ERR(pc->rst)) {
> > -		ret = dev_err_probe(dev, PTR_ERR(pc->rst), "Failed to get reset control\n");
> > +	rst = devm_reset_control_get_exclusive_deasserted(dev, "pwm");
> > +	if (IS_ERR(rst)) {
> > +		ret = dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset control\n");
> >  		goto put_pm;
> >  	}
> >  
> > -	reset_control_deassert(pc->rst);
> > -
> >  	chip->ops = &tegra_pwm_ops;
> >  
> >  	ret = pwmchip_add(chip);
> >  	if (ret < 0) {
> >  		dev_err_probe(dev, ret, "Adding pwmchip failed\n");
> > -		reset_control_assert(pc->rst);
> 
> With this change, pm_runtime_put_sync_suspend() and
> pm_runtime_force_suspend() are called before the reset control is
> asserted again in the error case. Is this safe?

Ah, this is true for pm_runtime_put_sync_suspend(). Too bad this isn't
easily fixable :-\ I would expect this not to be a problem, but given
that I don't have the hardware, I guess being conservative here is
needed.

Thanks for catching this.

Uwe

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

Re: [PATCH v1 4/6] pwm: tegra: Simplify using devm_reset_control_get_exclusive_deasserted()

[Mikko Perttunen]

On Tuesday, July 14, 2026 11:07 PM Uwe Kleine-König wrote:
> On Tue, Jul 14, 2026 at 02:12:38PM +0200, Philipp Zabel wrote:
> > On Di, 2026-07-14 at 14:02 +0200, Uwe Kleine-König wrote:
> > > This function ensures the reset is already deasserted at probe time and
> > > asserted at unbind. So the remove function and the error paths in the
> > > probe function can be simplified accordingly.
> > > 
> > > Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
> > > ---
> > >  drivers/pwm/pwm-tegra.c | 14 ++++----------
> > >  1 file changed, 4 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > > index e99e1c5b18c3..d7f4baa4cd9b 100644
> > > --- a/drivers/pwm/pwm-tegra.c
> > > +++ b/drivers/pwm/pwm-tegra.c
> > > @@ -108,7 +108,6 @@ struct tegra_pwm_soc {
> > >  
> > >  struct tegra_pwm_chip {
> > >  	struct clk *clk;
> > > -	struct reset_control*rst;
> > >  
> > >  	unsigned long clk_rate;
> > >  	unsigned long min_period_ns;
> > > @@ -319,6 +318,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
> > >  	struct device *dev = &pdev->dev;
> > >  	struct pwm_chip *chip;
> > >  	struct tegra_pwm_chip *pc;
> > > +	struct reset_control*rst;
> > 
> > You could use this opportunity to add a space between reset_control and
> > *rst.
> 
> Oh indeed. I thought I called checkpatch, but there are two more
> warnings in this series that I'm not aware of, so it seems I didn't
> check before sending :-o
> 
> > >  	const struct tegra_pwm_soc *soc;
> > >  	int ret;
> > >  
> > > @@ -391,20 +391,17 @@ static int tegra_pwm_probe(struct platform_device *pdev)
> > >  	pc->min_period_ns =
> > >  	    (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1;
> > >  
> > > -	pc->rst = devm_reset_control_get_exclusive(dev, "pwm");
> > > -	if (IS_ERR(pc->rst)) {
> > > -		ret = dev_err_probe(dev, PTR_ERR(pc->rst), "Failed to get reset control\n");
> > > +	rst = devm_reset_control_get_exclusive_deasserted(dev, "pwm");
> > > +	if (IS_ERR(rst)) {
> > > +		ret = dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset control\n");
> > >  		goto put_pm;
> > >  	}
> > >  
> > > -	reset_control_deassert(pc->rst);
> > > -
> > >  	chip->ops = &tegra_pwm_ops;
> > >  
> > >  	ret = pwmchip_add(chip);
> > >  	if (ret < 0) {
> > >  		dev_err_probe(dev, ret, "Adding pwmchip failed\n");
> > > -		reset_control_assert(pc->rst);
> > 
> > With this change, pm_runtime_put_sync_suspend() and
> > pm_runtime_force_suspend() are called before the reset control is
> > asserted again in the error case. Is this safe?
> 
> Ah, this is true for pm_runtime_put_sync_suspend(). Too bad this isn't
> easily fixable :-\ I would expect this not to be a problem, but given
> that I don't have the hardware, I guess being conservative here is
> needed.
> 
> Thanks for catching this.
> 
> Uwe

I agree that it's unlikely anything bad would happen in practice
if asserting the reset after disabling the clock, but the proper
sequence indeed to my understanding is the other way around.

Mikko

[PATCH v1 5/6] pwm: tegra: Simplify using devm_pwmchip_add()

[Uwe Kleine-König]

devm_pwmchip_add() makes the device core cope for calling
pwmchip_remove() at unbind time. This way the remove callback becomes
empty and can be dropped.

Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
---
 drivers/pwm/pwm-tegra.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index d7f4baa4cd9b..8e5e7e37f4ff 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -399,7 +399,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 
 	chip->ops = &tegra_pwm_ops;
 
-	ret = pwmchip_add(chip);
+	ret = devm_pwmchip_add(dev, chip);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "Adding pwmchip failed\n");
 		goto put_pm;
@@ -413,13 +413,6 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static void tegra_pwm_remove(struct platform_device *pdev)
-{
-	struct pwm_chip *chip = platform_get_drvdata(pdev);
-
-	pwmchip_remove(chip);
-}
-
 static int __maybe_unused tegra_pwm_runtime_suspend(struct device *dev)
 {
 	struct pwm_chip *chip = dev_get_drvdata(dev);
@@ -497,7 +490,6 @@ static struct platform_driver tegra_pwm_driver = {
 		.pm = &tegra_pwm_pm_ops,
 	},
 	.probe = tegra_pwm_probe,
-	.remove = tegra_pwm_remove,
 };
 
 module_platform_driver(tegra_pwm_driver);
-- 
2.55.0.11.g153666a7d9bb

[PATCH v1 6/6] pwm: tegra: Implement .get_state()

[Uwe Kleine-König]

The registers of the PWM IP are readable. Use that to implement the
.get_state() callback.

Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
---
 drivers/pwm/pwm-tegra.c | 48 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 8e5e7e37f4ff..79bfc7589db8 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -309,8 +309,56 @@ static int tegra_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return err;
 }
 
+static int tegra_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_state *state)
+{
+	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
+	int rc;
+	u32 val;
+
+	rc = pm_runtime_resume_and_get(pwmchip_parent(chip));
+	if (rc)
+		return rc;
+
+	val = tegra_pwm_readl(pwm, pc->soc->enable_reg);
+	if (val & TEGRA_PWM_ENABLE) {
+		u32 scale, pwm0;
+
+		if (pc->soc->enable_reg != TEGRA_PWM_CSR_0)
+			val = tegra_pwm_readl(pwm, TEGRA_PWM_CSR_0);
+
+		scale = (val >> TEGRA_PWM_SCALE_SHIFT) & (((1 << pc->soc->scale_width) - 1));
+		pwm0 = (val >> TEGRA_PWM_DUTY_SHIFT) & (2 * TEGRA_PWM_DEPTH - 1);
+
+		if (pwm0 > TEGRA_PWM_DEPTH)
+			pwm0 = TEGRA_PWM_DEPTH;
+
+		/*
+		 * scale + 1 is at most 1 << 17, TEGRA_PWM_DEPTH is 256, so the
+		 * multiplication for .period doesn't overflow a u64. With
+		 * pwm0 ≤ TEGRA_PWM_DEPTH, .duty_cycle is also fine.
+		 */
+		*state = (struct pwm_state){
+			.period = DIV64_U64_ROUND_UP((u64)(scale + 1) * TEGRA_PWM_DEPTH * NSEC_PER_SEC, pc->clk_rate),
+			.duty_cycle = DIV64_U64_ROUND_UP((u64)(scale + 1) * pwm0 * NSEC_PER_SEC, pc->clk_rate),
+			.polarity = PWM_POLARITY_NORMAL,
+			.enabled = true,
+		};
+
+	} else {
+		*state = (struct pwm_state){
+			.enabled = false,
+		};
+	}
+
+	pm_runtime_put(pwmchip_parent(chip));
+
+	return 0;
+}
+
 static const struct pwm_ops tegra_pwm_ops = {
 	.apply = tegra_pwm_apply,
+	.get_state = tegra_pwm_get_state,
 };
 
 static int tegra_pwm_probe(struct platform_device *pdev)
-- 
2.55.0.11.g153666a7d9bb

Re: [PATCH v1 6/6] pwm: tegra: Implement .get_state()

[Mikko Perttunen]

On Tuesday, July 14, 2026 9:02 PM Uwe Kleine-König wrote:
> The registers of the PWM IP are readable. Use that to implement the
> .get_state() callback.

I swear I was going to implement this after the Tegra264 series was
accepted, but I don't mind this way either.. :)

> 
> Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
> ---
>  drivers/pwm/pwm-tegra.c | 48 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 8e5e7e37f4ff..79bfc7589db8 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -309,8 +309,56 @@ static int tegra_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return err;
>  }
>  
> +static int tegra_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,(((1 << pc->soc->scale_width) - 1))
> +			       struct pwm_state *state)
> +{
> +	struct tegra_pwm_chip *pc = to_tegra_pwm_chip(chip);
> +	int rc;
> +	u32 val;
> +
> +	rc = pm_runtime_resume_and_get(pwmchip_parent(chip));
> +	if (rc)
> +		return rc;
> +
> +	val = tegra_pwm_readl(pwm, pc->soc->enable_reg);
> +	if (val & TEGRA_PWM_ENABLE) {
> +		u32 scale, pwm0;
> +
> +		if (pc->soc->enable_reg != TEGRA_PWM_CSR_0)
> +			val = tegra_pwm_readl(pwm, TEGRA_PWM_CSR_0);
> +
> +		scale = (val >> TEGRA_PWM_SCALE_SHIFT) & (((1 << pc->soc->scale_width) - 1));

There's one unnecessary pair of parentheses.

> +		pwm0 = (val >> TEGRA_PWM_DUTY_SHIFT) & (2 * TEGRA_PWM_DEPTH - 1);
> +
> +		if (pwm0 > TEGRA_PWM_DEPTH)
> +			pwm0 = TEGRA_PWM_DEPTH;
> +
> +		/*
> +		 * scale + 1 is at most 1 << 17, TEGRA_PWM_DEPTH is 256, so the
> +		 * multiplication for .period doesn't overflow a u64. With
> +		 * pwm0 ≤ TEGRA_PWM_DEPTH, .duty_cycle is also fine.
> +		 */
> +		*state = (struct pwm_state){
> +			.period = DIV64_U64_ROUND_UP((u64)(scale + 1) * TEGRA_PWM_DEPTH * NSEC_PER_SEC, pc->clk_rate),
> +			.duty_cycle = DIV64_U64_ROUND_UP((u64)(scale + 1) * pwm0 * NSEC_PER_SEC, pc->clk_rate),
> +			.polarity = PWM_POLARITY_NORMAL,
> +			.enabled = true,
> +		};
> +
> +	} else {
> +		*state = (struct pwm_state){
> +			.enabled = false,
> +		};
> +	}
> +
> +	pm_runtime_put(pwmchip_parent(chip));
> +
> +	return 0;
> +}
> +
>  static const struct pwm_ops tegra_pwm_ops = {
>  	.apply = tegra_pwm_apply,
> +	.get_state = tegra_pwm_get_state,
>  };
>  
>  static int tegra_pwm_probe(struct platform_device *pdev)
> -- 
> 2.55.0.11.g153666a7d9bb
> 
> 

Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>