On Thu, Oct 18, 2012 at 06:59:28PM +0530, Shiraz Hashim wrote: > Hi Thierry, > > Thanks for the quick review. > > On Thu, Oct 18, 2012 at 02:08:20PM +0200, Thierry Reding wrote: > > On Thu, Oct 18, 2012 at 04:58:32PM +0530, Shiraz Hashim wrote: [...] > > > + first cell specifies the per-chip index of the PWM to use and the > > > second > > > + cell is the duty cycle in nanoseconds. > > > > This should be "period in nanoseconds". I know this is wrong in the > > binding documentation for other drivers but I've recently committed a > > patch that fixes it. > > Okay but I couldn't see use of pwm->period set by of_pwm_simple_xlate > anywhere. Am I missing something ?
It's set by the call to pwm_set_period().
> > > +/* PWM registers and bits definitions */
> > > +#define PWMCR 0x00 /* Control Register */
> > > +#define PWMDCR 0x04 /* Duty Cycle Register */
> > > +#define PWMPCR 0x08 /* Period Register */
> > > +/* Following only available on 13xx SoCs */
> > > +#define PWMMCR 0x3C /* Master Control Register */
> > > +
> > > +#define PWM_ENABLE 0x1
> > > +
> > > +#define MIN_PRESCALE 0x00
> > > +#define MAX_PRESCALE 0x3FFF
> > > +#define PRESCALE_SHIFT 2
> > > +
> > > +#define MIN_DUTY 0x0001
> > > +#define MAX_DUTY 0xFFFF
> > > +
> > > +#define MIN_PERIOD 0x0001
> > > +#define MAX_PERIOD 0xFFFF
> >
> > Would it make sense to perhaps group the bitfields with the matching
> > register definitions to make their use more obvious. Also I prefer
> > lowercase hexadecimal digits, but that's pure bikeshedding.
> >
>
> Sure I would group them, but uppercase hexadecimal digits clearly
> seperate the value (number) which otherwise can be mixed (while
> reading) with normal letters. Isn't it ?
As I said, this is really very subjective, so if you prefer uppercase,
feel free to keep it. =)
> > > +static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned
> > > int num,
> > > + unsigned long offset)
> >
> > I personally like it better to have function arguments aligned, like so:
> >
> > static inline u32 spear_pwm_readl(struct spear_pwm_chip *chip, unsigned int
> > num,
> > unsigned long offset)
> >
> > Note, those are as many 8-spaces tabs with only spaces to align them
> > properly. But again, pure bikeshedding and I won't force the issue.
> >
>
> Would do that. Are you aware of some (vim) configuration which can
> autmatically do this while editing code ?
I'm not aware of any such setting, but the idea is interesting. I
usually do that automatically out of habit, but having the editor do it
would be nice as well.
> > __devinit will go away sometime soon, so please don't use it in new
> > code.
> >
>
> Okay. You mean all init sections would eventually be removed. I
> would read more about it.
Yes, commit 45f035a (only in linux-next I think) has some details.
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Shiraz Hashim <shiraz.has...@st.com>");
> > > +MODULE_AUTHOR("Viresh Kumar <viresh.ku...@linaro.com>");
> >
> > I don't think this works: the second entry will replace the first. Have
> > you verified that both authors appear in the output of modinfo?
>
> This is the output of modinfo (compiled for linux-3.5)
>
> $ modinfo pwm-spear.ko
> filename: drivers/pwm/pwm-spear.ko
> alias: platform:st-pwm
> author: Viresh Kumar <viresh.ku...@linaro.com>
> author: Shiraz Hashim <shiraz.has...@st.com>
> license: GPL
> alias: of:N*T*Cst,spear13xx-pwm*
> alias: of:N*T*Cst,spear-pwm*
> depends:
> intree: Y
> vermagic: 3.5.0-test-00138-g08e3584 SMP mod_unload modversions ARMv7
> p2v8
Interesting. I thought I'd seen this fail only recently. Will need to
investigate.
> > > +MODULE_ALIAS("platform:st-pwm");
> >
> > Should this not rather be "platform:spear-pwm"?
>
> Yes, I would double check these kind of small issues before
> sending patches next time.
No biggie. That's why we have reviews.
Thierry
pgp6NF7dQPYDX.pgp
Description: PGP signature
