| Message ID | 1406197295-10604-3-git-send-email-caesar.wang@rock-chips.com |
|---|---|
| State | Rejected |
| Headers | show |
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". Have you validated Thierry's suggestion to allow you to access your memory range? -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 . > > Have you validated Thierry's suggestion to allow you to access your > memory range? Yes,we have solve it in lcdc driver. The Mark Yao have the submission in [0]. [0]: https://lkml.org/lkml/2014/8/4/20 > > -Doug > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. >> Have you validated Thierry's suggestion to allow you to access your >> memory range? > > Yes,we have solve it in lcdc driver. > The Mark Yao have the submission in [0]. > > [0]: https://lkml.org/lkml/2014/8/4/20 Excellent! Then we should be able to land after you fix the above. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
在 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. > >>> Have you validated Thierry's suggestion to allow you to access your >>> memory range? >> Yes,we have solve it in lcdc driver. >> The Mark Yao have the submission in [0]. >> >> [0]: https://lkml.org/lkml/2014/8/4/20 > Excellent! Then we should be able to land after you fix the above. > > -Doug > > > OK,Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Doug, 在 2014年08月07日 11:26, Doug Anderson 写道: > 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. 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Caesar, On Wed, Aug 6, 2014 at 8:37 PM, caesar <caesar.wang@rock-chips.com> wrote: > Doug, > > 在 2014年08月07日 11:26, Doug Anderson 写道: > >> 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. > > 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 linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Doug, 在 2014年08月07日 11:46, Doug Anderson 写道: > Caesar, > > On Wed, Aug 6, 2014 at 8:37 PM, caesar <caesar.wang@rock-chips.com> wrote: >> Doug, >> >> 在 2014年08月07日 11:26, Doug Anderson 写道: >> >>> 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. >> 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 > > > Maybe you are right. I will sent patch v5 fix the about tomorrow if it has no other problems. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
On Thu, Jul 24, 2014 at 06:21:35PM +0800, Caesar Wang wrote: > This patch added to support the PWM controller found on > RK3288 SoC. > > Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com> > --- > drivers/pwm/pwm-rockchip.c | 124 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 105 insertions(+), 19 deletions(-) > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > index eec2145..59c2513 100644 > --- a/drivers/pwm/pwm-rockchip.c > +++ b/drivers/pwm/pwm-rockchip.c > @@ -2,6 +2,7 @@ > * PWM driver for Rockchip SoCs > * > * Copyright (C) 2014 Beniamino Galvani <b.galvani@gmail.com> > + * Copyright (C) 2014 ROCKCHIP, Inc. > * > * This program is free software; you can redistribute it and/or > * modify it under the terms of the GNU General Public License > @@ -12,6 +13,7 @@ > #include <linux/io.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/pwm.h> > #include <linux/time.h> > @@ -25,17 +27,72 @@ > > #define PRESCALER 2 > > +#define PWM_ENABLE (1 << 0) > +#define PWM_CONTINUOUS (1 << 1) > +#define PWM_DUTY_POSITIVE (1 << 3) > +#define PWM_INACTIVE_NEGATIVE (0 << 4) > +#define PWM_OUTPUT_LEFT (0 << 5) > +#define PWM_LP_DISABLE (0 << 8) > + > struct rockchip_pwm_chip { > struct pwm_chip chip; > struct clk *clk; > + const struct rockchip_pwm_data *data; > void __iomem *base; > }; > > +struct rockchip_pwm_regs { > + unsigned long duty; > + unsigned long period; > + unsigned long cntr; > + unsigned long ctrl; > +}; > + > +struct rockchip_pwm_data { > + struct rockchip_pwm_regs regs; > + unsigned int prescaler; > + > + void (*set_enable)(struct pwm_chip *chip, bool enable); > +}; > + > static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c) > { > return container_of(c, struct rockchip_pwm_chip, chip); > } > > +static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable) > +{ > + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); > + u32 val = 0; > + u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN; > + > + val = readl_relaxed(pc->base + pc->data->regs.ctrl); > + > + if (enable) > + val |= enable_conf; > + else > + val &= ~enable_conf; > + > + writel_relaxed(val, pc->base + pc->data->regs.ctrl); > +} > + > +static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable) > +{ > + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); > + u32 val = 0; > + u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE | > + PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE; > + > + val = readl_relaxed(pc->base + pc->data->regs.ctrl); > + > + if (enable) > + val |= enable_conf; > + else > + val &= ~enable_conf; > + > + writel_relaxed(val, pc->base + pc->data->regs.ctrl); > +} > + > static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > int duty_ns, int period_ns) > { > @@ -52,20 +109,20 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > * default prescaler value for all practical clock rate values. > */ > div = clk_rate * period_ns; > - do_div(div, PRESCALER * NSEC_PER_SEC); > + do_div(div, pc->data->prescaler * NSEC_PER_SEC); > period = div; > > div = clk_rate * duty_ns; > - do_div(div, PRESCALER * NSEC_PER_SEC); > + do_div(div, pc->data->prescaler * NSEC_PER_SEC); > duty = div; > > ret = clk_enable(pc->clk); > if (ret) > return ret; > > - writel(period, pc->base + PWM_LRC); > - writel(duty, pc->base + PWM_HRC); > - writel(0, pc->base + PWM_CNTR); > + writel(period, pc->base + pc->data->regs.period); > + writel(duty, pc->base + pc->data->regs.duty); > + writel(0, pc->base + pc->data->regs.cntr); > > clk_disable(pc->clk); > > @@ -76,15 +133,12 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > { > struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); > int ret; > - u32 val; > > ret = clk_enable(pc->clk); > if (ret) > return ret; > > - val = readl_relaxed(pc->base + PWM_CTRL); > - val |= PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN; > - writel_relaxed(val, pc->base + PWM_CTRL); > + pc->data->set_enable(chip, true); > > return 0; > } > @@ -92,11 +146,8 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > { > struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); > - u32 val; > > - val = readl_relaxed(pc->base + PWM_CTRL); > - val &= ~(PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN); > - writel_relaxed(val, pc->base + PWM_CTRL); > + pc->data->set_enable(chip, false); > > clk_disable(pc->clk); > } > @@ -108,12 +159,52 @@ static const struct pwm_ops rockchip_pwm_ops = { > .owner = THIS_MODULE, > }; > > +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, Perhaps a slightly more idiomatic way to write this would be: static const struct rockchip_pwm_data pwm_data_v1 = { .regs = { .duty = PWM_HRC, .period = PWM_LRC, .cntr = PWM_CNTR, .ctrl = PWM_CTRL, }, ... }; And similar for the v2 and vop structures. And like I said in another reply, since the defines are now only used in this structure it's a little redundant to give them symbolic names, so the above could equally well be: static const struct rockchip_pwm_data pwm_data_v1 = { .regs = { .duty = 0x04, .period = 0x08, .cntr = 0x00, .ctrl = 0x0c, }, ... }; > + .prescaler = PRESCALER, Similarly for the prescaler value, it can now simply be 2 here. > + .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, And 1 here. > + .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, > + .prescaler = PRESCALER-1, And 1 here. > + .set_enable = rockchip_pwm_set_enable_v2, > +}; No need for the double indirection. Thierry
Thierry, 在 2014年08月07日 14:18, Thierry Reding 写道: > On Thu, Jul 24, 2014 at 06:21:35PM +0800, Caesar Wang wrote: >> This patch added to support the PWM controller found on >> RK3288 SoC. >> >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com> >> --- >> drivers/pwm/pwm-rockchip.c | 124 ++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 105 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c >> index eec2145..59c2513 100644 >> --- a/drivers/pwm/pwm-rockchip.c >> +++ b/drivers/pwm/pwm-rockchip.c >> @@ -2,6 +2,7 @@ >> * PWM driver for Rockchip SoCs >> * >> * Copyright (C) 2014 Beniamino Galvani <b.galvani@gmail.com> >> + * Copyright (C) 2014 ROCKCHIP, Inc. >> * >> * This program is free software; you can redistribute it and/or >> * modify it under the terms of the GNU General Public License >> @@ -12,6 +13,7 @@ >> #include <linux/io.h> >> #include <linux/module.h> >> #include <linux/of.h> >> +#include <linux/of_device.h> >> #include <linux/platform_device.h> >> #include <linux/pwm.h> >> #include <linux/time.h> >> @@ -25,17 +27,72 @@ >> >> #define PRESCALER 2 >> >> +#define PWM_ENABLE (1 << 0) >> +#define PWM_CONTINUOUS (1 << 1) >> +#define PWM_DUTY_POSITIVE (1 << 3) >> +#define PWM_INACTIVE_NEGATIVE (0 << 4) >> +#define PWM_OUTPUT_LEFT (0 << 5) >> +#define PWM_LP_DISABLE (0 << 8) >> + >> struct rockchip_pwm_chip { >> struct pwm_chip chip; >> struct clk *clk; >> + const struct rockchip_pwm_data *data; >> void __iomem *base; >> }; >> >> +struct rockchip_pwm_regs { >> + unsigned long duty; >> + unsigned long period; >> + unsigned long cntr; >> + unsigned long ctrl; >> +}; >> + >> +struct rockchip_pwm_data { >> + struct rockchip_pwm_regs regs; >> + unsigned int prescaler; >> + >> + void (*set_enable)(struct pwm_chip *chip, bool enable); >> +}; >> + >> static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c) >> { >> return container_of(c, struct rockchip_pwm_chip, chip); >> } >> >> +static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable) >> +{ >> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); >> + u32 val = 0; >> + u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN; >> + >> + val = readl_relaxed(pc->base + pc->data->regs.ctrl); >> + >> + if (enable) >> + val |= enable_conf; >> + else >> + val &= ~enable_conf; >> + >> + writel_relaxed(val, pc->base + pc->data->regs.ctrl); >> +} >> + >> +static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable) >> +{ >> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); >> + u32 val = 0; >> + u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE | >> + PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE; >> + >> + val = readl_relaxed(pc->base + pc->data->regs.ctrl); >> + >> + if (enable) >> + val |= enable_conf; >> + else >> + val &= ~enable_conf; >> + >> + writel_relaxed(val, pc->base + pc->data->regs.ctrl); >> +} >> + >> static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, >> int duty_ns, int period_ns) >> { >> @@ -52,20 +109,20 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, >> * default prescaler value for all practical clock rate values. >> */ >> div = clk_rate * period_ns; >> - do_div(div, PRESCALER * NSEC_PER_SEC); >> + do_div(div, pc->data->prescaler * NSEC_PER_SEC); >> period = div; >> >> div = clk_rate * duty_ns; >> - do_div(div, PRESCALER * NSEC_PER_SEC); >> + do_div(div, pc->data->prescaler * NSEC_PER_SEC); >> duty = div; >> >> ret = clk_enable(pc->clk); >> if (ret) >> return ret; >> >> - writel(period, pc->base + PWM_LRC); >> - writel(duty, pc->base + PWM_HRC); >> - writel(0, pc->base + PWM_CNTR); >> + writel(period, pc->base + pc->data->regs.period); >> + writel(duty, pc->base + pc->data->regs.duty); >> + writel(0, pc->base + pc->data->regs.cntr); >> >> clk_disable(pc->clk); >> >> @@ -76,15 +133,12 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) >> { >> struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); >> int ret; >> - u32 val; >> >> ret = clk_enable(pc->clk); >> if (ret) >> return ret; >> >> - val = readl_relaxed(pc->base + PWM_CTRL); >> - val |= PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN; >> - writel_relaxed(val, pc->base + PWM_CTRL); >> + pc->data->set_enable(chip, true); >> >> return 0; >> } >> @@ -92,11 +146,8 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) >> static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) >> { >> struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); >> - u32 val; >> >> - val = readl_relaxed(pc->base + PWM_CTRL); >> - val &= ~(PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN); >> - writel_relaxed(val, pc->base + PWM_CTRL); >> + pc->data->set_enable(chip, false); >> >> clk_disable(pc->clk); >> } >> @@ -108,12 +159,52 @@ static const struct pwm_ops rockchip_pwm_ops = { >> .owner = THIS_MODULE, >> }; >> >> +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, > Perhaps a slightly more idiomatic way to write this would be: > > static const struct rockchip_pwm_data pwm_data_v1 = { > .regs = { > .duty = PWM_HRC, > .period = PWM_LRC, > .cntr = PWM_CNTR, > .ctrl = PWM_CTRL, > }, > ... > }; > > And similar for the v2 and vop structures. And like I said in another > reply, since the defines are now only used in this structure it's a > little redundant to give them symbolic names, so the above could equally > well be: > > static const struct rockchip_pwm_data pwm_data_v1 = { > .regs = { > .duty = 0x04, > .period = 0x08, > .cntr = 0x00, > .ctrl = 0x0c, > }, > ... > }; > >> + .prescaler = PRESCALER, > Similarly for the prescaler value, it can now simply be 2 here. > >> + .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, > And 1 here. > >> + .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, >> + .prescaler = PRESCALER-1, > And 1 here. As you say, I will rewrite the about if it's really need do so it. For example: static const struct rockchip_pwm_data pwm_data_v1 = { .regs = { .duty = 0x04, .period = 0x08, .cntr = 0x00, .ctrl = 0x0c, }, .prescaler = 2, .set_enable = rockchip_pwm_set_enable_v1, }; static const struct rockchip_pwm_data pwm_data_v2 = { .regs = { .duty = 0x08, .period = 0x04, .cntr = 0x00, .ctrl = 0x0c, }, .prescaler = 1, .set_enable = rockchip_pwm_set_enable_v2, }; static const struct rockchip_pwm_data pwm_data_vop = { .regs = { .duty = 0x08, .period = 0x04, .cntr = 0x0c, .ctrl = 0x00, }, .prescaler = 1, .set_enable = rockchip_pwm_set_enable_v2, }; Is that right? >> + .set_enable = rockchip_pwm_set_enable_v2, >> +}; > No need for the double indirection. Sorry, I think is need if you mean a double indirection for ".set_enable". Caesar > > Thierry -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 07, 2014 at 09:04:30PM +0800, caesar wrote: [...] > As you say, I will rewrite the about if it's really need do so it. > For example: > > static const struct rockchip_pwm_data pwm_data_v1 = { > .regs = { > .duty = 0x04, > .period = 0x08, > .cntr = 0x00, > .ctrl = 0x0c, > }, > .prescaler = 2, > .set_enable = rockchip_pwm_set_enable_v1, > }; > > static const struct rockchip_pwm_data pwm_data_v2 = { > .regs = { > .duty = 0x08, > .period = 0x04, > .cntr = 0x00, > .ctrl = 0x0c, > }, > .prescaler = 1, > .set_enable = rockchip_pwm_set_enable_v2, > }; > > static const struct rockchip_pwm_data pwm_data_vop = { > .regs = { > .duty = 0x08, > .period = 0x04, > .cntr = 0x0c, > .ctrl = 0x00, > }, > .prescaler = 1, > .set_enable = rockchip_pwm_set_enable_v2, > }; > > Is that right? Yes. > >>+ .set_enable = rockchip_pwm_set_enable_v2, > >>+}; > >No need for the double indirection. > > Sorry, I think is need if you mean a double indirection for ".set_enable". The "double indirection" was regarding the symbolic names for registers, not the .set_enable(). Sorry. Thierry
Thierry, 在 2014年08月07日 21:14, Thierry Reding 写道: > On Thu, Aug 07, 2014 at 09:04:30PM +0800, caesar wrote: > [...] >> As you say, I will rewrite the about if it's really need do so it. >> For example: >> >> static const struct rockchip_pwm_data pwm_data_v1 = { >> .regs = { >> .duty = 0x04, >> .period = 0x08, >> .cntr = 0x00, >> .ctrl = 0x0c, >> }, >> .prescaler = 2, >> .set_enable = rockchip_pwm_set_enable_v1, >> }; >> >> static const struct rockchip_pwm_data pwm_data_v2 = { >> .regs = { >> .duty = 0x08, >> .period = 0x04, >> .cntr = 0x00, >> .ctrl = 0x0c, >> }, >> .prescaler = 1, >> .set_enable = rockchip_pwm_set_enable_v2, >> }; >> >> static const struct rockchip_pwm_data pwm_data_vop = { >> .regs = { >> .duty = 0x08, >> .period = 0x04, >> .cntr = 0x0c, >> .ctrl = 0x00, >> }, >> .prescaler = 1, >> .set_enable = rockchip_pwm_set_enable_v2, >> }; >> >> Is that right? > Yes. > >>>> + .set_enable = rockchip_pwm_set_enable_v2, >>>> +}; >>> No need for the double indirection. >> Sorry, I think is need if you mean a double indirection for ".set_enable". > The "double indirection" was regarding the symbolic names for registers, > not the .set_enable(). Sorry. OK,I will fix the about in patch v5 tomorrow if no other problems,Thanks! > Thierry -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index eec2145..59c2513 100644 --- a/drivers/pwm/pwm-rockchip.c +++ b/drivers/pwm/pwm-rockchip.c @@ -2,6 +2,7 @@ * PWM driver for Rockchip SoCs * * Copyright (C) 2014 Beniamino Galvani <b.galvani@gmail.com> + * Copyright (C) 2014 ROCKCHIP, Inc. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -12,6 +13,7 @@ #include <linux/io.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pwm.h> #include <linux/time.h> @@ -25,17 +27,72 @@ #define PRESCALER 2 +#define PWM_ENABLE (1 << 0) +#define PWM_CONTINUOUS (1 << 1) +#define PWM_DUTY_POSITIVE (1 << 3) +#define PWM_INACTIVE_NEGATIVE (0 << 4) +#define PWM_OUTPUT_LEFT (0 << 5) +#define PWM_LP_DISABLE (0 << 8) + struct rockchip_pwm_chip { struct pwm_chip chip; struct clk *clk; + const struct rockchip_pwm_data *data; void __iomem *base; }; +struct rockchip_pwm_regs { + unsigned long duty; + unsigned long period; + unsigned long cntr; + unsigned long ctrl; +}; + +struct rockchip_pwm_data { + struct rockchip_pwm_regs regs; + unsigned int prescaler; + + void (*set_enable)(struct pwm_chip *chip, bool enable); +}; + static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c) { return container_of(c, struct rockchip_pwm_chip, chip); } +static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable) +{ + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); + u32 val = 0; + u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN; + + val = readl_relaxed(pc->base + pc->data->regs.ctrl); + + if (enable) + val |= enable_conf; + else + val &= ~enable_conf; + + writel_relaxed(val, pc->base + pc->data->regs.ctrl); +} + +static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable) +{ + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); + u32 val = 0; + u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE | + PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE; + + val = readl_relaxed(pc->base + pc->data->regs.ctrl); + + if (enable) + val |= enable_conf; + else + val &= ~enable_conf; + + writel_relaxed(val, pc->base + pc->data->regs.ctrl); +} + static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns, int period_ns) { @@ -52,20 +109,20 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, * default prescaler value for all practical clock rate values. */ div = clk_rate * period_ns; - do_div(div, PRESCALER * NSEC_PER_SEC); + do_div(div, pc->data->prescaler * NSEC_PER_SEC); period = div; div = clk_rate * duty_ns; - do_div(div, PRESCALER * NSEC_PER_SEC); + do_div(div, pc->data->prescaler * NSEC_PER_SEC); duty = div; ret = clk_enable(pc->clk); if (ret) return ret; - writel(period, pc->base + PWM_LRC); - writel(duty, pc->base + PWM_HRC); - writel(0, pc->base + PWM_CNTR); + writel(period, pc->base + pc->data->regs.period); + writel(duty, pc->base + pc->data->regs.duty); + writel(0, pc->base + pc->data->regs.cntr); clk_disable(pc->clk); @@ -76,15 +133,12 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) { struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); int ret; - u32 val; ret = clk_enable(pc->clk); if (ret) return ret; - val = readl_relaxed(pc->base + PWM_CTRL); - val |= PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN; - writel_relaxed(val, pc->base + PWM_CTRL); + pc->data->set_enable(chip, true); return 0; } @@ -92,11 +146,8 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) { struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); - u32 val; - val = readl_relaxed(pc->base + PWM_CTRL); - val &= ~(PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN); - writel_relaxed(val, pc->base + PWM_CTRL); + pc->data->set_enable(chip, false); clk_disable(pc->clk); } @@ -108,12 +159,52 @@ static const struct pwm_ops rockchip_pwm_ops = { .owner = THIS_MODULE, }; +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, + .prescaler = PRESCALER-1, + .set_enable = rockchip_pwm_set_enable_v2, +}; + +static const struct of_device_id rockchip_pwm_dt_ids[] = { + { .compatible = "rockchip,rk2928-pwm", .data = &pwm_data_v1}, + { .compatible = "rockchip,rk3288-pwm", .data = &pwm_data_v2}, + { .compatible = "rockchip,vop-pwm", .data = &pwm_data_vop}, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids); + static int rockchip_pwm_probe(struct platform_device *pdev) { + const struct of_device_id *id; struct rockchip_pwm_chip *pc; struct resource *r; int ret; + id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev); + if (!id) + return -EINVAL; + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); if (!pc) return -ENOMEM; @@ -133,6 +224,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) platform_set_drvdata(pdev, pc); + pc->data = id->data; pc->chip.dev = &pdev->dev; pc->chip.ops = &rockchip_pwm_ops; pc->chip.base = -1; @@ -156,12 +248,6 @@ static int rockchip_pwm_remove(struct platform_device *pdev) return pwmchip_remove(&pc->chip); } -static const struct of_device_id rockchip_pwm_dt_ids[] = { - { .compatible = "rockchip,rk2928-pwm" }, - { /* sentinel */ } -}; -MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids); - static struct platform_driver rockchip_pwm_driver = { .driver = { .name = "rockchip-pwm",
This patch added to support the PWM controller found on RK3288 SoC. Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com> --- drivers/pwm/pwm-rockchip.c | 124 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 105 insertions(+), 19 deletions(-)