[PATCH v2 0/3] pwm: tegra: Cleanups and .get_state()

[PATCH v2 0/3] pwm: tegra: Cleanups and .get_state()

[Uwe Kleine-König]

Hello,

v1 of this series can be found at
https://lore.kernel.org/cover.1784030076.git.ukleinek@kernel.org.

Changes since then:

 - Reordered the patches to have dev_err_probe and dev first. Fixes a
   build failure in the middle of v1. This way patch 2 -- which could be
   considered a fix -- isn't before the cleanup in patch 1, but doing
   patch 1 the old way first also feels strange.

 - add { } around blocks with a single statement if there is also a
   comment.

 - fixed too many parenthesis in patch #3 (formerly #6).

 - dropped other patches as they reorder stuff in unwanted or at least
   untested ways.

There was a concern in reply to patch #1 of the v1 series (now #2) from
Mikko Perttunen. He wrote: 

> 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.

It cannot be done reliably in the driver core, and IMHO even root
shouldn't be able to trigger a NULL pointer exception. So I kept the
check.

Uwe Kleine-König (3):
  pwm: tegra: Make use of dev_err_probe()
  pwm: tegra: Check for match_data being NULL
  pwm: tegra: Implement .get_state()

 drivers/pwm/pwm-tegra.c | 108 ++++++++++++++++++++++++++++++++--------
 1 file changed, 87 insertions(+), 21 deletions(-)


base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
-- 
2.47.3

[PATCH v2 1/3] 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.

Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-tegra.c | 52 ++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index 5cdbe120ba2d..efb7ab60f602 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -316,14 +316,15 @@ 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;
 	int ret;
 
-	soc = of_device_get_match_data(&pdev->dev);
+	soc = of_device_get_match_data(dev);
 
-	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);
@@ -331,28 +332,39 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	pc->soc = soc;
 
 	pc->regs = devm_platform_ioremap_resource(pdev, 0);
-	if (IS_ERR(pc->regs))
+	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);
-	if (ret)
+	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;
 	}
 
@@ -363,8 +375,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;
 	}
 
@@ -372,10 +383,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;
 	}
 
@@ -385,17 +395,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.47.3

[PATCH v2 2/3] 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 <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-tegra.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
index efb7ab60f602..b461d3877f43 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -323,6 +323,14 @@ static int tegra_pwm_probe(struct platform_device *pdev)
 	int ret;
 
 	soc = of_device_get_match_data(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(dev, soc->num_channels, sizeof(*pc));
 	if (IS_ERR(chip))
-- 
2.47.3

[PATCH v2 3/3] 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.

Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>
Tested-by: Ola Chr. Vaage <ola.christoffer.vage@scoutdi.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 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 b461d3877f43..0520d025c776 100644
--- a/drivers/pwm/pwm-tegra.c
+++ b/drivers/pwm/pwm-tegra.c
@@ -310,8 +310,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.47.3

Re: [PATCH v2 1/3] pwm: tegra: Make use of dev_err_probe()

[Thierry Reding]

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

On Fri, Sep 18, 2026 at 04:33:45PM +0200, 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.
> 
> Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/pwm/pwm-tegra.c | 52 ++++++++++++++++++++++++-----------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 5cdbe120ba2d..efb7ab60f602 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -316,14 +316,15 @@ 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;
>  	int ret;
>  
> -	soc = of_device_get_match_data(&pdev->dev);
> +	soc = of_device_get_match_data(dev);
>  
> -	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);
> @@ -331,28 +332,39 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	pc->soc = soc;
>  
>  	pc->regs = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(pc->regs))
> +	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.
> +		 */

Seems a bit counter-productive to leave comments like this. Function
comments should document what the function does and then people should
read those comments. Then we don't need to mention it every time we call
these functions.

>  		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);
> -	if (ret)
> +	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.
> +		 */

Same here.

> @@ -385,17 +395,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");

This stands out as very different from other error messages, so maybe
change this to something like "Failed to add PWM chip" for consistency?

Thierry

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

Re: [PATCH v2 2/3] pwm: tegra: Check for match_data being NULL

[Thierry Reding]

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

On Fri, Sep 18, 2026 at 04:33:46PM +0200, 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 <u.kleine-koenig@baylibre.com>
> ---
>  drivers/pwm/pwm-tegra.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index efb7ab60f602..b461d3877f43 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -323,6 +323,14 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	int ret;
>  
>  	soc = of_device_get_match_data(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");
> +	}

We don't usually do this. Matching via anything other than OF device ID
tables (or ACPI, I suppose) is a programming error and you deserve the
crash which forces you to fix things rather than continue with an error
that is easy to miss.

Driver overrides aren't going to work with these devices anyway, so I'm
beginning to think it might be worth looking into opting out of the
override behaviour for select drivers.

Thierry

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

Re: [PATCH v2 3/3] pwm: tegra: Implement .get_state()

[Thierry Reding]

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

