Caesar, On Wed, Aug 6, 2014 at 8:37 PM, caesar <caesar.wang@xxxxxxxxxxxxxx> wrote: > Doug, > > 在 2014年08月07日 11:26, Doug Anderson 写道: > >> caesar, >> >> On Wed, Aug 6, 2014 at 8:23 PM, caesar <caesar.wang@xxxxxxxxxxxxxx> wrote: >>> >>> 在 2014年08月07日 10:16, Doug Anderson 写道: >>> >>>> Caesar, >>>> >>>> On Wed, Aug 6, 2014 at 6:27 PM, caesar <caesar.wang@xxxxxxxxxxxxxx> >>>> wrote: >>>>> >>>>> Doug, >>>>> >>>>> 在 2014年08月07日 06:46, Doug Anderson 写道: >>>>> >>>>>> Caesar, >>>>>> >>>>>> On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang >>>>>> <caesar.wang@xxxxxxxxxxxxxx> >>>>>> 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. > > I remember In patch v2 [1],Thierry suggests me to fix it so if I have no to > get wrong. > > [1]: https://lkml.org/lkml/2014/7/21/113 I think Thierry might not have realized that they were flipped... -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html