Hi Viresh, On Fri, Oct 19, 2012 at 7:14 PM, Viresh Kumar <viresh.ku...@linaro.org> wrote: > On 19 October 2012 15:45, Shiraz Hashim <shiraz.has...@st.com> wrote: >> diff --git a/Documentation/devicetree/bindings/pwm/spear-pwm.txt >> b/Documentation/devicetree/bindings/pwm/spear-pwm.txt >> + pwm: pwm@a8000000 { >> + compatible ="st,spear320-pwm"; >> + reg = <0xa8000000 0x1000>; >> + #pwm-cells = <2>; >> + status = "disabled"; > > Must remove disabled from here. Isn't it?
yes, would remove it. >> diff --git a/drivers/pwm/pwm-spear.c b/drivers/pwm/pwm-spear.c >> +#include <linux/clk.h> >> +#include <linux/err.h> >> +#include <linux/io.h> >> +#include <linux/ioport.h> >> +#include <linux/kernel.h> >> +#include <linux/math64.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/pwm.h> >> +#include <linux/slab.h> >> +#include <linux/types.h> >> + >> +#define NUM_PWM 4 >> + >> +/* PWM registers and bits definitions */ >> +#define PWMCR 0x00 /* Control Register */ >> +#define PWMCR_PWM_ENABLE 0x1 >> +#define PWMCR_PRESCALE_SHIFT 2 >> +#define PWMCR_MIN_PRESCALE 0x00 >> +#define PWMCR_MAX_PRESCALE 0x3FFF > > I would do it as to make it more readable, your call: > > #define PWMCR 0x00 /* Control Register */ > #define PWMCR_PWM_ENABLE 0x1 > #define PWMCR_PRESCALE_SHIFT 2 > #define PWMCR_MIN_PRESCALE 0x00 > #define PWMCR_MAX_PRESCALE 0x3FFF There are some who don't like this (personally I see it quite clear), so it becomes a matter of choice. I intentionaly prefixed each bit definition by its register name to make association clear. > >> +static int spear_pwm_remove(struct platform_device *pdev) >> +{ >> + struct spear_pwm_chip *pc = platform_get_drvdata(pdev); >> + int i; >> + >> + if (WARN_ON(!pc)) >> + return -ENODEV; > > Sorry for not asking earlier, how can this be true anytime? Probably never :), just copied from some other implementation. Would remove this in V3. -- regards Shiraz Hashim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/