On Fri, Sep 18, 2026 at 04:33:47PM +0200, Uwe Kleine-König wrote:
> The registers of the PWM IP are readable. Use that to implement the
> .get_state() callback.
> 
> Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>
> Tested-by: Ola Chr. Vaage <ola.christoffer.vage@scoutdi.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  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 b461d3877f43..0520d025c776 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -310,8 +310,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;

It feels like this has too many assumptions built-in. That's mostly a
predefined issue, but I think if we want to get accurate hardware read-
out, we need to address this.

According to the register documentation, the PWM depth is 16 bits wide
(on generations where it can be programmed). The value defaults to 255
(which is n - 1 encoded, hence TEGRA_PWM_DEPTH), but it can technically
be reprogrammed to any 16-bit value, as far as I can tell.

So I think for this to be correct we'd need to read out the actual value
before overwriting with TEGRA_PWM_CSR_0 contents above. At that point I
think we'd need to either adjust the mask to be (2 * depth) - 1, or
maybe better yet, avoid masking it out arbitrarily based on the depth
and instead cap it at depth so we never exceed the 1:1 ratio for duty
cycle vs. period.

> +
> +		/*
> +		 * 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),

Maybe as part of the above this can be broken down a bit to make it
easier to read? Something like:

	u64 duty_cycle = (u64)(scale + 1) * pwm0 * NSEC_PER_SEC;
	u64 period = (u64)(scale + 1) * depth * NSEC_PER_SEC;

	...

	*state = (struct pwm_state) {
		.period = DIV64_U64_ROUND_UP(period, pc->clk_rate),
		.duty_cycle = DIV64_U64_ROUND_UP(duty_cycle, pc->clk_rate),
		...
	};

is a bit easier on the eye and would make checkpatch happy (or happier).

Thierry

> +			.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.47.3
> 

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

Re: [PATCH v2 0/3] pwm: tegra: Cleanups and .get_state()

[Thierry Reding]

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

On Fri, Sep 18, 2026 at 04:33:44PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> v1 of this series can be found at
> https://lore.kernel.org/cover.1784030076.git.ukleinek@kernel.org.
> 
> Changes since then:
> 
>  - Reordered the patches to have dev_err_probe and dev first. Fixes a
>    build failure in the middle of v1. This way patch 2 -- which could be
>    considered a fix -- isn't before the cleanup in patch 1, but doing
>    patch 1 the old way first also feels strange.
> 
>  - add { } around blocks with a single statement if there is also a
>    comment.
> 
>  - fixed too many parenthesis in patch #3 (formerly #6).
> 
>  - dropped other patches as they reorder stuff in unwanted or at least
>    untested ways.
> 
> There was a concern in reply to patch #1 of the v1 series (now #2) from
> Mikko Perttunen. He wrote: 
> 
> > 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.
> 
> It cannot be done reliably in the driver core, and IMHO even root
> shouldn't be able to trigger a NULL pointer exception. So I kept the
> check.

As I mentioned in a comment to the patch, I second Mikko's concern.
Adding validity checks for device data seems like one of those
boilerplate things we should be able to avoid. We never match by name in
the drivers and if driver_override is the only reason why the device
data might end up being NULL, then driver_override should be completely
disabled for this driver because it simply isn't going to work without
the match data (as evidenced by your patch returning an error code in
that case).

I'll take a look at adding a way for the core to let drivers opt-out of
driver_override if it doesn't make sense for them.

Thierry

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

Re: [PATCH v2 1/3] pwm: tegra: Make use of dev_err_probe()

[Uwe Kleine-König]

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

Hello Thierry,

On Mon, Sep 21, 2026 at 11:38:41AM +0200, Thierry Reding wrote:
> On Fri, Sep 18, 2026 at 04:33:45PM +0200, 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.
> > 
> > Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > ---
> >  drivers/pwm/pwm-tegra.c | 52 ++++++++++++++++++++++++-----------------
> >  1 file changed, 31 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > index 5cdbe120ba2d..efb7ab60f602 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -316,14 +316,15 @@ 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;
> >  	int ret;
> >  
> > -	soc = of_device_get_match_data(&pdev->dev);
> > +	soc = of_device_get_match_data(dev);
> >  
> > -	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);
> > @@ -331,28 +332,39 @@ static int tegra_pwm_probe(struct platform_device *pdev)
> >  	pc->soc = soc;
> >  
> >  	pc->regs = devm_platform_ioremap_resource(pdev, 0);
> > -	if (IS_ERR(pc->regs))
> > +	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.
> > +		 */
> 
> Seems a bit counter-productive to leave comments like this. Function
> comments should document what the function does and then people should
> read those comments. Then we don't need to mention it every time we call
> these functions.

I often deal with bug reports by users where things fail without an
error message[1]. So a usual thing I do is checking probe (and other)
functions for silent error paths. As I fail to follow the (continously
changing) set of functions that emit an error message, this comment is
very useful for a me at least and I'd wish others would add such
comments, too. (An IMHO fine thing here would be to let no generic
resource getter function emit an error message, but that ship has
sailed.)

I'd be open for a shorter marker, that might even be machine-parsable.
 
