pwm: tegra: Cleanups and .get_state()
Subsystems (3)
Uwe Kleine-König
maintainercover pwm: tegra: Cleanups and .get_state()
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
Signature · 1 line
2.55.0.11.g153666a7d9bb
Uwe Kleine-König
maintainerPATCH 1/6 pwm: tegra: Check for match_data being NULL
It's unlikely but not impossible that of_device_get_match_data() returns NULL. Handle this case instead of triggering a NULL pointer exception.
--- drivers/pwm/pwm-tegra.c | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/pwm/pwm-tegra.c +7 -0
@@ -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))
Signature · 1 line
2.55.0.11.g153666a7d9bb
Re: [PATCH v1 1/6] pwm: tegra: Check for match_data being NULL
Quoted 13 lines · 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 <[email protected]>--- drivers/pwm/pwm-tegra.c | 7 +++++++ 1 file changed, 7 insertions(+)drivers/pwm/pwm-tegra.c +0
-0
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 325
Quoted 4 lines
drivers/pwm/pwm-tegra.c +1
-0@@ -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.
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 330
Quoted 5 lines
drivers/pwm/pwm-tegra.c +5
-0+/* +* 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.
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 331
Quoted 1 line
drivers/pwm/pwm-tegra.c +1
-0+return dev_err_probe (dev, -ENODEV ,"Unsupported device\n" );
'dev' is not defined (yet).
Thank you
Mikko
Quoted 7 lines
drivers/pwm/pwm-tegra.c +0
-0chip =devm_pwmchip_alloc (&pdev->dev, soc->num_channels,sizeof (*pc));if (IS_ERR (chip))Signature · 1 line
2.55.0.11.g153666a7d9bb
Uwe Kleine-König
maintainerRe: [PATCH v1 1/6] pwm: tegra: Check for match_data being NULL
Hello,
Quoted 16 lines · Mikko Perttunen
Quoted 15 lines · 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 <[email protected]>--- drivers/pwm/pwm-tegra.c | 7 +++++++ 1 file changed, 7 insertions(+)drivers/pwm/pwm-tegra.c +0
-0@@ -322,6 +322,13 @@ static int tegra_pwm_probe(struct platform_device *pdev)int ret;
Quoted 5 lines
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 325
Quoted 2 lines
drivers/pwm/pwm-tegra.c +1
-0soc =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.
If you read Documentation/process/coding-style.rst by the letter, this case shouldn't have braces, but I agree that adding braces here is clearer (and that coding-style.rst shouldn't be read by the letter).
Quoted 9 lines
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 330
Quoted 5 lines
drivers/pwm/pwm-tegra.c +5
-0+/* +* 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_overridefalls 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'.
IMHO even root should not be able to trigger a NULL pointer exception. Not sure there is a general agreed on policy about that though.
Quoted 7 lines
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.
So you'd want to check the device-id table before calling .probe() and if all entries have a non-zero .driver_data don't honor the override? Hmm, maybe something to discuss at the "Driver Core" microconference (at LPC 2026 in Prague), but my spontanous reaction is that this is a heuristic only and might prevent valid use-cases.
Quoted 3 lines
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 331
Quoted 1 line
drivers/pwm/pwm-tegra.c +1
-0+return dev_err_probe (dev, -ENODEV ,"Unsupported device\n" );'dev' is not defined (yet).
Ooops, that leaked in as I reordered the patches to have the fixes first. Thanks for noticing.
Best regards
Uwe
Uwe Kleine-König
maintainerPATCH 2/6 pwm: tegra: Make use of dev_err_probe()
In reply to Uwe Kleine-KönigUsage 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.
--- drivers/pwm/pwm-tegra.c | 44 ++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 18 deletions(-)
drivers/pwm/pwm-tegra.c +26 -18
@@ -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; }
Signature · 1 line
2.55.0.11.g153666a7d9bb
Re: [PATCH v1 2/6] pwm: tegra: Make use of dev_err_probe()
Quoted 33 lines · Uwe Kleine-König
Usage of
dev_err_probe()is more compact than dev_err()'s, emits the error code and handles-ENOMEMand -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 <[email protected]>--- drivers/pwm/pwm-tegra.c | 44 ++++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 18 deletions(-)drivers/pwm/pwm-tegra.c +2
-1@@ -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);
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 348
Quoted 9 lines
drivers/pwm/pwm-tegra.c +5
-0@@ -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.
Quoted 84 lines
drivers/pwm/pwm-tegra.c +19
-17platform_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; }Signature · 1 line
2.55.0.11.g153666a7d9bb
Uwe Kleine-König
maintainerPATCH 3/6 pwm: tegra: Use devm function for pm_runtime_enable()
In reply to Uwe Kleine-KönigThis 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".
--- drivers/pwm/pwm-tegra.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/pwm/pwm-tegra.c +4 -4
@@ -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)
Signature · 1 line
2.55.0.11.g153666a7d9bb
Re: [PATCH v1 3/6] pwm: tegra: Use devm function for pm_runtime_enable()
Quoted 2 lines · Uwe Kleine-König
This simplifies the error paths as
pwm_runtime_disable()is called automatatically by the driver core.
Typo, s/ta//
Quoted 35 lines
Note that
pwm_runtime_disable()is the right function to undopm_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 <[email protected]>--- drivers/pwm/pwm-tegra.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)drivers/pwm/pwm-tegra.c +4
-2@@ -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; }
drivers/pwm/pwm-tegra.c static void tegra_pwm_remove(struct platform_device *pdev) · line 426
Quoted 5 lines
drivers/pwm/pwm-tegra.c +0
-2@@ -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
Quoted 7 lines
drivers/pwm/pwm-tegra.c +0
-0}static int __maybe_unused tegra_pwm_runtime_suspend (struct device *dev)Signature · 1 line
2.55.0.11.g153666a7d9bb
Uwe Kleine-König
maintainerPATCH 4/6 pwm: tegra: Simplify using devm_reset_control_get_exclusive_deasserted()
In reply to Uwe Kleine-KönigThis 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.
--- drivers/pwm/pwm-tegra.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
drivers/pwm/pwm-tegra.c +4 -10
@@ -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)
Signature · 1 line
2.55.0.11.g153666a7d9bb
Re: [PATCH v1 4/6] pwm: tegra: Simplify using devm_reset_control_get_exclusive_deasserted()
Quoted 22 lines · 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 <[email protected]>--- drivers/pwm/pwm-tegra.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)drivers/pwm/pwm-tegra.c +0
-1@@ -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;
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 321
Quoted 4 lines
drivers/pwm/pwm-tegra.c +1
-0@@ -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.
Quoted 14 lines
drivers/pwm/pwm-tegra.c +3
-3const 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;
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 407
Quoted 10 lines
drivers/pwm/pwm-tegra.c +0
-3} -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
Uwe Kleine-König
maintainerRe: [PATCH v1 4/6] pwm: tegra: Simplify using devm_reset_control_get_exclusive_deasserted()
Quoted 20 lines · Philipp Zabel
Quoted 19 lines · 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 <[email protected]>--- drivers/pwm/pwm-tegra.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)drivers/pwm/pwm-tegra.c +0
-1@@ -108,7 +108,6 @@ struct tegra_pwm_soc {struct tegra_pwm_chip {struct clk *clk; -struct reset_control *rst;
Quoted 10 lines
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 321
Quoted 7 lines
drivers/pwm/pwm-tegra.c +1
-0unsigned 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_controland *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
Quoted 20 lines
Quoted 20 lines
drivers/pwm/pwm-tegra.c +3
-5const 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;
Quoted 8 lines
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 407
Quoted 4 lines
drivers/pwm/pwm-tegra.c +0
-1ret =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()andpm_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
Re: [PATCH v1 4/6] pwm: tegra: Simplify using devm_reset_control_get_exclusive_deasserted()
Quoted 65 lines · Uwe Kleine-König
Quoted 20 lines · Philipp Zabel
Quoted 19 lines · 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 <[email protected]>--- drivers/pwm/pwm-tegra.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)drivers/pwm/pwm-tegra.c +0
-1@@ -108,7 +108,6 @@ struct tegra_pwm_soc {struct tegra_pwm_chip {struct clk *clk; -struct reset_control *rst;Quoted 10 lines
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 321
Quoted 7 lines
drivers/pwm/pwm-tegra.c +1
-0unsigned 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_controland *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
Quoted 20 lines
Quoted 20 lines
drivers/pwm/pwm-tegra.c +3
-5const 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;Quoted 8 lines
drivers/pwm/pwm-tegra.c static int tegra_pwm_probe(struct platform_device *pdev) · line 407
Quoted 4 lines
drivers/pwm/pwm-tegra.c +0
-1ret =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()andpm_runtime_force_suspend()are called before the reset control is asserted again in the error case. Is this safe?
Quoted 8 lines
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
Uwe Kleine-König
maintainerPATCH 5/6 pwm: tegra: Simplify using devm_pwmchip_add()
In reply to Uwe Kleine-Königdevm_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.
--- drivers/pwm/pwm-tegra.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)
drivers/pwm/pwm-tegra.c +1 -9
@@ -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);
Signature · 1 line
2.55.0.11.g153666a7d9bb
Uwe Kleine-König
maintainerPATCH 6/6 pwm: tegra: Implement .get_state()
In reply to Uwe Kleine-KönigThe registers of the PWM IP are readable. Use that to implement the .get_state() callback.
--- drivers/pwm/pwm-tegra.c | 48 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)
drivers/pwm/pwm-tegra.c +48 -0
@@ -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)
Signature · 1 line
2.55.0.11.g153666a7d9bb
Re: [PATCH v1 6/6] pwm: tegra: Implement .get_state()
Quoted 2 lines · Uwe Kleine-König
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.. :)
Quoted 23 lines
Signed-off-by Uwe Kleine-König <[email protected]>--- drivers/pwm/pwm-tegra.c | 48 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+)drivers/pwm/pwm-tegra.c +9
-0@@ -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)
drivers/pwm/pwm-tegra.c static int tegra_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 330
Quoted 10 lines
drivers/pwm/pwm-tegra.c +10
-0+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.
Quoted 38 lines
drivers/pwm/pwm-tegra.c +29
-0+ 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)Signature · 1 line
2.55.0.11.g153666a7d9bb
Re: [PATCH v1 6/6] pwm: tegra: Implement .get_state()
In reply to Uwe Kleine-KönigHello Uwe,
Quoted 1 line · Uwe Kleine-König
Subject: [PATCH v1 6/6] pwm: tegra: Implement .get_state()
I tested this on a Jetson Orin NX (Tegra234), the board from my divider truncation report, driving a fan on the 32a0000 PWM instance.
The board runs NVIDIA's L4T 5.15 kernel, so I backported the patch onto that tree: .get_state() returns void there, pwmchip_parent(chip) becomes the driver's device pointer, and the per-SoC enable_reg / scale_width indirection collapses to the fixed CSR layout (that tree predates the Tegra264 restructure). The decode logic is unchanged from your patch. The tree also carries my divider change from the other thread, which is why the achieved periods in the table differ from what your base would program.
Procedure: apply a state through the pwm sysfs interface, read the CSR register and the clock rate independently (/dev/mem and clk_summary), then unexport/re-export the channel so pwm_device_request() invokes .get_state(), and compare the reported state against values computed from the raw register:
applied period/duty/enabled readback computed from CSR+clk 45334/20000/1 46432/20496/1 46432/20496/1 500000/250000/1 481883/240942/1 481883/240942/1 5000/2500/1 5020/2510/1 5020/2510/1 45334/45334/1 (100% duty) 46432/46432/1 46432/46432/1 45334/20000/0 enabled=0 enabled=0
The clock rate varied across the cases (3.19, 11.03 and 102 MHz), so the scale field was exercised at several values, and the 100% duty case reads the full 9-bit duty field (pwm0 = 256). The readback consistently reports the achieved hardware state rather than the requested one.
One limitation of my backport, not your code: in the disabled case I only set state->enabled = false instead of zeroing the whole struct, so I did not verify the period/duty values your version reports for a disabled channel.
Best regards
Ola