On Wed, Sep 08, 2010 at 21:37:55, Kevin Hilman wrote: > Sugumar Natarajan <[email protected]> writes: > > > This patch adds generic PWM support where it maintains the > > list of PWM control devices that can be added or removed. > > > > The interface provides a list of functions that can be accessed > > by the PWM control driver module and the generic PWM driver. > > > > The PWM control driver module such as eCAP uses the interface to > > register and add itself to the list as a PWM control device. > > The generic PWM driver uses the interface to search for a PWM control > > device and if present, uses the device for PWM control. > > > > Signed-off-by: Sugumar Natarajan <[email protected]> > > Some more comments below... > > > --- > > Since v1, following changes have been made: > > 1.add_pwm and remove_pwm functions have been renamed to pwm_add > > and pwm_remove respectively. > > > > arch/arm/mach-davinci/Makefile | 3 + > > arch/arm/mach-davinci/davinci_pwm.c | 116 > > ++++++++++++++++++++++ > > arch/arm/mach-davinci/include/mach/davinci_pwm.h | 32 ++++++ > > 3 files changed, 151 insertions(+), 0 deletions(-) > > create mode 100644 arch/arm/mach-davinci/davinci_pwm.c > > create mode 100644 arch/arm/mach-davinci/include/mach/davinci_pwm.h > > > > diff --git a/arch/arm/mach-davinci/Makefile b/arch/arm/mach-davinci/Makefile > > index a7a70d1..90ca821 100644 > > --- a/arch/arm/mach-davinci/Makefile > > +++ b/arch/arm/mach-davinci/Makefile > > @@ -39,3 +39,6 @@ obj-$(CONFIG_MACH_MITYOMAPL138) += > > board-mityomapl138.o > > obj-$(CONFIG_CPU_FREQ) += cpufreq.o > > obj-$(CONFIG_CPU_IDLE) += cpuidle.o > > obj-$(CONFIG_SUSPEND) += pm.o sleep.o > > + > > +# Generic PWM control support > > +obj-$(CONFIG_HAVE_PWM) += davinci_pwm.o > > diff --git a/arch/arm/mach-davinci/davinci_pwm.c > > b/arch/arm/mach-davinci/davinci_pwm.c > > new file mode 100644 > > index 0000000..aa21b7a > > --- /dev/null > > +++ b/arch/arm/mach-davinci/davinci_pwm.c > > @@ -0,0 +1,116 @@ > > +/* > > + * Copyright (C) 2010 Texas Instruments Incorporated - http://www.ti.com/ > > + * > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed .as is. WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/kernel.h> > > +#include <linux/platform_device.h> > > +#include <linux/err.h> > > +#include <linux/clk.h> > > +#include <linux/io.h> > > +#include <linux/pwm.h> > > +#include <mach/davinci_pwm.h> > > + > > +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns) > > +{ > > + unsigned int clock_freq; > > + unsigned int period_cycles; > > + unsigned int duty_cycle; > > + > > + if (!pwm || !pwm->pwm_config_device || !period_ns || > > + duty_ns > period_ns) > > + return -EINVAL; > > Is it required to have a pwm_config_device callback? To be more > flexible, it might be better... > > > + clock_freq = clk_get_rate(pwm->clk) / USEC_PER_SEC; > > + period_cycles = (clock_freq * period_ns) / MSEC_PER_SEC; > > + duty_cycle = (clock_freq * duty_ns) / MSEC_PER_SEC; > > ... to add 'if (pwm->pwm_config_device)' here > > > + pwm->pwm_config_device(pwm, period_cycles, duty_cycle); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(pwm_config); > > + > > +int pwm_enable(struct pwm_device *pwm) > > +{ > if (WARN_ON(!pwm)) > return -EINVAL; > > For a little more safety, all of these functions should > sanity-check/warn if a NULL pwm pointer is passed in. > > > + return clk_enable(pwm->clk); > > +} > > +EXPORT_SYMBOL(pwm_enable); > > + > > +void pwm_disable(struct pwm_device *pwm) > > +{ > > + clk_disable(pwm->clk); > > +} > > +EXPORT_SYMBOL(pwm_disable); > > + > > +static DEFINE_MUTEX(pwm_lock); > > +static LIST_HEAD(pwm_list); > > + > > +struct pwm_device *pwm_request(int pwm_id, const char *label) > > +{ > > + struct pwm_device *pwm; > > + int found = 0; > > A relatively minor issue, but I'm not crazy about the 'found' flag. > Here's a proposal for a slightly cleaner, and more compact way to handle > this: > > struct pwm_device *tmp_pwm, *pwm = ERR_PTR(-ENOENT); > > > + mutex_lock(&pwm_lock); > > + > > + list_for_each_entry(pwm, &pwm_list, node) { > > Here, iterate using 'tmp_pwm' > > > + if (pwm->pwm_id == pwm_id) { > > + found = 1; > > and instead of 'found = 1', do 'pwm = tmp_pwm'; > > > + break; > > + } > > + } > > + > > + if (found) { > > this can be 'if (pwm) {' > > > + if (pwm->use_count == 0) { > > + pwm->use_count++; > > + pwm->label = label; > > + } else { > > + pwm = ERR_PTR(-EBUSY); > > + } > > + } else { > > + pwm = ERR_PTR(-ENOENT); > > + } > > and this else clause can be dropped > > > + mutex_unlock(&pwm_lock); > > + return pwm; > > +} > > +EXPORT_SYMBOL(pwm_request); > > [...] >
Kevin, I received your comments . I will modify and incorporate the changes in the next version. Regards, N.sugumar _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
