Messages in this thread | | | Date | Tue, 21 Jan 2014 10:52:36 +0200 | From | Mika Westerberg <> | Subject | Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM |
| |
On Mon, Jan 20, 2014 at 01:28:14PM +0000, One Thousand Gnomes wrote: > > +static int pwm_lpss_config(struct pwm_chip *chip, struct pwm_device *pwm, > > + int duty_ns, int period_ns) > > +{ > > + struct pwm_lpss_chip *lpwm = to_lpwm(chip); > > + u8 on_time_div; > > + unsigned long c = clk_get_rate(lpwm->clk); > > + unsigned long long base_unit, hz = 1000000000UL; > > + u32 ctrl; > > + > > + do_div(hz, period_ns); > > + > > + /* The equation is: base_unit = ((hz / c) * 65536) + correction */ > > + base_unit = hz * 65536; > > + do_div(base_unit, c); > > + base_unit += PWM_DIVISION_CORRECTION; > > + if (base_unit > PWM_LIMIT) > > + return -EINVAL; > > + > > + if (duty_ns <= 0) > > + duty_ns = 1; > > + on_time_div = 255 - (255 * duty_ns / period_ns); > > + > > + ctrl = readl(lpwm->regs + PWM); > > + ctrl &= ~(PWM_BASE_UNIT_MASK | PWM_ON_TIME_DIV_MASK); > > + ctrl |= (u16) base_unit << PWM_BASE_UNIT_SHIFT; > > + ctrl |= on_time_div; > > + /* request PWM to update on next cycle */ > > + ctrl |= PWM_SW_UPDATE; > > + writel(ctrl, lpwm->regs + PWM); > > + > > Who handles the locking on all these functions. The pwm layer doesn't but > simnply exports stuff like pwm_config() directly to other bits of the > kernel so you are not guaranteed to be called via sysfs. > > (This btw looks to be a problem with a pile of the other pwm drivers, and > with the pwm core code which doesn't properly lock its own handling of > pwm->duty_cycle and pwm->period in pwm_config(), nor pwm->polarity in > pwm_set_polarity).
Good point. I checked few PWM drivers as well and none of them (including this one) does any locking around ->config().
> I think the core config methods need some kind of locking ?
Thierry, any comments on this?
| |