Hello,
On Sun, Feb 04, 2024 at 11:08:51PM +0100, Stefan Wahren wrote:
> +static enum hrtimer_restart pwm_gpio_timer(struct hrtimer *gpio_timer)
> +{
> + struct pwm_gpio *gpwm = container_of(gpio_timer, struct pwm_gpio,
> + gpio_timer);
> + unsigned long next_toggle;
> + unsigned long flags;
> + bool new_level;
> +
> + spin_lock_irqsave(&gpwm->lock, flags);
> +
> + /* Apply new state at end of current period */
> + if (!gpwm->level && gpwm->changing) {
> + gpwm->changing = false;
> + gpwm->state = gpwm->next_state;
> + new_level = !!gpwm->state.duty_cycle;
> + } else {
> + new_level = !gpwm->level;
> + }
> +
> + next_toggle = pwm_gpio_toggle(gpwm, new_level);
> + if (next_toggle) {
> + hrtimer_forward(gpio_timer, hrtimer_get_expires(gpio_timer),
> + ns_to_ktime(next_toggle));
How does this work in relation to hrtimer_resolution? If the resolution
is (say) 300 and next_toggle is 2000. Does the trigger trigger at
hrtimer_get_expires(...) + 1800, or at ... + 2100?
If you assume we have period = 6000 and duty_cycle = 2000, the delta to
forward the driver alternates between 1800 and 3900 (if rounding down)
or between 2100 and 4200 if rounding up. Both is not optimal.
Ideally you'd round down the active phase (i.e. pick 1800) and for the
inactive phase you'd use rounddown(period) - rounddown(duty_cycle) which
gives 4200 here. Does this make sense?
> + }
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return next_toggle ? HRTIMER_RESTART : HRTIMER_NORESTART;
> +}
> +
> +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
> + bool invert = state->polarity == PWM_POLARITY_INVERSED;
> + unsigned long flags;
> +
> + if (state->duty_cycle && state->duty_cycle < hrtimer_resolution)
> + return -EINVAL;
> +
> + if (state->duty_cycle != state->period &&
> + (state->period - state->duty_cycle < hrtimer_resolution))
> + return -EINVAL;
If you assume that hrtimer_resolution = 300 again, you don't want to
refuse
.duty_cycle = 200
.period = 200
do you? I think you want:
mininterval = min(state->duty_cycle, state->period - state->duty_cycle);
if (mininterval && mininterval < hrtimer_resolution)
return -EINVAL;
to catch both cases in a single check.
> + if (!state->enabled) {
> + hrtimer_cancel(&gpwm->gpio_timer);
> + } else if (!gpwm->running) {
> + /*
> + * This just enables the output, but pwm_gpio_toggle()
> + * really starts the duty cycle.
> + */
> + int ret = gpiod_direction_output(gpwm->gpio, invert);
> +
> + if (ret)
> + return ret;
> + }
> +
> + spin_lock_irqsave(&gpwm->lock, flags);
> +
> + if (!state->enabled) {
> + gpwm->state = *state;
> + gpwm->running = false;
> + gpwm->changing = false;
> +
> + gpiod_set_value(gpwm->gpio, invert);
> + } else if (gpwm->running) {
> + gpwm->next_state = *state;
> + gpwm->changing = true;
> + } else {
> + unsigned long next_toggle;
> +
> + gpwm->state = *state;
> + gpwm->changing = false;
> +
> + next_toggle = pwm_gpio_toggle(gpwm, !!state->duty_cycle);
> + if (next_toggle) {
> + hrtimer_start(&gpwm->gpio_timer, next_toggle,
> + HRTIMER_MODE_REL);
> + }
The curly braces can be dropped here and in a few more locations.
> + }
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return 0;
> +}
> +
> +static int pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gpwm->lock, flags);
> +
> + if (gpwm->changing)
> + *state = gpwm->next_state;
> + else
> + *state = gpwm->state;
> +
> + spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops pwm_gpio_ops = {
> + .apply = pwm_gpio_apply,
> + .get_state = pwm_gpio_get_state,
> +};
> +
> +static int pwm_gpio_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pwm_gpio *gpwm;
> + int ret;
> +
> + gpwm = devm_kzalloc(dev, sizeof(*gpwm), GFP_KERNEL);
> + if (!gpwm)
> + return -ENOMEM;
> +
> + spin_lock_init(&gpwm->lock);
> +
> + gpwm->gpio = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> + if (IS_ERR(gpwm->gpio)) {
> + return dev_err_probe(dev, PTR_ERR(gpwm->gpio),
> + "could not get gpio\n");
> + }
> +
> + if (gpiod_cansleep(gpwm->gpio)) {
> + return dev_err_probe(dev, -EINVAL,
> + "sleeping GPIO %d not supported\n",
> + desc_to_gpio(gpwm->gpio));
> + }
Is it still state of the art to add gpio numbers to error messages?
> + gpwm->chip.dev = dev;
> + gpwm->chip.ops = &pwm_gpio_ops;
> + gpwm->chip.npwm = 1;
> + gpwm->chip.atomic = true;
> +
> + hrtimer_init(&gpwm->gpio_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + gpwm->gpio_timer.function = pwm_gpio_timer;
> +
> + ret = pwmchip_add(&gpwm->chip);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "could not add pwmchip\n");
> +
> + platform_set_drvdata(pdev, gpwm);
> +
> + return 0;
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
