| Message ID | 20190815093839.23710-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC] pwm: add debug knob to help driver authors | expand |
On 15. 08. 19 11:38, Uwe Kleine-König wrote: > This patch adds some additional checks to the pwm core that help getting > the details right. The check about rounding isn't approved yet, but I > consider that sensible as it helps consistency when all drivers round in > the same direction. The other checks are in line with what I understood > are the intended requirements. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > this patch is only compile tested up to now but still I think it adds > useful diagnostics for driver authors that helps even more than an exact > documentation. Feedback welcome. Hello Uwe, First of all, thank you for your continuous effort to improve the situation! I just tried your patch on our imx6 platform. Here are my few cents. > > drivers/pwm/Kconfig | 8 +++++++ > drivers/pwm/core.c | 56 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+) > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index a7e57516959e..76105cfd581d 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -33,6 +33,14 @@ config PWM_SYSFS > bool > default y if SYSFS > > +config PWM_DEBUG > + bool "Additional checks for correctness of lowlevel drivers" I am not native speaker too but this sounds weird to me. And as a driver author I would maybe like to see the word "debug" somewhere in the prompt for this option so its purpose is more obvious? What about something like "PWM lowlevel drivers debugging" or "PWM lowlevel drivers additional checks and debug messages" ? > + help > + This option enables some additional checks to help lowlevel driver > + authors to get their callbacks implemented correctly. > + It is expected to introduce some runtime overhead and diagnostic > + output to the kernel log, so only enable while working on a driver. > + > config PWM_AB8500 > tristate "AB8500 PWM support" > depends on AB8500_CORE && ARCH_U8500 > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 8edfac17364e..6ce341a4574d 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -467,12 +467,68 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > return 0; > > if (pwm->chip->ops->apply) { > + struct pwm_state state_pre = *state; > + > err = pwm->chip->ops->apply(pwm->chip, pwm, state); > if (err) > return err; > > + if (IS_ENABLED(CONFIG_PWM_DEBUG)) { > + if (pwm->chip->ops->get_state) { > + struct pwm_state state_post = { 0, }; > + > + pwm->chip->ops->get_state(pwm->chip, pwm, &state_post); > + > + if (state_post.period != state->period || > + state_post.duty_cycle != state->duty_cycle || > + state_post.polarity != state->polarity || > + state_post.enabled != state->enabled) { > + > + dev_warn(pwm->chip->dev, > + ".apply is supposed to modify the pwm_state parameter to match the actual output.\n"); I tend to turn the warning messages around to more emphasize there is something wrong in the driver. Maybe something like: "pwm_state parameter does not match actual output! Update .apply to modify the state please.\n" > + } > + > + if (state_pre.polarity == state_post.polarity && > + state_pre.enabled && > + (state_pre.period > state_post.period || > + state_pre.duty_cycle > state_post.period)) { > + > + dev_warn(pwm->chip->dev, > + ".apply is supposed to round down both period and duty_cycle.\n"); And here something like: "period or duty_cycle is not rounded down. Update .apply to do so please.\n" > + > + } > + > + *state = state_post; > + > + /* reapply state_post and check it is unmodified */ > + err = pwm->chip->ops->apply(pwm->chip, pwm, state); Here you reapply even if the pre and post states match. On imx6 that means you just wrote to the PWM_PWMPR register. Such write "results in the counter being reset to zero and the start of a new count period." viz 51.7.5 in the RM. That means the current period most probably was not completed when you applied the new configuration. Which is one of the requirements we are finally trying to met. Hmm, now while I just finished the above paragraph I think the answer to this is that it is responsibility of the PWM driver to ensure such requirement is met. So that is just another opportunity how to improve the pwm-imx27 driver .) > + if (err) { > + dev_err(pwm->chip->dev, > + "failed to reapply the current setting\n"); Another nitpicking. Consistency in first capital letter and period at the end of message? > + return err; > + } > + > + if (state_post.period != state->period || > + state_post.duty_cycle != state->duty_cycle || > + state_post.polarity != state->polarity || > + state_post.enabled != state->enabled) { > + dev_warn(pwm->chip->dev, > + "applying the settings that .get_state returned yields changes\n"); Same here. > + } > + > + } else { > + dev_warn(pwm->chip->dev, > + "Please update the driver to provide .get_state()\n"); This could be rephrased to: "Update the driver to provide .get_state() please.\n" to be consistent with the other messages I proposed. > + } > + } > + > pwm->state = *state; > } else { > + if (IS_ENABLED(CONFIG_PWM_DEBUG)) { > + dev_warn(pwm->chip->dev, > + "Please update the driver to provide .apply() instead of .config()\n"); Same here. > + } > + > /* > * FIXME: restore the initial state in case of error. > */ > While testing the patch I tried to configure some extreme values and got quite strange results. The best resolution of the i.MX6 PWM is ~15ns. root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 1000 > period root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 45 > duty_cycle root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [352446.858141] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle 30 root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 30 > duty_cycle root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle 30 root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 15 > duty_cycle root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [352511.598175] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle 0 root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 30 > duty_cycle root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [352538.138203] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle 15 root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 45 > duty_cycle root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [352559.248184] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle 30 root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 60 > duty_cycle root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [353811.638114] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle 45 root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 75 > duty_cycle root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [353819.718146] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle 61 root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 76 > duty_cycle root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle 76 So the driver refuses to configure a certain duty_cycle value, e.g. 30, but it happily configures it when you try the next higher one, e.g. 45. I suppose it is related to rounding in the pwm-imx27 driver but I did spent much time thinking about the root cause yet, sorry. Anyway, your patch already prove to be useful ;) Thank you, Michal
Hi Uwe, I have two suggestions below: On Thu, 2019-08-15 at 11:38 +0200, Uwe Kleine-König wrote: > This patch adds some additional checks to the pwm core that help getting > the details right. The check about rounding isn't approved yet, but I > consider that sensible as it helps consistency when all drivers round in > the same direction. The other checks are in line with what I understood > are the intended requirements. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > this patch is only compile tested up to now but still I think it adds > useful diagnostics for driver authors that helps even more than an exact > documentation. Feedback welcome. > > Best regards > Uwe > > drivers/pwm/Kconfig | 8 +++++++ > drivers/pwm/core.c | 56 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+) > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index a7e57516959e..76105cfd581d 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -33,6 +33,14 @@ config PWM_SYSFS > bool > default y if SYSFS > > +config PWM_DEBUG > + bool "Additional checks for correctness of lowlevel drivers" > + help > + This option enables some additional checks to help lowlevel driver > + authors to get their callbacks implemented correctly. > + It is expected to introduce some runtime overhead and diagnostic > + output to the kernel log, so only enable while working on a driver. > + > config PWM_AB8500 > tristate "AB8500 PWM support" > depends on AB8500_CORE && ARCH_U8500 > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 8edfac17364e..6ce341a4574d 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -467,12 +467,68 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > return 0; > > if (pwm->chip->ops->apply) { > + struct pwm_state state_pre = *state; > + > err = pwm->chip->ops->apply(pwm->chip, pwm, state); > if (err) > return err; > > + if (IS_ENABLED(CONFIG_PWM_DEBUG)) { How about moving this whole block into a separate function, say 'pwm_validate_state', to reduce the level of indentation and to separate the debug/validation code from the common business logic a bit? > + if (pwm->chip->ops->get_state) { > + struct pwm_state state_post = { 0, }; > + > + pwm->chip->ops->get_state(pwm->chip, pwm, &state_post); > + > + if (state_post.period != state->period || > + state_post.duty_cycle != state->duty_cycle || > + state_post.polarity != state->polarity || > + state_post.enabled != state->enabled) { This is only used twice here, but there is an inverted version of this check in the beginning of pwm_apply_state already. You could introduce a 'pwm_state_equal' helper function to simplify this. > + > + dev_warn(pwm->chip->dev, > + ".apply is supposed to modify the pwm_state parameter to match the actual output.\n"); > + } > + > + if (state_pre.polarity == state_post.polarity && > + state_pre.enabled && > + (state_pre.period > state_post.period || > + state_pre.duty_cycle > state_post.period)) { > + > + dev_warn(pwm->chip->dev, > + ".apply is supposed to round down both period and duty_cycle.\n"); > + > + } > + > + *state = state_post; > + > + /* reapply state_post and check it is unmodified */ > + err = pwm->chip->ops->apply(pwm->chip, pwm, state); > + if (err) { > + dev_err(pwm->chip->dev, > + "failed to reapply the current setting\n"); > + return err; > + } > + > + if (state_post.period != state->period || > + state_post.duty_cycle != state->duty_cycle || > + state_post.polarity != state->polarity || > + state_post.enabled != state->enabled) { > + dev_warn(pwm->chip->dev, > + "applying the settings that .get_state returned yields changes\n"); > + } > + > + } else { > + dev_warn(pwm->chip->dev, > + "Please update the driver to provide .get_state()\n"); > + } > + } > + > pwm->state = *state; > } else { > + if (IS_ENABLED(CONFIG_PWM_DEBUG)) { > + dev_warn(pwm->chip->dev, > + "Please update the driver to provide .apply() instead of .config()\n"); > + } > + > /* > * FIXME: restore the initial state in case of error. > */ regards Philipp
Hello Michal, On Tue, Aug 20, 2019 at 04:29:10PM +0200, Michal Vokáč wrote: > On 15. 08. 19 11:38, Uwe Kleine-König wrote: > > This patch adds some additional checks to the pwm core that help getting > > the details right. The check about rounding isn't approved yet, but I > > consider that sensible as it helps consistency when all drivers round in > > the same direction. The other checks are in line with what I understood > > are the intended requirements. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > Hello, > > > > this patch is only compile tested up to now but still I think it adds > > useful diagnostics for driver authors that helps even more than an exact > > documentation. Feedback welcome. > > First of all, thank you for your continuous effort to improve the situation! > I just tried your patch on our imx6 platform. Here are my few cents. Great. I didn't test my patch at all because I first wanted to collect some feedback if such a debug knob is considered a good idea. So I really appreciate your feedback. > > drivers/pwm/Kconfig | 8 +++++++ > > drivers/pwm/core.c | 56 +++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 64 insertions(+) > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index a7e57516959e..76105cfd581d 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -33,6 +33,14 @@ config PWM_SYSFS > > bool > > default y if SYSFS > > +config PWM_DEBUG > > + bool "Additional checks for correctness of lowlevel drivers" > > I am not native speaker too but this sounds weird to me. And as > a driver author I would maybe like to see the word "debug" somewhere > in the prompt for this option so its purpose is more obvious? > > What about something like "PWM lowlevel drivers debugging" or > "PWM lowlevel drivers additional checks and debug messages" ? Yeah, also a depend on DEBUG_KERNEL is probably sensible, similar to what DEBUG_GPIO does. > > + help > > + This option enables some additional checks to help lowlevel driver > > + authors to get their callbacks implemented correctly. > > + It is expected to introduce some runtime overhead and diagnostic > > + output to the kernel log, so only enable while working on a driver. > > + > > config PWM_AB8500 > > tristate "AB8500 PWM support" > > depends on AB8500_CORE && ARCH_U8500 > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > index 8edfac17364e..6ce341a4574d 100644 > > --- a/drivers/pwm/core.c > > +++ b/drivers/pwm/core.c > > @@ -467,12 +467,68 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > > return 0; > > if (pwm->chip->ops->apply) { > > + struct pwm_state state_pre = *state; > > + > > err = pwm->chip->ops->apply(pwm->chip, pwm, state); > > if (err) > > return err; > > + if (IS_ENABLED(CONFIG_PWM_DEBUG)) { > > + if (pwm->chip->ops->get_state) { > > + struct pwm_state state_post = { 0, }; > > + > > + pwm->chip->ops->get_state(pwm->chip, pwm, &state_post); > > + > > + if (state_post.period != state->period || > > + state_post.duty_cycle != state->duty_cycle || > > + state_post.polarity != state->polarity || > > + state_post.enabled != state->enabled) { > > + > > + dev_warn(pwm->chip->dev, > > + ".apply is supposed to modify the pwm_state parameter to match the actual output.\n"); > > I tend to turn the warning messages around to more emphasize there > is something wrong in the driver. Maybe something like: > "pwm_state parameter does not match actual output! Update .apply to modify the state please.\n" BTW: Before making the expectation on .apply to modify the state parameter official, I wonder if this is sensible. Most of the time the consumer doesn't seem to care. And providing the actual implemented parameters is some extra effort. I think it is more sensible to let lowlevel drivers not modify the state argument and change the PWM core instead to call .get_state() in pwm_get_state. (And maybe cache the result for later calls.) Also in general it would probably be helpful to put some details about the configured state in the debug warnings to make them more helpful in case they are just forwarded to the PWM list. > > + } > > + > > + if (state_pre.polarity == state_post.polarity && > > + state_pre.enabled && > > + (state_pre.period > state_post.period || > > + state_pre.duty_cycle > state_post.period)) { > > + > > + dev_warn(pwm->chip->dev, > > + ".apply is supposed to round down both period and duty_cycle.\n"); > > And here something like: > "period or duty_cycle is not rounded down. Update .apply to do so please.\n" fine. > > + > > + } > > + > > + *state = state_post; > > + > > + /* reapply state_post and check it is unmodified */ > > + err = pwm->chip->ops->apply(pwm->chip, pwm, state); > > Here you reapply even if the pre and post states match. Yeah, I don't think optimizing the debug code is a critical thing to do :-) > On imx6 that means you just wrote to the PWM_PWMPR register. > Such write "results in the counter being reset to zero and the start of > a new count period." viz 51.7.5 in the RM. > > That means the current period most probably was not completed when you > applied the new configuration. Which is one of the requirements we are > finally trying to met. > > Hmm, now while I just finished the above paragraph I think the answer > to this is that it is responsibility of the PWM driver to ensure such > requirement is met. So that is just another opportunity how to improve > the pwm-imx27 driver .) Yeah, I thought a bit about how this could be catched by core debug code, but this isn't trivial. I think the condition is: If the configured output is actually changed by a call to .apply() the callback must not return earlier that $oldperiod after the previous .apply() was entered. This is definitively something I want to have checked, but I postponed that to later. > [...] > > While testing the patch I tried to configure some extreme values > and got quite strange results. > > The best resolution of the i.MX6 PWM is ~15ns. > > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 1000 > period > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 45 > duty_cycle > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [352446.858141] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle > 30 > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 30 > duty_cycle > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle > 30 > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 15 > duty_cycle > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [352511.598175] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle > 0 > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 30 > duty_cycle > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [352538.138203] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle > 15 Oh wow, so the result of configuring duty_cycle = 30ns depends on the previous state. > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 45 > duty_cycle > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [352559.248184] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle > 30 > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 60 > duty_cycle > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [353811.638114] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle > 45 > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 75 > duty_cycle > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# [353819.718146] pwm-imx27 2080000.pwm: .apply is supposed to modify the pwm_state parameter to match the actual output. > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle > 61 > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# echo 76 > duty_cycle > root@hydraco:/sys/class/pwm/pwmchip0/pwm0# cat duty_cycle > 76 > > So the driver refuses to configure a certain duty_cycle value, e.g. 30, > but it happily configures it when you try the next higher one, e.g. 45. > I suppose it is related to rounding in the pwm-imx27 driver but I did > spent much time thinking about the root cause yet, sorry. > > Anyway, your patch already prove to be useful ;) \o/ Best regards and thanks for your feedback, Uwe
Hello Philipp, On Tue, Aug 20, 2019 at 05:24:28PM +0200, Philipp Zabel wrote: > I have two suggestions below: > > [...] Good ideas. I will address them in a v2. But I'd like to wait for some feedback by Thierry first to not spend time and energy in stuff that won't get applied in the end. Best regards Uwe
Hello Thierry, On Thu, Aug 15, 2019 at 11:38:39AM +0200, Uwe Kleine-König wrote: > This patch adds some additional checks to the pwm core that help getting > the details right. The check about rounding isn't approved yet, but I > consider that sensible as it helps consistency when all drivers round in > the same direction. The other checks are in line with what I understood > are the intended requirements. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > this patch is only compile tested up to now but still I think it adds > useful diagnostics for driver authors that helps even more than an exact > documentation. Feedback welcome. do you consider the idea here worthwile? If so I'd update the patch to current mainline and address the feedback I got so far. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index a7e57516959e..76105cfd581d 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -33,6 +33,14 @@ config PWM_SYSFS bool default y if SYSFS +config PWM_DEBUG + bool "Additional checks for correctness of lowlevel drivers" + help + This option enables some additional checks to help lowlevel driver + authors to get their callbacks implemented correctly. + It is expected to introduce some runtime overhead and diagnostic + output to the kernel log, so only enable while working on a driver. + config PWM_AB8500 tristate "AB8500 PWM support" depends on AB8500_CORE && ARCH_U8500 diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 8edfac17364e..6ce341a4574d 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -467,12 +467,68 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) return 0; if (pwm->chip->ops->apply) { + struct pwm_state state_pre = *state; + err = pwm->chip->ops->apply(pwm->chip, pwm, state); if (err) return err; + if (IS_ENABLED(CONFIG_PWM_DEBUG)) { + if (pwm->chip->ops->get_state) { + struct pwm_state state_post = { 0, }; + + pwm->chip->ops->get_state(pwm->chip, pwm, &state_post); + + if (state_post.period != state->period || + state_post.duty_cycle != state->duty_cycle || + state_post.polarity != state->polarity || + state_post.enabled != state->enabled) { + + dev_warn(pwm->chip->dev, + ".apply is supposed to modify the pwm_state parameter to match the actual output.\n"); + } + + if (state_pre.polarity == state_post.polarity && + state_pre.enabled && + (state_pre.period > state_post.period || + state_pre.duty_cycle > state_post.period)) { + + dev_warn(pwm->chip->dev, + ".apply is supposed to round down both period and duty_cycle.\n"); + + } + + *state = state_post; + + /* reapply state_post and check it is unmodified */ + err = pwm->chip->ops->apply(pwm->chip, pwm, state); + if (err) { + dev_err(pwm->chip->dev, + "failed to reapply the current setting\n"); + return err; + } + + if (state_post.period != state->period || + state_post.duty_cycle != state->duty_cycle || + state_post.polarity != state->polarity || + state_post.enabled != state->enabled) { + dev_warn(pwm->chip->dev, + "applying the settings that .get_state returned yields changes\n"); + } + + } else { + dev_warn(pwm->chip->dev, + "Please update the driver to provide .get_state()\n"); + } + } + pwm->state = *state; } else { + if (IS_ENABLED(CONFIG_PWM_DEBUG)) { + dev_warn(pwm->chip->dev, + "Please update the driver to provide .apply() instead of .config()\n"); + } + /* * FIXME: restore the initial state in case of error. */
This patch adds some additional checks to the pwm core that help getting the details right. The check about rounding isn't approved yet, but I consider that sensible as it helps consistency when all drivers round in the same direction. The other checks are in line with what I understood are the intended requirements. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, this patch is only compile tested up to now but still I think it adds useful diagnostics for driver authors that helps even more than an exact documentation. Feedback welcome. Best regards Uwe drivers/pwm/Kconfig | 8 +++++++ drivers/pwm/core.c | 56 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+)