> >  		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);
> > -	if (ret)
> > +	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.
> > +		 */
> 
> Same here.
> 
> > @@ -385,17 +395,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");
> 
> This stands out as very different from other error messages, so maybe
> change this to something like "Failed to add PWM chip" for consistency?

Fine for me, will fix in the next submission.

Thanks for your feedback,
Uwe

[1] last instance was just today, where on a bananapi USB didn't work
    and /sys/kernel/debug/devices_deferred ended up containing:

	1c13000.usb     platform: supplier 1c13400.phy not ready
	1c1c000.usb     platform: supplier 1c13400.phy not ready
	1c14400.usb     platform: supplier 1c13400.phy not ready
	1c13400.phy     platform: supplier axp20x-usb-power-supply not ready
	1c14000.usb     platform: supplier 1c13400.phy not ready
	1c1c400.usb     platform: supplier 1c13400.phy not ready
	axp20x-usb-power-supply

    It would be so easy[2] to add a useful debugging hint here

[2] https://lore.kernel.org/all/b699f8251afed736af454a981630926e45eb7da7.1789983244.git.ukleinek@debian.org/


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

Re: [PATCH v2 3/3] pwm: tegra: Implement .get_state()

[Uwe Kleine-König]

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

Hello Thierry,

On Mon, Sep 21, 2026 at 12:18:16PM +0200, Thierry Reding wrote:
> On Fri, Sep 18, 2026 at 04:33:47PM +0200, Uwe Kleine-König wrote:
> > The registers of the PWM IP are readable. Use that to implement the
> > .get_state() callback.
> > 
> > Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>
> > Tested-by: Ola Chr. Vaage <ola.christoffer.vage@scoutdi.com>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > ---
> >  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 b461d3877f43..0520d025c776 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -310,8 +310,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;
> 
> It feels like this has too many assumptions built-in. That's mostly a
> predefined issue, but I think if we want to get accurate hardware read-
> out, we need to address this.
> 
> According to the register documentation, the PWM depth is 16 bits wide
> (on generations where it can be programmed). The value defaults to 255
> (which is n - 1 encoded, hence TEGRA_PWM_DEPTH), but it can technically
> be reprogrammed to any 16-bit value, as far as I can tell.
> 
> So I think for this to be correct we'd need to read out the actual value
> before overwriting with TEGRA_PWM_CSR_0 contents above. At that point I
> think we'd need to either adjust the mask to be (2 * depth) - 1, or
> maybe better yet, avoid masking it out arbitrarily based on the depth
> and instead cap it at depth so we never exceed the 1:1 ratio for duty
> cycle vs. period.

As long as .apply() also hardcodes TEGRA_PWM_DEPTH, it's IMO fine that
.get_state() does so, too.

> > +		/*
> > +		 * 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),
> 
> Maybe as part of the above this can be broken down a bit to make it
> easier to read? Something like:
> 
> 	u64 duty_cycle = (u64)(scale + 1) * pwm0 * NSEC_PER_SEC;
> 	u64 period = (u64)(scale + 1) * depth * NSEC_PER_SEC;
> 
> 	...
> 
> 	*state = (struct pwm_state) {
> 		.period = DIV64_U64_ROUND_UP(period, pc->clk_rate),
> 		.duty_cycle = DIV64_U64_ROUND_UP(duty_cycle, pc->clk_rate),
> 		...
> 	};
> 
> is a bit easier on the eye and would make checkpatch happy (or happier).

Fine for me, will address in the next revision.

Best regards
Uwe

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

Re: [PATCH v2 2/3] pwm: tegra: Check for match_data being NULL

[Uwe Kleine-König]

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

Hello Thierry,

On Mon, Sep 21, 2026 at 11:47:17AM +0200, Thierry Reding wrote:
> On Fri, Sep 18, 2026 at 04:33:46PM +0200, 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 <u.kleine-koenig@baylibre.com>
> > ---
> >  drivers/pwm/pwm-tegra.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> > index efb7ab60f602..b461d3877f43 100644
> > --- a/drivers/pwm/pwm-tegra.c
> > +++ b/drivers/pwm/pwm-tegra.c
> > @@ -323,6 +323,14 @@ static int tegra_pwm_probe(struct platform_device *pdev)
> >  	int ret;
> >  
> >  	soc = of_device_get_match_data(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");
> > +	}
> 
> We don't usually do this. Matching via anything other than OF device ID
> tables (or ACPI, I suppose) is a programming error and you deserve the
> crash which forces you to fix things rather than continue with an error
> that is easy to miss.

I don't agree to "you deserve the crash". IMHO even root should be
unable to make the kernel crash. I don't understand what you think
should be fixed if I hit that crash. My userspace interactions in /sys?
Which error is easy to miss?

> Driver overrides aren't going to work with these devices anyway, so I'm
> beginning to think it might be worth looking into opting out of the
> override behaviour for select drivers.

I think there are much more drivers that don't expect to be forced on a
device, so opting in for override would be a more sensible result. Of
course the path to there is more painful ...

Best regards
Uwe

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