Commit 3ad1f3a3 authored by Uwe Kleine-König's avatar Uwe Kleine-König Committed by Thierry Reding

pwm: Implement some checks for lowlevel drivers

There are some expectations which the callbacks provided by lowlevel
drivers should fulfill. Implement checks that help driver authors to get
these semantics right. As these have some overhead the checks can be
disabled using a Kconfig setting.
Signed-off-by: default avatarUwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: default avatarThierry Reding <thierry.reding@gmail.com>
parent 2cb5cd90
...@@ -33,6 +33,15 @@ config PWM_SYSFS ...@@ -33,6 +33,15 @@ config PWM_SYSFS
bool bool
default y if SYSFS default y if SYSFS
config PWM_DEBUG
bool "PWM lowlevel drivers additional checks and debug messages"
depends on DEBUG_KERNEL
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 config PWM_AB8500
tristate "AB8500 PWM support" tristate "AB8500 PWM support"
depends on AB8500_CORE && ARCH_U8500 depends on AB8500_CORE && ARCH_U8500
......
...@@ -120,6 +120,9 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label) ...@@ -120,6 +120,9 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
if (pwm->chip->ops->get_state) { if (pwm->chip->ops->get_state) {
pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state); pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
trace_pwm_get(pwm, &pwm->state); trace_pwm_get(pwm, &pwm->state);
if (IS_ENABLED(PWM_DEBUG))
pwm->last = pwm->state;
} }
set_bit(PWMF_REQUESTED, &pwm->flags); set_bit(PWMF_REQUESTED, &pwm->flags);
...@@ -232,17 +235,28 @@ void *pwm_get_chip_data(struct pwm_device *pwm) ...@@ -232,17 +235,28 @@ void *pwm_get_chip_data(struct pwm_device *pwm)
} }
EXPORT_SYMBOL_GPL(pwm_get_chip_data); EXPORT_SYMBOL_GPL(pwm_get_chip_data);
static bool pwm_ops_check(const struct pwm_ops *ops) static bool pwm_ops_check(const struct pwm_chip *chip)
{ {
const struct pwm_ops *ops = chip->ops;
/* driver supports legacy, non-atomic operation */ /* driver supports legacy, non-atomic operation */
if (ops->config && ops->enable && ops->disable) if (ops->config && ops->enable && ops->disable) {
return true; if (IS_ENABLED(CONFIG_PWM_DEBUG))
dev_warn(chip->dev,
"Driver needs updating to atomic API\n");
/* driver supports atomic operation */
if (ops->apply)
return true; return true;
}
return false; if (!ops->apply)
return false;
if (IS_ENABLED(CONFIG_PWM_DEBUG) && !ops->get_state)
dev_warn(chip->dev,
"Please implement the .get_state() callback\n");
return true;
} }
/** /**
...@@ -266,7 +280,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, ...@@ -266,7 +280,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
if (!chip || !chip->dev || !chip->ops || !chip->npwm) if (!chip || !chip->dev || !chip->ops || !chip->npwm)
return -EINVAL; return -EINVAL;
if (!pwm_ops_check(chip->ops)) if (!pwm_ops_check(chip))
return -EINVAL; return -EINVAL;
mutex_lock(&pwm_lock); mutex_lock(&pwm_lock);
...@@ -450,6 +464,107 @@ void pwm_free(struct pwm_device *pwm) ...@@ -450,6 +464,107 @@ void pwm_free(struct pwm_device *pwm)
} }
EXPORT_SYMBOL_GPL(pwm_free); EXPORT_SYMBOL_GPL(pwm_free);
void pwm_apply_state_debug(struct pwm_device *pwm,
const struct pwm_state *state)
{
struct pwm_state *last = &pwm->last;
struct pwm_chip *chip = pwm->chip;
struct pwm_state s1, s2;
int err;
if (!IS_ENABLED(CONFIG_PWM_DEBUG))
return;
/* No reasonable diagnosis possible without .get_state() */
if (!chip->ops->get_state)
return;
/*
* *state was just applied. Read out the hardware state and do some
* checks.
*/
chip->ops->get_state(chip, pwm, &s1);
trace_pwm_get(pwm, &s1);
/*
* The lowlevel driver either ignored .polarity (which is a bug) or as
* best effort inverted .polarity and fixed .duty_cycle respectively.
* Undo this inversion and fixup for further tests.
*/
if (s1.enabled && s1.polarity != state->polarity) {
s2.polarity = state->polarity;
s2.duty_cycle = s1.period - s1.duty_cycle;
s2.period = s1.period;
s2.enabled = s1.enabled;
} else {
s2 = s1;
}
if (s2.polarity != state->polarity &&
state->duty_cycle < state->period)
dev_warn(chip->dev, ".apply ignored .polarity\n");
if (state->enabled &&
last->polarity == state->polarity &&
last->period > s2.period &&
last->period <= state->period)
dev_warn(chip->dev,
".apply didn't pick the best available period (requested: %u, applied: %u, possible: %u)\n",
state->period, s2.period, last->period);
if (state->enabled && state->period < s2.period)
dev_warn(chip->dev,
".apply is supposed to round down period (requested: %u, applied: %u)\n",
state->period, s2.period);
if (state->enabled &&
last->polarity == state->polarity &&
last->period == s2.period &&
last->duty_cycle > s2.duty_cycle &&
last->duty_cycle <= state->duty_cycle)
dev_warn(chip->dev,
".apply didn't pick the best available duty cycle (requested: %u/%u, applied: %u/%u, possible: %u/%u)\n",
state->duty_cycle, state->period,
s2.duty_cycle, s2.period,
last->duty_cycle, last->period);
if (state->enabled && state->duty_cycle < s2.duty_cycle)
dev_warn(chip->dev,
".apply is supposed to round down duty_cycle (requested: %u/%u, applied: %u/%u)\n",
state->duty_cycle, state->period,
s2.duty_cycle, s2.period);
if (!state->enabled && s2.enabled && s2.duty_cycle > 0)
dev_warn(chip->dev,
"requested disabled, but yielded enabled with duty > 0");
/* reapply the state that the driver reported being configured. */
err = chip->ops->apply(chip, pwm, &s1);
if (err) {
*last = s1;
dev_err(chip->dev, "failed to reapply current setting\n");
return;
}
trace_pwm_apply(pwm, &s1);
chip->ops->get_state(chip, pwm, last);
trace_pwm_get(pwm, last);
/* reapplication of the current state should give an exact match */
if (s1.enabled != last->enabled ||
s1.polarity != last->polarity ||
(s1.enabled && s1.period != last->period) ||
(s1.enabled && s1.duty_cycle != last->duty_cycle)) {
dev_err(chip->dev,
".apply is not idempotent (ena=%d pol=%d %u/%u) -> (ena=%d pol=%d %u/%u)\n",
s1.enabled, s1.polarity, s1.duty_cycle, s1.period,
last->enabled, last->polarity, last->duty_cycle,
last->period);
}
}
/** /**
* pwm_apply_state() - atomically apply a new state to a PWM device * pwm_apply_state() - atomically apply a new state to a PWM device
* @pwm: PWM device * @pwm: PWM device
...@@ -480,6 +595,12 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) ...@@ -480,6 +595,12 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
trace_pwm_apply(pwm, state); trace_pwm_apply(pwm, state);
pwm->state = *state; pwm->state = *state;
/*
* only do this after pwm->state was applied as some
* implementations of .get_state depend on this
*/
pwm_apply_state_debug(pwm, state);
} else { } else {
/* /*
* FIXME: restore the initial state in case of error. * FIXME: restore the initial state in case of error.
......
...@@ -71,7 +71,8 @@ struct pwm_state { ...@@ -71,7 +71,8 @@ struct pwm_state {
* @chip: PWM chip providing this PWM device * @chip: PWM chip providing this PWM device
* @chip_data: chip-private data associated with the PWM device * @chip_data: chip-private data associated with the PWM device
* @args: PWM arguments * @args: PWM arguments
* @state: curent PWM channel state * @state: last applied state
* @last: last implemented state (for PWM_DEBUG)
*/ */
struct pwm_device { struct pwm_device {
const char *label; const char *label;
...@@ -83,6 +84,7 @@ struct pwm_device { ...@@ -83,6 +84,7 @@ struct pwm_device {
struct pwm_args args; struct pwm_args args;
struct pwm_state state; struct pwm_state state;
struct pwm_state last;
}; };
/** /**
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment