Messages in this thread | | | Date | Tue, 7 Jun 2011 09:25:25 +0530 | From | viresh kumar <> | Subject | Re: [PATCH V2 1/3] drivers/pwm st_pwm: Add support for ST's Pulse Width Modulator |
| |
On 06/07/2011 06:03 AM, Andrew Morton wrote: > On Tue, 31 May 2011 14:21:51 +0530 > Viresh Kumar <viresh.kumar@st.com> wrote:
>> + * lock: lock specific to a pwm device > > More specificity here would be helpful. Precisely which data does the > lock protect? >
>> + * lock: lock specific to current pwm ip > > Ditto. >
Ok.
>> +int pwm_config(struct pwm_device *pwmd, int duty_ns, int period_ns) >> +{ >> + u64 val, div, clk_rate; >> + unsigned long prescale = MIN_PRESCALE, pv, dc; >> + int ret = 0; >> + >> + if (!pwmd) { >> + pr_err("pwm: config - NULL pwm device pointer\n"); >> + return -EFAULT; >> + } >> + >> + if (period_ns == 0 || duty_ns > period_ns) { >> + ret = -EINVAL; >> + goto err; >> + } >> + >> + /* TODO: Need to optimize this loop */ >> + while (1) { >> + div = 1000000000; >> + div *= 1 + prescale; >> + clk_rate = clk_get_rate(pwmd->pwm->clk); >> + val = clk_rate * period_ns; >> + pv = div64_u64(val, div); >> + val = clk_rate * duty_ns; >> + dc = div64_u64(val, div); >> + >> + if ((pv == 0) || (dc == 0)) { >> + ret = -EINVAL; >> + goto err; >> + } >> + if ((pv > MAX_PERIOD) || (dc > MAX_DUTY)) { >> + prescale++; >> + if (prescale > MAX_PRESCALE) { >> + ret = -EINVAL; >> + goto err; >> + } >> + continue; >> + } >> + if ((pv < MIN_PERIOD) || (dc < MIN_DUTY)) { >> + ret = -EINVAL; >> + goto err; >> + } >> + break; >> + } > > gee, is this some sort of puzzle? So human-readable description of > what this code is doing would be an improvement. >
Sure. Will add that.
>> + spin_lock(&pwmd->pwm->lock); >> + ret = clk_enable(pwmd->pwm->clk); >> + if (ret) { >> + spin_unlock(&pwmd->pwm->lock); >> + goto err; >> + } >> + >> + spin_lock(&pwmd->lock); >> + writel(prescale << PRESCALE_SHIFT, pwmd->pwm->mmio_base + >> + pwmd->offset + PWMCR); >> + writel(dc, pwmd->pwm->mmio_base + pwmd->offset + PWMDCR); >> + writel(pv, pwmd->pwm->mmio_base + pwmd->offset + PWMPCR); >> + spin_unlock(&pwmd->lock); >> + clk_disable(pwmd->pwm->clk); >> + spin_unlock(&pwmd->pwm->lock); > > The nesting rules for these two locks seems sensible and obvious, but I > guess documenting the rule wouldn't hurt. >
Ok.
>> + return 0; >> +err: >> + dev_err(&pwmd->pwm->pdev->dev, "pwm config fail\n"); >> + return ret; >> +} >> +EXPORT_SYMBOL(pwm_config); >> + >> >> ... >> >> +static int __devinit st_pwm_probe(struct platform_device *pdev) > > And here things get rather odd. > > Most of this file is a generic, non-device specific PWM layer, exported > to other modules. But then we get into driver bits which are specific > to one paritular type of device. Confused - this is like putting the > e100 driver inside net/ipv4/tcp.c? >
Sorry but i couldn't get this one completely. :( Driver is specific to pwm peripheral by ST. This driver can be used for SPEAr or may be other SoC or Devices, and is not at all dependent on SPEAr.
-- viresh
| |