lkml.org 
[lkml]   [2014]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@rock-chips.com> wrote:
    > >
    > > 在 2014年08月07日 10:16, Doug Anderson 写道:
    > >
    > >> Caesar,
    > >>
    > >> On Wed, Aug 6, 2014 at 6:27 PM, caesar <caesar.wang@rock-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@rock-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
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2014-08-07 08:21    [W:6.874 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site