linux-kernel - Re: [PATCH] pwm: add spear pwm driver support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20121018134245.GA30997@avionic-0098.mockup.avionic-design.de>
Date:	Thu, 18 Oct 2012 15:42:45 +0200
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Shiraz Hashim <shiraz.hashim@...com>
Cc:	spear--sw-devel@...ts.codex.cro.st.com,
	linux-kernel@...r.kernel.org, spear-devel@...t.st.com,
	Viresh Kumar <viresh.kumar@...aro.org>
Subject: Re: [PATCH] pwm: add spear pwm driver support

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

Content of type "application/pgp-signature" skipped