Re: [PATCH] pwm: add support for Intel Low Power Subsystem PWM

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2755 bytes --]

On Tue, Jan 21, 2014 at 10:52:36AM +0200, Mika Westerberg wrote:
> 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?

The idea behind this is that only a single user can have access to a
given PWM device at a time. The PWM device's PWMF_REQUESTED flag is set
(and cleared) under the pwm_lock and any subsequent users will not be
able to use that specific device (pwm_request() return -EBUSY).

There is obviously an assumption here that each user knows what they are
doing and aren't calling any of the public pwm_*() functions
concurrently. I haven't come across a situation where this is actually a
problem because typically these functions are called either via sysfs or
some other higher-level where synchronization is already properly
handled.

So the only thing that drivers should be taking care of is synchronizing
access to registers common to multiple PWM devices.

Does that clarify things?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]