linux-kernel - RE: [PATCHv6 1/4] pwm: Add Freescale FTM 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: <1DD289F6464F0949A2FCA5AA6DC23F828E17D3@039-SN2MPN1-011.039d.mgd.msft.net>
Date:	Mon, 2 Dec 2013 02:45:07 +0000
From:	Li Xiubo <Li.Xiubo@...escale.com>
To:	Thierry Reding <thierry.reding@...il.com>
CC:	Shawn Guo <Shawn.Guo@...escale.com>,
	"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
	"swarren@...dotorg.org" <swarren@...dotorg.org>,
	"t.figa@...sung.com" <t.figa@...sung.com>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"rob@...dley.net" <rob@...dley.net>,
	"ian.campbell@...rix.com" <ian.campbell@...rix.com>,
	"mark.rutland@....com" <mark.rutland@....com>,
	"pawel.moll@....com" <pawel.moll@....com>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	Huan Wang <Huan.Wang@...escale.com>,
	Jingchang Lu <jingchang.lu@...escale.com>
Subject: RE: [PATCHv6 1/4] pwm: Add Freescale FTM PWM driver support

> > > > +#define FTM_CNTIN_VAL       0x00
> > >
> > > Do we really need this?
> > >
> >
> > Maybe not, I think that the initial value maybe modified in the future.
> > And this can be more easy to ajust it.
> 
> Why would it need to be modified?
> 

Well, for the PWM function modifying it make no sense, so I'll remove this. 


> > > > +	writel(period_cycles + cntin - 1, fpc->base + FTM_MOD);
> > > > +	writel(duty_cycles + cntin, fpc->base + FTM_CV(pwm->hwpwm));
> > >
> > > And these:
> > >
> > > 	writel(period - 1, fpc->base + FTM_MOD);
> > > 	writel(duty, fpc->base + FTM_CV(pwm->hwpwm));
> > >
> > > Although now that I think about it, this seems broken. The period is
> > > set in a global register, so I assume it is valid for all channels.
> > > What if you want to use different periods for individual channels?
> > > The way I read this the last one to be configured will win and
> > > change the period to whatever it wants. Other channels won't even
> notice.
> > >
> >
> > That's right. And all the 8 channels share the same period settings.
> >
> > > Is there a way to set the period per channel?
> > >
> >
> > Not yet. Only could we do is to set the duty value individually for
> > each channel. So here is a limitation for the cusumers that all the 8
> channels'
> > period values should be the same.
> 
> That will need to be handled some way. Perhaps the easiest would be to
> check whether or not another channel is already running and check that
> indeed the period as requested by the new channel matches that of any
> running channels. If it doesn't match, then pwm_config() should fail.
> 
> When you do that you can also optimize this a bit by only setting the
> frequency once. Whenever a new channel is configured it will have the same
> period and therefore the register doesn't need to be reprogrammed.
> 

Yes, if it dosen't match it should fail, or nothing to do with the period
register if it's not the first channel to be configured.


> > > > +static void fsl_pwm_disable(struct pwm_chip *chip, struct
> > > > +pwm_device
> > > *pwm)
> > > > +{
> > > > +	struct fsl_pwm_chip *fpc;
> > > > +
> > > > +	fpc = to_fsl_chip(chip);
> > > > +
> > > > +	fsl_counter_clock_disable(fpc);
> > > > +}
> > >
> > > Same here. Since you can't propagate the error, perhaps an error
> > > message would be appropriate here?
> > >
> >
> > Well, in fsl_counter_clock_disable(fpc); only '0' could be returned,
> > maybe let it a void type value to return is better.
> > Just like:
> > static void fsl_counter_clock_disable(struct fsl_pwm_chip *fpc) 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/