[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

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 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 1/3] pwm: tegra: Make use of dev_err_probe()

[Thierry Reding]

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

On Mon, Sep 21, 2026 at 02:46:03PM +0200, Uwe Kleine-König wrote:
> 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 seem to remember that we discussed both options at the time and the
general concensus was that these functions should provide canonical
error messages since their whole purpose was to remove boilerplate and
about a quarter or so of the boilerplate was the error message, with the
added issue that error messages were all over the place. All of these
callsites are going to print an message for these errors, so might as
well add standard messages for these types of situations and save a
bunch of text. I think overall it's a win, but it comes at the cost of
people having to know that these already print errors.

> I'd be open for a shorter marker, that might even be machine-parsable.

I don't know if an extra marker would be all that helpful over a
comment. The beauty of the current solution is that the error handling
is reduced to just the check and the return value, everything else is
encapsulated into the helper.

There are semantic patches that check for these situations, but I
suspect not everyone runs those. I don't know if we can somehow make the
compiler warn about these situations.

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

I see. Yeah, I find it increasingly difficult to remember all the
context of a function call, whether it is that it prints an error
message or needs some lock to be held, sleeps or not. Sashiko has
been really good about pointing some of those things out.

Thierry

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

[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

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

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

[Thierry Reding]

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

On Mon, Sep 21, 2026 at 04:34:09PM +0200, Uwe Kleine-König wrote:
> 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?

Oh, root can easily make the kernel crash in any number of ways. That's
really kind of baked into the concept.

To me this is in the same category as force-unloading a module. You can
do it, but you should know that it's potentially dangerous and most of
the time doesn't make sense either. It's called forcing because there
are guardrails in place to prevent you from trying to do it.

If a device cannot operate without device data, it doesn't make sense to
bind to it with a driver override because then you just don't get that
data.

I'll grant you that purposefully crashing the system is maybe a bit of
an exaggeration if there's a knob specifically designed to let you do
this, hence why I volunteered to look into opting out of driver_override
where it doesn't make sense.

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

Yeah, driver_override has been baked into the driver core for quite a
long time (platform devices seem to have had this functionality for more
than a decade).

You're probably not wrong about opt-in being the more natural choice,
but looking at commit 3d713e0e382e ("driver core: platform: add device
binding path 'driver_override'"), the intended use-cases are very
generic, so it would probably lead to a continuous stream of patches
needing to be added whenever a new device wants to be supported with
vfio or something.

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

[Uwe Kleine-König]

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

On Mon, Sep 21, 2026 at 06:24:39PM +0200, Thierry Reding wrote:
> On Mon, Sep 21, 2026 at 04:34:09PM +0200, Uwe Kleine-König wrote:
> > 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:
> > > > 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?
> 
> Oh, root can easily make the kernel crash in any number of ways. That's
> really kind of baked into the concept.

Yeah, right. root can poke in /dev/mem (unless STRICT_DEVMEM=y). And
root can allocate memory until the machine crashes (unless a resource
limit is in place). And root can unbind devices, or bring down the
network, but that shouldn't result in a kernel crash. If it does, that's
a bug worth fixing.
 
> To me this is in the same category as force-unloading a module. You can
> do it, but you should know that it's potentially dangerous and most of
> the time doesn't make sense either.

Yes, force-unloading is another such thing, but this can only be done
if MODULE_FORCE_LOAD (default n) is enabled.

The only thing I'm aware that root can do to crash the kernel where I'm
not aware of a guard rail is `kill 1`.

> It's called forcing because there
> are guardrails in place to prevent you from trying to do it.
> 
> If a device cannot operate without device data, it doesn't make sense to
> bind to it with a driver override because then you just don't get that
> data.

Ack, it doesn't make sense, and so IMHO it's worth to spend a check to
prevent that from happening.

> I'll grant you that purposefully crashing the system is maybe a bit of
> an exaggeration if there's a knob specifically designed to let you do
> this, hence why I volunteered to look into opting out of driver_override
> where it doesn't make sense.

Cc: me if you find something. Until that happens I consider introducing
that check the right thing to do. My patch is essentially a codifycation
of such an opt-out. :-D But I agree a more semantical version would be
nicer (but not sooo nice that *I*'d start a new quest for it).

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: 762 bytes --]

Hello,

On Mon, Sep 21, 2026 at 06:24:39PM +0200, Thierry Reding wrote:
> You're probably not wrong about opt-in being the more natural choice,
> but looking at commit 3d713e0e382e ("driver core: platform: add device
> binding path 'driver_override'"), the intended use-cases are very
> generic, so it would probably lead to a continuous stream of patches
> needing to be added whenever a new device wants to be supported with
> vfio or something.

thinking a bit more about that: The use-case presented in that commit is
about

	echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override

. If we had an opt-in mechanism on the driver side, it would only be
vfio* that would need it, wouldn't it? That sounds handleable.

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

[Thierry Reding]

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

On Tue, Sep 22, 2026 at 10:33:16AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Sep 21, 2026 at 06:24:39PM +0200, Thierry Reding wrote:
> > You're probably not wrong about opt-in being the more natural choice,
> > but looking at commit 3d713e0e382e ("driver core: platform: add device
> > binding path 'driver_override'"), the intended use-cases are very
> > generic, so it would probably lead to a continuous stream of patches
> > needing to be added whenever a new device wants to be supported with
> > vfio or something.
> 
> thinking a bit more about that: The use-case presented in that commit is
> about
> 
> 	echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override
> 
> . If we had an opt-in mechanism on the driver side, it would only be
> vfio* that would need it, wouldn't it? That sounds handleable.

I have a prototype patch that I'm going to send out shortly (after
testing that it actually works). The problem ended up being that the
driver_override is a device attribute, so there's no good way to drop it
based on a driver flag.

What I ended up doing was add a flag to the driver that causes the
override matching to abort if the driver doesn't allow it.

And yes, you could probably do this the other way around and require
drivers to opt-in, but given how long this has been there and how
generic the interface is (and it is ABI after all), I don't know if
vfio-platform is the only one where this is being used. For all we know
there could be a myriad of odd use-cases where people are using this in
one way or another.

I was briefly pondering a more automatic way where we'd check for the
presence of any device ID match tables and checking the device data
pointers, but that's a bad heuristic since there's nothing stopping
anyone from providing "sensible" defaults if there is not matched data.

So ultimately I think individual drivers opting out of this behaviour if
they explicitly don't want to support it is probably the only safe way
to do it.

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

[Uwe Kleine-König]

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

On Tue, Sep 22, 2026 at 12:06:14PM +0200, Thierry Reding wrote:
> On Tue, Sep 22, 2026 at 10:33:16AM +0200, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Mon, Sep 21, 2026 at 06:24:39PM +0200, Thierry Reding wrote:
> > > You're probably not wrong about opt-in being the more natural choice,
> > > but looking at commit 3d713e0e382e ("driver core: platform: add device
> > > binding path 'driver_override'"), the intended use-cases are very
> > > generic, so it would probably lead to a continuous stream of patches
> > > needing to be added whenever a new device wants to be supported with
> > > vfio or something.
> > 
> > thinking a bit more about that: The use-case presented in that commit is
> > about
> > 
> > 	echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override
> > 
> > . If we had an opt-in mechanism on the driver side, it would only be
> > vfio* that would need it, wouldn't it? That sounds handleable.
> 
> I have a prototype patch that I'm going to send out shortly (after
> testing that it actually works). The problem ended up being that the
> driver_override is a device attribute, so there's no good way to drop it
> based on a driver flag.

Did you see that I sent such a patch already.
https://lore.kernel.org/lkml/0f7446324f6a0c8f0153d6532d92a6eeecd6a308.1790612298.git.u.kleine-koenig@baylibre.com/ .
You were on Cc:, I thought this to be enough to make you aware and
didn't mention it in this thread here.

I hope to not have provoked much duplicate work.

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

[Thierry Reding]

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

On Wed, Sep 30, 2026 at 01:55:59PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 22, 2026 at 12:06:14PM +0200, Thierry Reding wrote:
> > On Tue, Sep 22, 2026 at 10:33:16AM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > > 
> > > On Mon, Sep 21, 2026 at 06:24:39PM +0200, Thierry Reding wrote:
> > > > You're probably not wrong about opt-in being the more natural choice,
> > > > but looking at commit 3d713e0e382e ("driver core: platform: add device
> > > > binding path 'driver_override'"), the intended use-cases are very
> > > > generic, so it would probably lead to a continuous stream of patches
> > > > needing to be added whenever a new device wants to be supported with
> > > > vfio or something.
> > > 
> > > thinking a bit more about that: The use-case presented in that commit is
> > > about
> > > 
> > > 	echo vfio-platform > /sys/bus/platform/devices/fff51000.ethernet/driver_override
> > > 
> > > . If we had an opt-in mechanism on the driver side, it would only be
> > > vfio* that would need it, wouldn't it? That sounds handleable.
> > 
> > I have a prototype patch that I'm going to send out shortly (after
> > testing that it actually works). The problem ended up being that the
> > driver_override is a device attribute, so there's no good way to drop it
> > based on a driver flag.
> 
> Did you see that I sent such a patch already.
> https://lore.kernel.org/lkml/0f7446324f6a0c8f0153d6532d92a6eeecd6a308.1790612298.git.u.kleine-koenig@baylibre.com/ .
> You were on Cc:, I thought this to be enough to make you aware and
> didn't mention it in this thread here.
> 
> I hope to not have provoked much duplicate work.

Well, from the looks of it you ended up hijacking my earlier attempt
while it was being discussed. It's customary to ask before you do so,
specifically in order to avoid duplication of work. Or at least answer
on the original thread that you plan to take this in a different
direction so that people know what's going on.

Anyway, I was still waiting for any conclusions on the original thread
and didn't spend any more time revisiting my original series, so no harm
done.

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

[Uwe Kleine-König]

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

Hello Thierry,

On Wed, Sep 30, 2026 at 03:14:35PM +0200, Thierry Reding wrote:
> On Wed, Sep 30, 2026 at 01:55:59PM +0200, Uwe Kleine-König wrote:
> > On Tue, Sep 22, 2026 at 12:06:14PM +0200, Thierry Reding wrote:
> > > I have a prototype patch that I'm going to send out shortly (after
> > > testing that it actually works). The problem ended up being that the
> > > driver_override is a device attribute, so there's no good way to drop it
> > > based on a driver flag.
> > 
> > Did you see that I sent such a patch already.
> > https://lore.kernel.org/lkml/0f7446324f6a0c8f0153d6532d92a6eeecd6a308.1790612298.git.u.kleine-koenig@baylibre.com/ .
> > You were on Cc:, I thought this to be enough to make you aware and
> > didn't mention it in this thread here.
> > 
> > I hope to not have provoked much duplicate work.
> 
> Well, from the looks of it you ended up hijacking my earlier attempt
> while it was being discussed. It's customary to ask before you do so,
> specifically in order to avoid duplication of work. Or at least answer
> on the original thread that you plan to take this in a different
> direction so that people know what's going on.

Yeah, sorry about that. On that other thread where we were discussing
taints in context of driver_overrides the topics overlapped, so my idea
was to get all the suggestions into a single place. It seems I failed to
make you notice that in time. Mea culpa.
 
> Anyway, I was still waiting for any conclusions on the original thread
> and didn't spend any more time revisiting my original series, so no harm
> done.

😅

Best regards
Uwe

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

[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 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 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 3/3] pwm: tegra: Implement .get_state()

[Thierry Reding]

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

On Mon, Sep 21, 2026 at 04:26:03PM +0200, Uwe Kleine-König wrote:
> 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.

Okay, fair enough.

Thierry

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

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

[Uwe Kleine-König]

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

Hello Thierry,

On Tue, Sep 22, 2026 at 12:07:26PM +0200, Thierry Reding wrote:
> On Mon, Sep 21, 2026 at 04:26:03PM +0200, Uwe Kleine-König wrote:
> > On Mon, Sep 21, 2026 at 12:18:16PM +0200, Thierry Reding wrote:
> > > 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.
> 
> Okay, fair enough.

Is that an Ack then?

Best regards
Uwe

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

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

[Thierry Reding]

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

On Wed, Sep 30, 2026 at 11:54:03AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Tue, Sep 22, 2026 at 12:07:26PM +0200, Thierry Reding wrote:
> > On Mon, Sep 21, 2026 at 04:26:03PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Sep 21, 2026 at 12:18:16PM +0200, Thierry Reding wrote:
> > > > 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.
> > 
> > Okay, fair enough.
> 
> Is that an Ack then?

I've been thinking about this some more and I don't know if it really
makes sense to keep hard-coding TEGRA_PWM_DEPTH. If only .apply() uses
it, then it's mostly fine, I suppose, because we don't care what the
current (or initial) state is/was. So we either don't use the device or
we overwrite it with a custom set of values.

Once we add .get_state() into the mix, now we kind of have to care about
the initial state, because we might end up using those values. If we did
not care, what would be the point, right? Which means that if we read
out wrong values, we, well, get wrong values. Which then may mean that
we overwrite values that we shouldn't, etc.

I suppose this would be okay if we reject any depth values other than
the default as errors. But then we also significantly reduce the
usefulness of this patch.

Thierry

[-- 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 --]