Re: [PATCH] pwm: add spear pwm driver support

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 4082 bytes --]

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@st.com>");
> > > +MODULE_AUTHOR("Viresh Kumar <viresh.kumar@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.kumar@linaro.com>
> author:         Shiraz Hashim <shiraz.hashim@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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]