Re: [PATCH] pwm: add spear pwm driver support
From: Thierry Reding
Date: Thu Oct 18 2012 - 09:42:41 EST
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.hashim@xxxxxx>");
> > > +MODULE_AUTHOR("Viresh Kumar <viresh.kumar@xxxxxxxxxx>");
> > 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.kumar@xxxxxxxxxx>
> author: Shiraz Hashim <shiraz.hashim@xxxxxx>
> license: GPL
> alias: of:N*T*Cst,spear13xx-pwm*
> alias: of:N*T*Cst,spear-pwm*
> 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
> > > +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.
Description: PGP signature