| Message ID | 1515766983-15151-11-git-send-email-claudiu.beznea@microchip.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | extend PWM framework to support PWM modes | expand |
On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote: > Define a macros for PWM modes to be used by device tree sources. > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > include/dt-bindings/pwm/pwm.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h > index ab9a077e3c7d..b8617431f8ec 100644 > --- a/include/dt-bindings/pwm/pwm.h > +++ b/include/dt-bindings/pwm/pwm.h > @@ -12,4 +12,7 @@ > > #define PWM_POLARITY_INVERTED (1 << 0) > > +#define PWM_DTMODE_NORMAL (1 << 0) Bit 0 is already taken. I think you mean (0 << 1)? Personally, I'd just drop this define. A define for a 0 value makes more sense when each state is equally used (like active high or low), but if 0 is the more common case, then I don't the need for a define. > +#define PWM_DTMODE_COMPLEMENTARY (1 << 1) > + > #endif > -- > 2.7.4 > -- 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 20.01.2018 00:34, Rob Herring wrote: > On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote: >> Define a macros for PWM modes to be used by device tree sources. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> include/dt-bindings/pwm/pwm.h | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h >> index ab9a077e3c7d..b8617431f8ec 100644 >> --- a/include/dt-bindings/pwm/pwm.h >> +++ b/include/dt-bindings/pwm/pwm.h >> @@ -12,4 +12,7 @@ >> >> #define PWM_POLARITY_INVERTED (1 << 0) >> >> +#define PWM_DTMODE_NORMAL (1 << 0) > > Bit 0 is already taken. I think you mean (0 << 1)? I wanted to have the PWM modes in a new cell, so that the pwms binding to be something like: pwms=<pwm-controller pwm-channel pwm-period pwm-flags pwm-mode> If you think it is mode feasible to also include PWM mode in the cell for PWM flags, please let me know. > > Personally, I'd just drop this define. A define for a 0 value makes more > sense when each state is equally used (like active high or low), but if > 0 is the more common case, then I don't the need for a define. I want it to have these defines like bit defines: PWM_DTMODE_NORMAL=0x1 PWM_DTMODE_COMPLEMENTARY=0x2 PWM_DTMODE_PUSH_PULL=0x4 Thank you, Claudiu Beznea > >> +#define PWM_DTMODE_COMPLEMENTARY (1 << 1) >> + >> #endif >> -- >> 2.7.4 >> > -- 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 Mon, Jan 22, 2018 at 2:54 AM, Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote: > > > On 20.01.2018 00:34, Rob Herring wrote: >> On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote: >>> Define a macros for PWM modes to be used by device tree sources. >>> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>> --- >>> include/dt-bindings/pwm/pwm.h | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h >>> index ab9a077e3c7d..b8617431f8ec 100644 >>> --- a/include/dt-bindings/pwm/pwm.h >>> +++ b/include/dt-bindings/pwm/pwm.h >>> @@ -12,4 +12,7 @@ >>> >>> #define PWM_POLARITY_INVERTED (1 << 0) >>> >>> +#define PWM_DTMODE_NORMAL (1 << 0) >> >> Bit 0 is already taken. I think you mean (0 << 1)? > I wanted to have the PWM modes in a new cell, so that the pwms binding to be > something like: > pwms=<pwm-controller pwm-channel pwm-period pwm-flags pwm-mode> > > If you think it is mode feasible to also include PWM mode in the cell for > PWM flags, please let me know. Yes, but you have to make "normal" be no bit set to be compatible with everything already out there. >> Personally, I'd just drop this define. A define for a 0 value makes more >> sense when each state is equally used (like active high or low), but if >> 0 is the more common case, then I don't the need for a define. > I want it to have these defines like bit defines: > PWM_DTMODE_NORMAL=0x1 > PWM_DTMODE_COMPLEMENTARY=0x2 > PWM_DTMODE_PUSH_PULL=0x4 Thinking about this some more, shouldn't the new modes just be implied? A client is going to require one of these modes or it won't work right. Also complementary mode could be accomplished with a single pwm output and a board level inverter, right? How would that be handled when the PWM driver doesn't support that mode? Rob -- 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 22.01.2018 20:12, Rob Herring wrote: > On Mon, Jan 22, 2018 at 2:54 AM, Claudiu Beznea > <Claudiu.Beznea@microchip.com> wrote: >> >> >> On 20.01.2018 00:34, Rob Herring wrote: >>> On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote: >>>> Define a macros for PWM modes to be used by device tree sources. >>>> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>>> --- >>>> include/dt-bindings/pwm/pwm.h | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h >>>> index ab9a077e3c7d..b8617431f8ec 100644 >>>> --- a/include/dt-bindings/pwm/pwm.h >>>> +++ b/include/dt-bindings/pwm/pwm.h >>>> @@ -12,4 +12,7 @@ >>>> >>>> #define PWM_POLARITY_INVERTED (1 << 0) >>>> >>>> +#define PWM_DTMODE_NORMAL (1 << 0) >>> >>> Bit 0 is already taken. I think you mean (0 << 1)? >> I wanted to have the PWM modes in a new cell, so that the pwms binding to be >> something like: >> pwms=<pwm-controller pwm-channel pwm-period pwm-flags pwm-mode> >> >> If you think it is mode feasible to also include PWM mode in the cell for >> PWM flags, please let me know. > > Yes, but you have to make "normal" be no bit set to be compatible with > everything already out there. I'm thinking having it separately is more clear and modular. > >>> Personally, I'd just drop this define. A define for a 0 value makes more >>> sense when each state is equally used (like active high or low), but if >>> 0 is the more common case, then I don't the need for a define. >> I want it to have these defines like bit defines: >> PWM_DTMODE_NORMAL=0x1 >> PWM_DTMODE_COMPLEMENTARY=0x2 >> PWM_DTMODE_PUSH_PULL=0x4 > > Thinking about this some more, shouldn't the new modes just be > implied? A client is going to require one of these modes or it won't > work right. The clients could or could not request the mode via DT. Every PWM chip registers supported modes at driver probe (default, if no PWM mode support is added to the PWM chip driver the PWM chip will supports only normal mode). If a PWM channel is requested by a PWM client via DT, and there is no PWM mode setting in DT bindings, e.g. requested with these bindings: pwms=<pwm-controller pwm-channel pwm-period> or pwms=<pwm-controller pwm-channel pwm-period pwm-flags> the first available mode of PWM chip will be used to instantiate the mode. If the defined modes are: PWM_DTMODE_NORMAL=0x1 PWM_DTMODE_COMPLEMENTARY=0x2 PWM_DTMODE_PUSH_PULL=0x4 and the PWM chip driver registers itself, at probe, with (PWM_DTMODE_COMPLEMENTARY | PWM_DTMODE_PUSH_PULL) then the first available mode will be PWM_DTMODE_COMPLEMENTARY (first LSB bit of the variable that keeps the available modes). > > Also complementary mode could be accomplished with a single pwm output > and a board level inverter, right? Yes, I think this could be accomplished. Here I took into account only PWM controller capabilities. Having this, I think will involve having extra PWM bindings describing extra capabilities per-channel. And to change a little bit the implementation in order to have these capabilities per channel nor per PWM controller. What do you think? I think push-pull mode could also be accomplished having board inverter and delay modules. So, in these cases make sense to have per channel capabilities, as per my understanding. Thank you, Claudiu Beznea How would that be handled when the > PWM driver doesn't support that mode? > > Rob > -- 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 Tue, Jan 23, 2018 at 4:40 AM, Claudiu Beznea <Claudiu.Beznea@microchip.com> wrote: > > > On 22.01.2018 20:12, Rob Herring wrote: >> On Mon, Jan 22, 2018 at 2:54 AM, Claudiu Beznea >> <Claudiu.Beznea@microchip.com> wrote: >>> >>> >>> On 20.01.2018 00:34, Rob Herring wrote: >>>> On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote: >>>>> Define a macros for PWM modes to be used by device tree sources. >>>>> >>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>>>> --- >>>>> include/dt-bindings/pwm/pwm.h | 3 +++ >>>>> 1 file changed, 3 insertions(+) >>>>> >>>>> diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h >>>>> index ab9a077e3c7d..b8617431f8ec 100644 >>>>> --- a/include/dt-bindings/pwm/pwm.h >>>>> +++ b/include/dt-bindings/pwm/pwm.h >>>>> @@ -12,4 +12,7 @@ >>>>> >>>>> #define PWM_POLARITY_INVERTED (1 << 0) >>>>> >>>>> +#define PWM_DTMODE_NORMAL (1 << 0) >>>> >>>> Bit 0 is already taken. I think you mean (0 << 1)? >>> I wanted to have the PWM modes in a new cell, so that the pwms binding to be >>> something like: >>> pwms=<pwm-controller pwm-channel pwm-period pwm-flags pwm-mode> >>> >>> If you think it is mode feasible to also include PWM mode in the cell for >>> PWM flags, please let me know. >> >> Yes, but you have to make "normal" be no bit set to be compatible with >> everything already out there. > I'm thinking having it separately is more clear and modular. Having a standard number of cells (and fields in cells) is easier to maintain. We've set this at 3 for PWMs and you have already found what happens when you have a different number of cells. Adding a 4th cell (and possibly a different form of 3 cells in the case of no channel # cell) just creates more combinations to parse. And we don't want to go update all the existing users using 3 cells to use 4 cells just to align. If the mode needed to be set in the common case, then I might feel differently about having a separate cell. But these modes seem like a special case. How many PWM controllers actually support modes like these? >>>> Personally, I'd just drop this define. A define for a 0 value makes more >>>> sense when each state is equally used (like active high or low), but if >>>> 0 is the more common case, then I don't the need for a define. >>> I want it to have these defines like bit defines: >>> PWM_DTMODE_NORMAL=0x1 >>> PWM_DTMODE_COMPLEMENTARY=0x2 >>> PWM_DTMODE_PUSH_PULL=0x4 >> >> Thinking about this some more, shouldn't the new modes just be >> implied? A client is going to require one of these modes or it won't >> work right. > The clients could or could not request the mode via DT. Every PWM chip registers > supported modes at driver probe (default, if no PWM mode support is added > to the PWM chip driver the PWM chip will supports only normal mode). If a > PWM channel is requested by a PWM client via DT, and there is no PWM mode setting > in DT bindings, e.g. requested with these bindings: > pwms=<pwm-controller pwm-channel pwm-period> or > pwms=<pwm-controller pwm-channel pwm-period pwm-flags> > the first available mode of PWM chip will be used to instantiate the mode. > If the defined modes are: > PWM_DTMODE_NORMAL=0x1 > PWM_DTMODE_COMPLEMENTARY=0x2 > PWM_DTMODE_PUSH_PULL=0x4 > and the PWM chip driver registers itself, at probe, with (PWM_DTMODE_COMPLEMENTARY | PWM_DTMODE_PUSH_PULL) > then the first available mode will be PWM_DTMODE_COMPLEMENTARY (first LSB bit > of the variable that keeps the available modes). Every driver already supports "normal", so that's implied. It would be pointless to make every driver register that explicitly. It would be pretty hard to not support normal as that's just ignore the 2nd signal. >> Also complementary mode could be accomplished with a single pwm output >> and a board level inverter, right? > Yes, I think this could be accomplished. Here I took into account only PWM controller > capabilities. Having this, I think will involve having extra PWM bindings describing > extra capabilities per-channel. And to change a little bit the implementation in order > to have these capabilities per channel nor per PWM controller. What do you think? > > I think push-pull mode could also be accomplished having board inverter and delay > modules. So, in these cases make sense to have per channel capabilities, as per my > understanding. Yes, it certainly is per channel. You may or may not have the 2nd pin on any given channel. But again, if the client needs one of these modes, then the h/w must be hooked up correctly to a channel with 2 signals. Rob -- 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 23.01.2018 17:21, Rob Herring wrote: > On Tue, Jan 23, 2018 at 4:40 AM, Claudiu Beznea > <Claudiu.Beznea@microchip.com> wrote: >> >> >> On 22.01.2018 20:12, Rob Herring wrote: >>> On Mon, Jan 22, 2018 at 2:54 AM, Claudiu Beznea >>> <Claudiu.Beznea@microchip.com> wrote: >>>> >>>> >>>> On 20.01.2018 00:34, Rob Herring wrote: >>>>> On Fri, Jan 12, 2018 at 04:22:57PM +0200, Claudiu Beznea wrote: >>>>>> Define a macros for PWM modes to be used by device tree sources. >>>>>> >>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>>>>> --- >>>>>> include/dt-bindings/pwm/pwm.h | 3 +++ >>>>>> 1 file changed, 3 insertions(+) >>>>>> >>>>>> diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h >>>>>> index ab9a077e3c7d..b8617431f8ec 100644 >>>>>> --- a/include/dt-bindings/pwm/pwm.h >>>>>> +++ b/include/dt-bindings/pwm/pwm.h >>>>>> @@ -12,4 +12,7 @@ >>>>>> >>>>>> #define PWM_POLARITY_INVERTED (1 << 0) >>>>>> >>>>>> +#define PWM_DTMODE_NORMAL (1 << 0) >>>>> >>>>> Bit 0 is already taken. I think you mean (0 << 1)? >>>> I wanted to have the PWM modes in a new cell, so that the pwms binding to be >>>> something like: >>>> pwms=<pwm-controller pwm-channel pwm-period pwm-flags pwm-mode> >>>> >>>> If you think it is mode feasible to also include PWM mode in the cell for >>>> PWM flags, please let me know. >>> >>> Yes, but you have to make "normal" be no bit set to be compatible with >>> everything already out there. >> I'm thinking having it separately is more clear and modular. > > Having a standard number of cells (and fields in cells) is easier to > maintain. We've set this at 3 for PWMs and you have already found what > happens when you have a different number of cells. Adding a 4th cell > (and possibly a different form of 3 cells in the case of no channel # > cell) just creates more combinations to parse. Agree with you! And we don't want to go > update all the existing users using 3 cells to use 4 cells just to > align. Is this necessary? I mean, the drivers that will use PWM modes could be updated to use 4 cells or not. For these drivers I created a generic OF parse function which parse everything. This is currently on my local branch only (previous approach to have a common parse function for all driver wasn't well accepted): struct pwm_device * of_pwm_xlate_all(struct pwm_chip *pc, const struct of_phandle_args *args) { struct pwm_device *pwm; enum pwm_mode mode; /* check, whether the driver supports all cells */ if (pc->of_pwm_n_cells < 4) return ERR_PTR(-EINVAL); /* flags in the third and fourth cells are optional */ if (args->args_count < 2) return ERR_PTR(-EINVAL); if (args->args[0] >= pc->npwm) return ERR_PTR(-EINVAL); pwm = pwm_request_from_chip(pc, args->args[0], NULL); if (IS_ERR(pwm)) return pwm; pwm->args.period = args->args[1]; pwm->args.polarity = PWM_POLARITY_NORMAL; pwm->args.mode = BIT(ffs(pwm->chip->caps->modes) - 1); switch (args->args_count) { case 3: mode = BIT(ffs(args->args[3]) - 1); if (pwm_mode_valid(pwm->chip, mode)) pwm->args.mode = mode; case 2: if (args->args[2] & PWM_POLARITY_INVERTED) pwm->args.polarity = PWM_POLARITY_INVERSED; break; default: break; } return pwm; } Drivers which uses PWM mode could use this generic parse function. In case of using old DT binaries, with no PWM mode bindings, and with - a driver that support PWM modes, the pwm->args will be initialized with PWM mode normal (the default for every PWM controller). In case the driver haven't specific support for PWM modes (so it support only PWM normal mode) there is no need to update to 4 cells. The of_pwm_xlate_with_flags() or of_pwm_simple_xlate() could be used to parse DT bindings (with some little changes to also update the PWM mode for pwm->args). Currently I have this line to be added, in these two functions, in next version for this series: pwm->args.mode = BIT(ffs(pwm->chip->caps->modes) - 1); I did some tests on my side and it looks ok. Please correct me if I'm saying something wrong. > > If the mode needed to be set in the common case, then I might feel > differently about having a separate cell. No there is no need to set the mode explicitly. The implicit mode is normal mode. If other mode needs to be set then the mode cells (or the mode inserted in the flag cells as you wish) should take care of this. But these modes seem like a > special case. How many PWM controllers actually support modes like > these?I did a little research on this back in time and seems that is a characteristic of PWM controller. I'm saying about PWM push-pull mode. Also about PWM controllers with 2 outputs per PWM channel. Regarding the other modes I had a discussion with Thierry, back in time, regarding this (see [1]) where he asked me to add these normal and complementary modes before adding push-pull mode in order to differentiate b/w PWM controllers with one output and PWM controllers with 2 outputs per PWM channel so that the PWM user to be aware of how its PWM channel works. [1] https://www.spinics.net/lists/arm-kernel/msg580275.html > >>>>> Personally, I'd just drop this define. A define for a 0 value makes more >>>>> sense when each state is equally used (like active high or low), but if >>>>> 0 is the more common case, then I don't the need for a define. >>>> I want it to have these defines like bit defines: >>>> PWM_DTMODE_NORMAL=0x1 >>>> PWM_DTMODE_COMPLEMENTARY=0x2 >>>> PWM_DTMODE_PUSH_PULL=0x4 >>> >>> Thinking about this some more, shouldn't the new modes just be >>> implied? A client is going to require one of these modes or it won't >>> work right. >> The clients could or could not request the mode via DT. Every PWM chip registers >> supported modes at driver probe (default, if no PWM mode support is added >> to the PWM chip driver the PWM chip will supports only normal mode). If a >> PWM channel is requested by a PWM client via DT, and there is no PWM mode setting >> in DT bindings, e.g. requested with these bindings: >> pwms=<pwm-controller pwm-channel pwm-period> or >> pwms=<pwm-controller pwm-channel pwm-period pwm-flags> >> the first available mode of PWM chip will be used to instantiate the mode. >> If the defined modes are: >> PWM_DTMODE_NORMAL=0x1 >> PWM_DTMODE_COMPLEMENTARY=0x2 >> PWM_DTMODE_PUSH_PULL=0x4 >> and the PWM chip driver registers itself, at probe, with (PWM_DTMODE_COMPLEMENTARY | PWM_DTMODE_PUSH_PULL) >> then the first available mode will be PWM_DTMODE_COMPLEMENTARY (first LSB bit >> of the variable that keeps the available modes). > > Every driver already supports "normal", so that's implied. It would be > pointless to make every driver register that explicitly. Agree! The chip registered with COMPLEMENTARY and PUSH-PULL modes was just an example. It would be > pretty hard to not support normal as that's just ignore the 2nd > signal. Also, agree! > >>> Also complementary mode could be accomplished with a single pwm output >>> and a board level inverter, right? >> Yes, I think this could be accomplished. Here I took into account only PWM controller >> capabilities. Having this, I think will involve having extra PWM bindings describing >> extra capabilities per-channel. And to change a little bit the implementation in order >> to have these capabilities per channel nor per PWM controller. What do you think? >> >> I think push-pull mode could also be accomplished having board inverter and delay >> modules. So, in these cases make sense to have per channel capabilities, as per my >> understanding. > > Yes, it certainly is per channel. You may or may not have the 2nd pin > on any given channel.The changes in this series are for PWM controllers. Having for instance push-pull mode registered for the channel of a PWM controller which has only one output will also involve some control of the external h/w that is doing push-pull. For instance, I'm talking about a schema like this: +---------------+ | | ch0 +-----------+ __ __ | |------>| push-pull |---> _| |________| |____ |PWM controller | | external | __ __ | | | module |---> _______| |_________| |__ | | +-----------+ +---------------+ From my point of view, for this kind of schematic having the PWM channel controller supporting all the modes (normal, complementary, push-pull) will be useless without a way to control the push-pull external module (e.g. a GPIO). Thank you, Claudiu Beznea But again, if the client needs one of these > modes, then the h/w must be hooked up correctly to a channel with 2 > signals. > > Rob > -- 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/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h index ab9a077e3c7d..b8617431f8ec 100644 --- a/include/dt-bindings/pwm/pwm.h +++ b/include/dt-bindings/pwm/pwm.h @@ -12,4 +12,7 @@ #define PWM_POLARITY_INVERTED (1 << 0) +#define PWM_DTMODE_NORMAL (1 << 0) +#define PWM_DTMODE_COMPLEMENTARY (1 << 1) + #endif
Define a macros for PWM modes to be used by device tree sources. Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- include/dt-bindings/pwm/pwm.h | 3 +++ 1 file changed, 3 insertions(+)