Hello Nicola,
On Sun, Jan 17, 2021 at 10:06:18PM +0100, Nicola Di Lieto wrote:
> On Sun, Jan 17, 2021 at 07:45:56PM +0100, Uwe Kleine-König wrote:
> > > > > + pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert;
> >
> > So far I understood also only comment. What wasn't obvious immediately
> > is the state member.
>
> Would it become clear enough by adding: "state is the logical PWM output;
> the actual PIN output level is inverted by XORing with cur.invert when the
> latter is true" ?
This was at least good enough for me to understand it now.
So iff state is true, the PWM is in the active phase of the current
period. Maybe "currently_active" is a better name for this variable?
Then the code could (with some comments added and a few more variables
renamed) could look as follows:
if (ddata->currently_active) {
/* Enter the inactive part of the current period. */
ddata->currently_active = false;
next_transistion = ddata->cur.toff_ns;
} else {
/*
* Start a new period. First check if there is a new
* configuration setting pending in ddata->new.
*/
ddata->currently_active = true;
if (spin_trylock(&ddata->lock)) {
ddata->cur = ddata->new;
spin_unlock(&ddata->lock);
}
next_transition = ddata->cur.ton_ns;
}
...
which IMHO is easier to understand.
I think there are still two problems with this approach:
- The locking is hard to follow, .enabled is accessed using atomic
accessors, .new is protected by the spinlock and the other members
are not accessed concurrently, right?
If pwm_apply(..., {.enabled = false}) and pwm_apply(.., {.enabled =
true}) are called in quick sequence (e.g. faster than the first call
triggers the work queue) there is trouble ahead, isn't there?
- If .duty_cycle is equal to 0 (or .period) the output should be
constant. I think this isn't what will happen.
> > > Would it be ok to cancel the timer first and then "return
> > > pwmchip_remove(...)"?
> >
> > No. The PWM must stay functional until pwmchip_remove() returns.
> >
>
> Could you please clarify what I should do when pwmchip_remove returns
> non-zero? In my original implementation
> - if pwmchip_remove returns a non-zero error code, I return it to the
> caller and do not cancel the timer.
> - if pwmchip_remove returns zero, I cancel the timer and return zero to the
> caller
IMHO it's a bug that pwmchip_remove() can return an error code. I think
the best you can do currently is:
ret = pwmchip_remove(...)
WARN_ON(ret);
hrtimer_cancel(..);
return 0;
because whatever you do is wrong. To sort this out needs some thought
and work in the framework and so is unrelated to this patch.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
