linux-kernel - Re: [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC

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]

Date:	Thu, 7 Aug 2014 08:12:57 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Doug Anderson <dianders@...omium.org>
Cc:	caesar <caesar.wang@...k-chips.com>,
	Heiko Stübner <heiko@...ech.de>,
	Beniamino Galvani <b.galvani@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Randy Dunlap <rdunlap@...radead.org>,
	Kumar Gala <galak@...eaurora.org>,
	Eddie Cai <cf@...k-chips.com>,
	Tao Huang <huangtao@...k-chips.com>,
	Jianqun Xu <xjq@...k-chips.com>,
	Addy Ke <addy.ke@...k-chips.com>,
	陈渐飞 <cjf@...k-chips.com>,
	han jiang <hj@...k-chips.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	linux-pwm <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>
Subject: Re: [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC

On Wed, Aug 06, 2014 at 08:26:51PM -0700, Doug Anderson wrote:
> caesar,
> 
> On Wed, Aug 6, 2014 at 8:23 PM, caesar <caesar.wang@...k-chips.com> wrote:
> >
> > 在 2014年08月07日 10:16, Doug Anderson 写道:
> >
> >> Caesar,
> >>
> >> On Wed, Aug 6, 2014 at 6:27 PM, caesar <caesar.wang@...k-chips.com> wrote:
> >>>
> >>> Doug,
> >>>
> >>> 在 2014年08月07日 06:46, Doug Anderson 写道:
> >>>
> >>>> Caesar,
> >>>>
> >>>> On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang
> >>>> <caesar.wang@...k-chips.com>
> >>>> wrote:
> >>>>>
> >>>>> +static const struct rockchip_pwm_data pwm_data_v1 = {
> >>>>> +       .regs.duty = PWM_HRC,
> >>>>> +       .regs.period = PWM_LRC,
> >>>>> +       .regs.cntr = PWM_CNTR,
> >>>>> +       .regs.ctrl = PWM_CTRL,
> >>>>> +       .prescaler = PRESCALER,
> >>>>> +       .set_enable = rockchip_pwm_set_enable_v1,
> >>>>> +};
> >>>>> +
> >>>>> +static const struct rockchip_pwm_data pwm_data_v2 = {
> >>>>> +       .regs.duty = PWM_LRC,
> >>>>> +       .regs.period = PWM_HRC,
> >>>>> +       .regs.cntr = PWM_CNTR,
> >>>>> +       .regs.ctrl = PWM_CTRL,
> >>>>> +       .prescaler = PRESCALER-1,
> >>>>> +       .set_enable = rockchip_pwm_set_enable_v2,
> >>>>> +};
> >>>>> +
> >>>>> +static const struct rockchip_pwm_data pwm_data_vop = {
> >>>>> +       .regs.duty = PWM_LRC,
> >>>>> +       .regs.period = PWM_HRC,
> >>>>> +       .regs.cntr = PWM_CTRL,
> >>>>> +       .regs.ctrl = PWM_CNTR,
> >>>>
> >>>> Did you really mean to flip CTRL and CNTR here?  If so, that's super
> >>>> confusing and deserves a comment.  AKA, I think the above should not
> >>>> be:
> >>>>
> >>>>    +       .regs.cntr = PWM_CTRL,
> >>>>    +       .regs.ctrl = PWM_CNTR,
> >>>>
> >>>> ...but should be
> >>>>
> >>>>    +       .regs.cntr = PWM_CNTR,
> >>>>    +       .regs.ctrl = PWM_CTRL,
> >>>>
> >>>> If you didn't mean to flip CTRL and CNTR here, then just get rid of
> >>>> pwm_data_vop and refer to pwm_data_v2.  In fact, I'd suggest that you
> >>>> totally remove the "rockchip,vop-pwm" since there's nothing different
> >>>> between "rockchip,rk3288-pwm" and "rockchip,vop-pwm".
> >>>
> >>>
> >>> Sorry,I think it's no problem. the  "rockchip,rk3288-pwm" and
> >>> "rockchip,vop-pwm" are seperate PWM controllers.
> >>> They are just different registers address between CNTR and CTRL .
> >>
> >> OK, I looked up in the TRM.  Right, the CNTR and CTRL are flipped on
> >> the vop.  So I think that the only change you need is to add:
> >>
> >> #define PWM_VOP_CTRL  0x00
> >> #define PWM_VOP_CNTR  0x0c
> >>
> >> ...then use these new #defines for the vop structure.
> >>
> >>
> >> As you have the code written right now it's very confusing.  The new
> >> #defines will fix this.
> >>
> > yeah, I think they can be used in the same context.
> >
> > I will fix it in patch v5 if it is really need.
> 
> I think you should fix this, but if Thierry doesn't think so then it's
> really his decision.

Frankly, I'm fine if these don't use symbolic names at all since only
the structure fields are used to refer to them now.

Thierry

Content of type "application/pgp-signature" skipped