| Message ID | 1535461286-12308-2-git-send-email-claudiu.beznea@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | extend PWM framework to support PWM modes | expand |
On Tue, Aug 28, 2018 at 04:01:18PM +0300, Claudiu Beznea wrote: > Add basic PWM modes: normal and complementary. These modes should > differentiate the single output PWM channels from two outputs PWM > channels. These modes could be set as follow: > 1. PWM channels with one output per channel: > - normal mode > 2. PWM channels with two outputs per channel: > - normal mode > - complementary mode > Since users could use a PWM channel with two output as one output PWM > channel, the PWM normal mode is allowed to be set for PWM channels with > two outputs; in fact PWM normal mode should be supported by all PWMs. > > The PWM capabilities were implemented per PWM channel. Every PWM controller > will register a function to get PWM capabilities. If this is not explicitly > set by the driver a default function will be used to retrieve the PWM > capabilities (in this case the PWM capabilities will contain only PWM > normal mode). This function is set in pwmchip_add_with_polarity() as a > member of "struct pwm_chip". To retrieve capabilities the pwm_get_caps() > function could be used. > > Every PWM channel have associated a mode in the PWM state. Proper > support was added to get/set PWM mode. The mode could also be set > from DT via flag cells. The valid DT modes are located in > include/dt-bindings/pwm/pwm.h. Only modes supported by PWM channel could be > set. If nothing is specified for a PWM channel, via DT, the first available > mode will be used (normally, this will be PWM normal mode). > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > drivers/pwm/core.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > drivers/pwm/sysfs.c | 61 ++++++++++++++++++++++++++ > include/linux/pwm.h | 39 +++++++++++++++++ > 3 files changed, 221 insertions(+), 3 deletions(-) > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 1581f6ab1b1f..59a9df9120de 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -136,6 +136,7 @@ struct pwm_device * > of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) > { > struct pwm_device *pwm; > + int modebit; > > /* check, whether the driver supports a third cell for flags */ > if (pc->of_pwm_n_cells < 3) > @@ -154,9 +155,23 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) > > pwm->args.period = args->args[1]; > pwm->args.polarity = PWM_POLARITY_NORMAL; > + pwm->args.mode = pwm_mode_get_valid(pc, pwm); > > - if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) > - pwm->args.polarity = PWM_POLARITY_INVERSED; > + if (args->args_count > 2) { > + if (args->args[2] & PWM_POLARITY_INVERTED) > + pwm->args.polarity = PWM_POLARITY_INVERSED; > + > + for (modebit = PWMC_MODE_COMPLEMENTARY_BIT; > + modebit < PWMC_MODE_CNT; modebit++) { > + unsigned long mode = BIT(modebit); > + > + if ((args->args[2] & mode) && > + pwm_mode_valid(pwm, mode)) { > + pwm->args.mode = mode; > + break; > + } > + } > + } > > return pwm; > } > @@ -183,6 +198,7 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) > return pwm; > > pwm->args.period = args->args[1]; > + pwm->args.mode = pwm_mode_get_valid(pc, pwm); > > return pwm; > } > @@ -250,6 +266,97 @@ static bool pwm_ops_check(const struct pwm_ops *ops) > } > > /** > + * pwm_get_caps() - get PWM capabilities of a PWM device > + * @chip: PWM chip > + * @pwm: PWM device to get the capabilities for > + * @caps: returned capabilities > + * > + * Returns: 0 on success or a negative error code on failure > + */ > +int pwm_get_caps(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_caps *caps) > +{ > + if (!chip || !pwm || !caps) > + return -EINVAL; > + > + if (chip->ops && chip->ops->get_caps) > + pwm->chip->ops->get_caps(chip, pwm, caps); > + else if (chip->get_default_caps) > + chip->get_default_caps(caps); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(pwm_get_caps); I'm confused by the way ->get_default_caps() is used here. This always points at pwmchip_get_default_caps() if I understand correctly, so why bother with the indirection? I think it would be better to either export pwmchip_get_default_caps() as a default implementation of ->get_caps(), something like: void pwm_get_default_caps(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_caps *caps) { ... } EXPORT_SYMBOL_GPL(pwm_get_default_caps); This could be used by the individual drivers as the implementation if they don't support any modes. Another, probably simpler approach would be to just get rid of the chip->get_default_caps() pointer and directly call pwmchip_get_default_caps() from pwm_get_caps(): int pwm_get_caps(...) { /* * Note that you don't need the check for chip->ops because * the core checks for that upon registration of the chip. */ if (chip->ops->get_caps) return chip->ops->get_caps(...); return pwm_get_default_caps(...); } And if we do that, might as well fold pwm_get_default_caps() into pwm_get_caps(). > +/** > + * pwm_mode_get_valid() - get the first available valid mode for PWM > + * @chip: PWM chip > + * @pwm: PWM device to get the valid mode for > + * > + * Returns: first valid mode for PWM device > + */ > +unsigned long pwm_mode_get_valid(struct pwm_chip *chip, struct pwm_device *pwm) This is a little ambiguous. Perhaps pwm_get_default_mode()? > +/** > + * pwm_mode_valid() - check if mode is valid for PWM device > + * @pwm: PWM device > + * @mode: PWM mode to check if valid > + * > + * Returns: true if mode is valid and false otherwise > + */ > +bool pwm_mode_valid(struct pwm_device *pwm, unsigned long mode) Again, this is slightly ambiguous because the name suggests that it merely tests a mode for validity, not that it is really supported by the given device. Perhaps something like pwm_supports_mode()? > +const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode) > +{ > + static const char * const modes[] = { > + "invalid", > + "normal", > + "complementary", > + }; > + > + if (!pwm_mode_valid(pwm, mode)) > + return modes[0]; > + > + return modes[ffs(mode)]; > +} Do we really need to be able to get the name of the mode in the context of a given PWM channel? Couldn't we drop the pwm parameter and simply return the name (pwm_get_mode_name()?) and at the same time remove the extra "invalid" mode in there? I'm not sure what the use-case here is, but it seems to me like the code should always check for supported modes first before reporting their names in any way. > +/** > * pwmchip_add_with_polarity() - register a new PWM chip > * @chip: the PWM chip to add > * @polarity: initial polarity of PWM channels > @@ -275,6 +382,8 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, > > mutex_lock(&pwm_lock); > > + chip->get_default_caps = pwmchip_get_default_caps; > + > ret = alloc_pwms(chip->base, chip->npwm); > if (ret < 0) > goto out; > @@ -294,6 +403,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, > pwm->pwm = chip->base + i; > pwm->hwpwm = i; > pwm->state.polarity = polarity; > + pwm->state.mode = pwm_mode_get_valid(chip, pwm); > > if (chip->ops->get_state) > chip->ops->get_state(chip, pwm, &pwm->state); > @@ -469,7 +579,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > int err; > > if (!pwm || !state || !state->period || > - state->duty_cycle > state->period) > + state->duty_cycle > state->period || > + !pwm_mode_valid(pwm, state->mode)) > return -EINVAL; > > if (!memcmp(state, &pwm->state, sizeof(*state))) > @@ -530,6 +641,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > > pwm->state.enabled = state->enabled; > } > + > + /* No mode support for non-atomic PWM. */ > + pwm->state.mode = state->mode; That comment seems misplaced. This is actually part of atomic PWM, so maybe just reverse the logic and say "mode support only for atomic PWM" or something. I would personally just leave it away. The legacy API has no way of setting the mode, which is indication enough that we don't support it. > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > index 56518adc31dd..a4ce4ad7edf0 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -26,9 +26,32 @@ enum pwm_polarity { > }; > > /** > + * PWM modes capabilities > + * @PWMC_MODE_NORMAL_BIT: PWM has one output > + * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities > + * @PWMC_MODE_CNT: PWM modes count > + */ > +enum { > + PWMC_MODE_NORMAL_BIT, > + PWMC_MODE_COMPLEMENTARY_BIT, > + PWMC_MODE_CNT, > +}; > + > +#define PWMC_MODE(name) BIT(PWMC_MODE_##name##_BIT) Why the C in the prefix? Why not just PWM_MODE_* for all of the above? > + > +/** > + * struct pwm_caps - PWM capabilities > + * @modes: PWM modes This should probably say that it's a bitmask of PWM_MODE_*. > +struct pwm_caps { > + unsigned long modes; > +}; > + > +/** > * struct pwm_args - board-dependent PWM arguments > * @period: reference period > * @polarity: reference polarity > + * @mode: reference mode As discussed in my reply to the cover letter, what is the use for the reference mode? Drivers want to use either normal or some other mode. What good is it to get this from board-dependent arguments if the driver already knows which one it wants or needs? > @@ -300,6 +331,7 @@ struct pwm_chip { > > struct pwm_device * (*of_xlate)(struct pwm_chip *pc, > const struct of_phandle_args *args); > + void (*get_default_caps)(struct pwm_caps *caps); As mentioned above, I don't think we need this. Thierry
On 16.10.2018 15:25, Thierry Reding wrote: > On Tue, Aug 28, 2018 at 04:01:18PM +0300, Claudiu Beznea wrote: >> Add basic PWM modes: normal and complementary. These modes should >> differentiate the single output PWM channels from two outputs PWM >> channels. These modes could be set as follow: >> 1. PWM channels with one output per channel: >> - normal mode >> 2. PWM channels with two outputs per channel: >> - normal mode >> - complementary mode >> Since users could use a PWM channel with two output as one output PWM >> channel, the PWM normal mode is allowed to be set for PWM channels with >> two outputs; in fact PWM normal mode should be supported by all PWMs. >> >> The PWM capabilities were implemented per PWM channel. Every PWM controller >> will register a function to get PWM capabilities. If this is not explicitly >> set by the driver a default function will be used to retrieve the PWM >> capabilities (in this case the PWM capabilities will contain only PWM >> normal mode). This function is set in pwmchip_add_with_polarity() as a >> member of "struct pwm_chip". To retrieve capabilities the pwm_get_caps() >> function could be used. >> >> Every PWM channel have associated a mode in the PWM state. Proper >> support was added to get/set PWM mode. The mode could also be set >> from DT via flag cells. The valid DT modes are located in >> include/dt-bindings/pwm/pwm.h. Only modes supported by PWM channel could be >> set. If nothing is specified for a PWM channel, via DT, the first available >> mode will be used (normally, this will be PWM normal mode). >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> drivers/pwm/core.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++-- >> drivers/pwm/sysfs.c | 61 ++++++++++++++++++++++++++ >> include/linux/pwm.h | 39 +++++++++++++++++ >> 3 files changed, 221 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c >> index 1581f6ab1b1f..59a9df9120de 100644 >> --- a/drivers/pwm/core.c >> +++ b/drivers/pwm/core.c >> @@ -136,6 +136,7 @@ struct pwm_device * >> of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) >> { >> struct pwm_device *pwm; >> + int modebit; >> >> /* check, whether the driver supports a third cell for flags */ >> if (pc->of_pwm_n_cells < 3) >> @@ -154,9 +155,23 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) >> >> pwm->args.period = args->args[1]; >> pwm->args.polarity = PWM_POLARITY_NORMAL; >> + pwm->args.mode = pwm_mode_get_valid(pc, pwm); >> >> - if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) >> - pwm->args.polarity = PWM_POLARITY_INVERSED; >> + if (args->args_count > 2) { >> + if (args->args[2] & PWM_POLARITY_INVERTED) >> + pwm->args.polarity = PWM_POLARITY_INVERSED; >> + >> + for (modebit = PWMC_MODE_COMPLEMENTARY_BIT; >> + modebit < PWMC_MODE_CNT; modebit++) { >> + unsigned long mode = BIT(modebit); >> + >> + if ((args->args[2] & mode) && >> + pwm_mode_valid(pwm, mode)) { >> + pwm->args.mode = mode; >> + break; >> + } >> + } >> + } >> >> return pwm; >> } >> @@ -183,6 +198,7 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) >> return pwm; >> >> pwm->args.period = args->args[1]; >> + pwm->args.mode = pwm_mode_get_valid(pc, pwm); >> >> return pwm; >> } >> @@ -250,6 +266,97 @@ static bool pwm_ops_check(const struct pwm_ops *ops) >> } >> >> /** >> + * pwm_get_caps() - get PWM capabilities of a PWM device >> + * @chip: PWM chip >> + * @pwm: PWM device to get the capabilities for >> + * @caps: returned capabilities >> + * >> + * Returns: 0 on success or a negative error code on failure >> + */ >> +int pwm_get_caps(struct pwm_chip *chip, struct pwm_device *pwm, >> + struct pwm_caps *caps) >> +{ >> + if (!chip || !pwm || !caps) >> + return -EINVAL; >> + >> + if (chip->ops && chip->ops->get_caps) >> + pwm->chip->ops->get_caps(chip, pwm, caps); >> + else if (chip->get_default_caps) >> + chip->get_default_caps(caps); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(pwm_get_caps); > > I'm confused by the way ->get_default_caps() is used here. This always > points at pwmchip_get_default_caps() if I understand correctly, so why > bother with the indirection? Yep, looking again at this it also looks ugly to me and I dont't remember why I chose this approach. I looked over emails and so but didn't manage to find the reason for this. I agree with you that there is no need to keep a pointer in struct pwm_chip for this. > > I think it would be better to either export pwmchip_get_default_caps() > as a default implementation of ->get_caps(), something like: > > void pwm_get_default_caps(struct pwm_chip *chip, struct pwm_device *pwm, > struct pwm_caps *caps) > { > ... > } > EXPORT_SYMBOL_GPL(pwm_get_default_caps); > > This could be used by the individual drivers as the implementation if > they don't support any modes. > > Another, probably simpler approach would be to just get rid of the > chip->get_default_caps() pointer and directly call > pwmchip_get_default_caps() from pwm_get_caps(): > > int pwm_get_caps(...) > { > /* > * Note that you don't need the check for chip->ops because > * the core checks for that upon registration of the chip. > */ > if (chip->ops->get_caps) > return chip->ops->get_caps(...); > > return pwm_get_default_caps(...); > } > > And if we do that, might as well fold pwm_get_default_caps() into > pwm_get_caps(). I am also in favor of this approach. > >> +/** >> + * pwm_mode_get_valid() - get the first available valid mode for PWM >> + * @chip: PWM chip >> + * @pwm: PWM device to get the valid mode for >> + * >> + * Returns: first valid mode for PWM device >> + */ >> +unsigned long pwm_mode_get_valid(struct pwm_chip *chip, struct pwm_device *pwm) > > This is a little ambiguous. Perhaps pwm_get_default_mode()? Ok, will change it to pwm_get_default_mode(). I placed pwm_mode in front of every function in this patch to emphasize the functions are dealing with PWM modes. But being inside PWM core and using this approach might be confusing. > >> +/** >> + * pwm_mode_valid() - check if mode is valid for PWM device >> + * @pwm: PWM device >> + * @mode: PWM mode to check if valid >> + * >> + * Returns: true if mode is valid and false otherwise >> + */ >> +bool pwm_mode_valid(struct pwm_device *pwm, unsigned long mode) > > Again, this is slightly ambiguous because the name suggests that it > merely tests a mode for validity, not that it is really supported by the > given device. Perhaps something like pwm_supports_mode()? Ok, understand. I'll use pwm_supports_mode() instead. > >> +const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode) >> +{ >> + static const char * const modes[] = { >> + "invalid", >> + "normal", >> + "complementary", >> + }; >> + >> + if (!pwm_mode_valid(pwm, mode)) >> + return modes[0]; >> + >> + return modes[ffs(mode)]; >> +} > > Do we really need to be able to get the name of the mode in the context > of a given PWM channel? Couldn't we drop the pwm parameter and simply > return the name (pwm_get_mode_name()?) and at the same time remove the > extra "invalid" mode in there? I'm not sure what the use-case here is, > but it seems to me like the code should always check for supported modes > first before reporting their names in any way. Looking back at this code, the main use case for checking PWM mode validity in pwm_mode_desc() was only with regards to mode_store(). But there is not need for this checking since the same thing will be checked in pwm_apply_state() and, in case user provides an invalid mode via sysfs the pwm_apply_state() will fail. To conclude, I will change this function in something like: if (mode == PWM_MODE_NORMAL) return "normal"; else if (mode == PWM_MODE_COMPLEMENTARY) return "complementary"; else if (mode == PWM_MODE_PUSH_PULL) return "push-pull"; else return "invalid"; Please let me know if it is OK for you. > >> +/** >> * pwmchip_add_with_polarity() - register a new PWM chip >> * @chip: the PWM chip to add >> * @polarity: initial polarity of PWM channels >> @@ -275,6 +382,8 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, >> >> mutex_lock(&pwm_lock); >> >> + chip->get_default_caps = pwmchip_get_default_caps; >> + >> ret = alloc_pwms(chip->base, chip->npwm); >> if (ret < 0) >> goto out; >> @@ -294,6 +403,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, >> pwm->pwm = chip->base + i; >> pwm->hwpwm = i; >> pwm->state.polarity = polarity; >> + pwm->state.mode = pwm_mode_get_valid(chip, pwm); >> >> if (chip->ops->get_state) >> chip->ops->get_state(chip, pwm, &pwm->state); >> @@ -469,7 +579,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) >> int err; >> >> if (!pwm || !state || !state->period || >> - state->duty_cycle > state->period) >> + state->duty_cycle > state->period || >> + !pwm_mode_valid(pwm, state->mode)) >> return -EINVAL; >> >> if (!memcmp(state, &pwm->state, sizeof(*state))) >> @@ -530,6 +641,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) >> >> pwm->state.enabled = state->enabled; >> } >> + >> + /* No mode support for non-atomic PWM. */ >> + pwm->state.mode = state->mode; > > That comment seems misplaced. This is actually part of atomic PWM, so > maybe just reverse the logic and say "mode support only for atomic PWM" > or something. I would personally just leave it away. Ok, sure. I will remove the comment. But the code has to be there to avoid unassigned mode value for PWM state (normal mode means BIT(0)) and so to avoid future PWM applies failure. The legacy API has > no way of setting the mode, which is indication enough that we don't > support it. > >> diff --git a/include/linux/pwm.h b/include/linux/pwm.h >> index 56518adc31dd..a4ce4ad7edf0 100644 >> --- a/include/linux/pwm.h >> +++ b/include/linux/pwm.h >> @@ -26,9 +26,32 @@ enum pwm_polarity { >> }; >> >> /** >> + * PWM modes capabilities >> + * @PWMC_MODE_NORMAL_BIT: PWM has one output >> + * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities >> + * @PWMC_MODE_CNT: PWM modes count >> + */ >> +enum { >> + PWMC_MODE_NORMAL_BIT, >> + PWMC_MODE_COMPLEMENTARY_BIT, >> + PWMC_MODE_CNT, >> +}; >> + >> +#define PWMC_MODE(name) BIT(PWMC_MODE_##name##_BIT) > > Why the C in the prefix? Why not just PWM_MODE_* for all of the above? Well... PWM_MODE() macro was already took by pwm-sun4i.c driver at the time I wrote this patch... So I choose to add extra C to this macro, "C" meaning "constant" in my mind. I was not sure it was the best solution at that time either. > >> + >> +/** >> + * struct pwm_caps - PWM capabilities >> + * @modes: PWM modes > > This should probably say that it's a bitmask of PWM_MODE_*. Ok, I'll update. > >> +struct pwm_caps { >> + unsigned long modes; >> +}; >> + >> +/** >> * struct pwm_args - board-dependent PWM arguments >> * @period: reference period >> * @polarity: reference polarity >> + * @mode: reference mode > > As discussed in my reply to the cover letter, what is the use for the > reference mode? Drivers want to use either normal or some other mode. > What good is it to get this from board-dependent arguments if the > driver already knows which one it wants or needs? For the moment, no, there is no in-kernel user. Only sysfs. I was thinking I would also adapt, e.g., the pwm-regulator driver to also make use of this features in setups like the one described in [1], page 2126 (Figure 56-13: Half-Bridge Converter Application: No Feedback Regulation). But, for the moment there is no in-kernel user. [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf > >> @@ -300,6 +331,7 @@ struct pwm_chip { >> >> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >> const struct of_phandle_args *args); >> + void (*get_default_caps)(struct pwm_caps *caps); > > As mentioned above, I don't think we need this. Ok, I'll update. > > Thierry > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Wed, Oct 17, 2018 at 12:42:00PM +0000, Claudiu.Beznea@microchip.com wrote: > On 16.10.2018 15:25, Thierry Reding wrote: > > On Tue, Aug 28, 2018 at 04:01:18PM +0300, Claudiu Beznea wrote: [...] > >> +const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode) > >> +{ > >> + static const char * const modes[] = { > >> + "invalid", > >> + "normal", > >> + "complementary", > >> + }; > >> + > >> + if (!pwm_mode_valid(pwm, mode)) > >> + return modes[0]; > >> + > >> + return modes[ffs(mode)]; > >> +} > > > > Do we really need to be able to get the name of the mode in the context > > of a given PWM channel? Couldn't we drop the pwm parameter and simply > > return the name (pwm_get_mode_name()?) and at the same time remove the > > extra "invalid" mode in there? I'm not sure what the use-case here is, > > but it seems to me like the code should always check for supported modes > > first before reporting their names in any way. > > Looking back at this code, the main use case for checking PWM mode validity > in pwm_mode_desc() was only with regards to mode_store(). But there is not > need for this checking since the same thing will be checked in > pwm_apply_state() and, in case user provides an invalid mode via sysfs the > pwm_apply_state() will fail. > > To conclude, I will change this function in something like: > > if (mode == PWM_MODE_NORMAL) > return "normal"; > else if (mode == PWM_MODE_COMPLEMENTARY) > return "complementary"; > else if (mode == PWM_MODE_PUSH_PULL) > return "push-pull"; > else > return "invalid"; > > Please let me know if it is OK for you. Do we even have to check here for validity of the mode in the first place? Shouldn't this already happen at a higher level? I mean we do need to check for valid input in mode_store(), but whatever mode we pass into this could already have been validated, so that this would never return "invalid". For example, you already define an enum for the PWM modes. I think it'd be best if we then used that enum to pass the modes around. That way it becomes easy to check for validity. So taking one step back, I think we can remove some of the ambiguities by making sure we only ever specify one mode. When the mode is explicitly being set, we only ever want one, right? The only point in time where we can store more than one is for the capabilities. So I think being more explicit about that would be useful. That way we remove any uncertainties about what the unsigned long might contain at any point in time. > >> +/** > >> * pwmchip_add_with_polarity() - register a new PWM chip > >> * @chip: the PWM chip to add > >> * @polarity: initial polarity of PWM channels > >> @@ -275,6 +382,8 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, > >> > >> mutex_lock(&pwm_lock); > >> > >> + chip->get_default_caps = pwmchip_get_default_caps; > >> + > >> ret = alloc_pwms(chip->base, chip->npwm); > >> if (ret < 0) > >> goto out; > >> @@ -294,6 +403,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, > >> pwm->pwm = chip->base + i; > >> pwm->hwpwm = i; > >> pwm->state.polarity = polarity; > >> + pwm->state.mode = pwm_mode_get_valid(chip, pwm); > >> > >> if (chip->ops->get_state) > >> chip->ops->get_state(chip, pwm, &pwm->state); > >> @@ -469,7 +579,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > >> int err; > >> > >> if (!pwm || !state || !state->period || > >> - state->duty_cycle > state->period) > >> + state->duty_cycle > state->period || > >> + !pwm_mode_valid(pwm, state->mode)) > >> return -EINVAL; > >> > >> if (!memcmp(state, &pwm->state, sizeof(*state))) > >> @@ -530,6 +641,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) > >> > >> pwm->state.enabled = state->enabled; > >> } > >> + > >> + /* No mode support for non-atomic PWM. */ > >> + pwm->state.mode = state->mode; > > > > That comment seems misplaced. This is actually part of atomic PWM, so > > maybe just reverse the logic and say "mode support only for atomic PWM" > > or something. I would personally just leave it away. > > Ok, sure. I will remove the comment. But the code has to be there to avoid > unassigned mode value for PWM state (normal mode means BIT(0)) and so to > avoid future PWM applies failure. Oh yeah, definitely keep the code around. I was only commenting on the... comment. =) > The legacy API has > > no way of setting the mode, which is indication enough that we don't > > support it. > > > >> diff --git a/include/linux/pwm.h b/include/linux/pwm.h > >> index 56518adc31dd..a4ce4ad7edf0 100644 > >> --- a/include/linux/pwm.h > >> +++ b/include/linux/pwm.h > >> @@ -26,9 +26,32 @@ enum pwm_polarity { > >> }; > >> > >> /** > >> + * PWM modes capabilities > >> + * @PWMC_MODE_NORMAL_BIT: PWM has one output > >> + * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities > >> + * @PWMC_MODE_CNT: PWM modes count > >> + */ > >> +enum { > >> + PWMC_MODE_NORMAL_BIT, > >> + PWMC_MODE_COMPLEMENTARY_BIT, > >> + PWMC_MODE_CNT, > >> +}; > >> + > >> +#define PWMC_MODE(name) BIT(PWMC_MODE_##name##_BIT) > > > > Why the C in the prefix? Why not just PWM_MODE_* for all of the above? > > Well... PWM_MODE() macro was already took by pwm-sun4i.c driver at the time > I wrote this patch... So I choose to add extra C to this macro, "C" meaning > "constant" in my mind. I was not sure it was the best solution at that time > either. We can always rename definitions in drivers if we want to use conflicting names in the core. In this particular case, and given what I said above regarding the PWM mode definitions, I think it'd be better to drop the _BIT suffix from the enum values, so that we get something like this: enum pwm_mode { PWM_MODE_NORMAL, PWM_MODE_COMPLEMENTARY, PWM_MODE_COUNT }; Then in order to create the actual bitmasks we can use a macro that is explicit about creating bitmasks: #define PWM_MODE_MASK(name) BIT(PWM_MODE_##name##) With that, the conflict with pwm-sun4i conveniently goes away. > >> +struct pwm_caps { > >> + unsigned long modes; > >> +}; > >> + > >> +/** > >> * struct pwm_args - board-dependent PWM arguments > >> * @period: reference period > >> * @polarity: reference polarity > >> + * @mode: reference mode > > > > As discussed in my reply to the cover letter, what is the use for the > > reference mode? Drivers want to use either normal or some other mode. > > What good is it to get this from board-dependent arguments if the > > driver already knows which one it wants or needs? > > For the moment, no, there is no in-kernel user. Only sysfs. I was thinking > I would also adapt, e.g., the pwm-regulator driver to also make use of this > features in setups like the one described in [1], page 2126 > (Figure 56-13: Half-Bridge Converter Application: No Feedback Regulation). > But, for the moment there is no in-kernel user. > > [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf Okay, so that means we don't have any use-cases where we even need the mode in DT? If so, I don't think we should add that code yet. We'd be adding an ABI that we don't know how it will be used or if it is even sufficient. Granted, as long as nobody uses it we could probably just silently drop it again, but why risk if it's just dead code anyway. If I understand the half-bridge converter application correctly you would want to model that with pwm-regulator and instead of using a regular PWM (normal mode) you'd use a PWM_MODE_PUSH_PULL instead. Is that really all there is to support this? Does the voltage output of the half-bridge converter vary linearly with the duty-cycle? I suppose one could always combine the push-pull PWM with a voltage table to make it work. I'm not opposed to the idea, just trying to figure out if the pwm-regulator driver would be viable or whether there are other things we need to make these setups work. Thierry
On 18.10.2018 18:32, Thierry Reding wrote: > On Wed, Oct 17, 2018 at 12:42:00PM +0000, Claudiu.Beznea@microchip.com wrote: >> On 16.10.2018 15:25, Thierry Reding wrote: >>> On Tue, Aug 28, 2018 at 04:01:18PM +0300, Claudiu Beznea wrote: > [...] >>>> +const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode) >>>> +{ >>>> + static const char * const modes[] = { >>>> + "invalid", >>>> + "normal", >>>> + "complementary", >>>> + }; >>>> + >>>> + if (!pwm_mode_valid(pwm, mode)) >>>> + return modes[0]; >>>> + >>>> + return modes[ffs(mode)]; >>>> +} >>> >>> Do we really need to be able to get the name of the mode in the context >>> of a given PWM channel? Couldn't we drop the pwm parameter and simply >>> return the name (pwm_get_mode_name()?) and at the same time remove the >>> extra "invalid" mode in there? I'm not sure what the use-case here is, >>> but it seems to me like the code should always check for supported modes >>> first before reporting their names in any way. >> >> Looking back at this code, the main use case for checking PWM mode validity >> in pwm_mode_desc() was only with regards to mode_store(). But there is not >> need for this checking since the same thing will be checked in >> pwm_apply_state() and, in case user provides an invalid mode via sysfs the >> pwm_apply_state() will fail. >> >> To conclude, I will change this function in something like: >> >> if (mode == PWM_MODE_NORMAL) >> return "normal"; >> else if (mode == PWM_MODE_COMPLEMENTARY) >> return "complementary"; >> else if (mode == PWM_MODE_PUSH_PULL) >> return "push-pull"; >> else >> return "invalid"; >> >> Please let me know if it is OK for you. > > Do we even have to check here for validity of the mode in the first > place? Shouldn't this already happen at a higher level? I mean we do > need to check for valid input in mode_store(), but whatever mode we > pass into this could already have been validated, so that this would > never return "invalid". Ok, now I see your point. I agree, there is no need here for "invalid" here neither since in mode_store there is a look through all the defined modes and, anyway, if nothing valid matches with what user provides, yes, the: + if (modebit == PWMC_MODE_CNT) + return -EINVAL; will return anyway. And every place this pwm_mode_desc() is used, the mode has been already checked and it is valid. > > For example, you already define an enum for the PWM modes. I think it'd > be best if we then used that enum to pass the modes around. That way it > becomes easy to check for validity. Ok. > > So taking one step back, I think we can remove some of the ambiguities > by making sure we only ever specify one mode. When the mode is > explicitly being set, we only ever want one, right? Right! > The only point in > time where we can store more than one is for the capabilities. So I > think being more explicit about that would be useful. Ok. > That way we remove > any uncertainties about what the unsigned long might contain at any > point in time. > >>>> +/** >>>> * pwmchip_add_with_polarity() - register a new PWM chip >>>> * @chip: the PWM chip to add >>>> * @polarity: initial polarity of PWM channels >>>> @@ -275,6 +382,8 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, >>>> >>>> mutex_lock(&pwm_lock); >>>> >>>> + chip->get_default_caps = pwmchip_get_default_caps; >>>> + >>>> ret = alloc_pwms(chip->base, chip->npwm); >>>> if (ret < 0) >>>> goto out; >>>> @@ -294,6 +403,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, >>>> pwm->pwm = chip->base + i; >>>> pwm->hwpwm = i; >>>> pwm->state.polarity = polarity; >>>> + pwm->state.mode = pwm_mode_get_valid(chip, pwm); >>>> >>>> if (chip->ops->get_state) >>>> chip->ops->get_state(chip, pwm, &pwm->state); >>>> @@ -469,7 +579,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) >>>> int err; >>>> >>>> if (!pwm || !state || !state->period || >>>> - state->duty_cycle > state->period) >>>> + state->duty_cycle > state->period || >>>> + !pwm_mode_valid(pwm, state->mode)) >>>> return -EINVAL; >>>> >>>> if (!memcmp(state, &pwm->state, sizeof(*state))) >>>> @@ -530,6 +641,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) >>>> >>>> pwm->state.enabled = state->enabled; >>>> } >>>> + >>>> + /* No mode support for non-atomic PWM. */ >>>> + pwm->state.mode = state->mode; >>> >>> That comment seems misplaced. This is actually part of atomic PWM, so >>> maybe just reverse the logic and say "mode support only for atomic PWM" >>> or something. I would personally just leave it away. >> >> Ok, sure. I will remove the comment. But the code has to be there to avoid >> unassigned mode value for PWM state (normal mode means BIT(0)) and so to >> avoid future PWM applies failure. > > Oh yeah, definitely keep the code around. I was only commenting on > the... comment. =) > >> The legacy API has >>> no way of setting the mode, which is indication enough that we don't >>> support it. >>> >>>> diff --git a/include/linux/pwm.h b/include/linux/pwm.h >>>> index 56518adc31dd..a4ce4ad7edf0 100644 >>>> --- a/include/linux/pwm.h >>>> +++ b/include/linux/pwm.h >>>> @@ -26,9 +26,32 @@ enum pwm_polarity { >>>> }; >>>> >>>> /** >>>> + * PWM modes capabilities >>>> + * @PWMC_MODE_NORMAL_BIT: PWM has one output >>>> + * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities >>>> + * @PWMC_MODE_CNT: PWM modes count >>>> + */ >>>> +enum { >>>> + PWMC_MODE_NORMAL_BIT, >>>> + PWMC_MODE_COMPLEMENTARY_BIT, >>>> + PWMC_MODE_CNT, >>>> +}; >>>> + >>>> +#define PWMC_MODE(name) BIT(PWMC_MODE_##name##_BIT) >>> >>> Why the C in the prefix? Why not just PWM_MODE_* for all of the above? >> >> Well... PWM_MODE() macro was already took by pwm-sun4i.c driver at the time >> I wrote this patch... So I choose to add extra C to this macro, "C" meaning >> "constant" in my mind. I was not sure it was the best solution at that time >> either. > > We can always rename definitions in drivers if we want to use > conflicting names in the core. In this particular case, and given what I > said above regarding the PWM mode definitions, I think it'd be better to > drop the _BIT suffix from the enum values, so that we get something like > this: > > enum pwm_mode { > PWM_MODE_NORMAL, > PWM_MODE_COMPLEMENTARY, > PWM_MODE_COUNT > }; > > Then in order to create the actual bitmasks we can use a macro that is > explicit about creating bitmasks: > > #define PWM_MODE_MASK(name) BIT(PWM_MODE_##name##) > > With that, the conflict with pwm-sun4i conveniently goes away. Ok, thanks! > >>>> +struct pwm_caps { >>>> + unsigned long modes; >>>> +}; >>>> + >>>> +/** >>>> * struct pwm_args - board-dependent PWM arguments >>>> * @period: reference period >>>> * @polarity: reference polarity >>>> + * @mode: reference mode >>> >>> As discussed in my reply to the cover letter, what is the use for the >>> reference mode? Drivers want to use either normal or some other mode. >>> What good is it to get this from board-dependent arguments if the >>> driver already knows which one it wants or needs? >> >> For the moment, no, there is no in-kernel user. Only sysfs. I was thinking >> I would also adapt, e.g., the pwm-regulator driver to also make use of this >> features in setups like the one described in [1], page 2126 >> (Figure 56-13: Half-Bridge Converter Application: No Feedback Regulation). >> But, for the moment there is no in-kernel user. >> >> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf > > Okay, so that means we don't have any use-cases where we even need the > mode in DT? If so, I don't think we should add that code yet. Agree! > We'd be > adding an ABI that we don't know how it will be used or if it is even > sufficient. Granted, as long as nobody uses it we could probably just > silently drop it again, but why risk if it's just dead code anyway. I agree with you. My bad that I added without having a user. I've just had in mind that I will work with it around PWM regulator. > > If I understand the half-bridge converter application correctly you > would want to model that with pwm-regulator and instead of using a > regular PWM (normal mode) you'd use a PWM_MODE_PUSH_PULL instead. Yes, that was the idea. > Is > that really all there is to support this? Than and the the variation at page 2127, same datasheet [1] (figure Figure 56-14: Half-Bridge Converter Application: Feedback Regulation). Also, complementary could also be used in motor control applications. [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf > Does the voltage output of > the half-bridge converter vary linearly with the duty-cycle? That's my understanding. > I suppose > one could always combine the push-pull PWM with a voltage table to make > it work. I'm not opposed to the idea, just trying to figure out if the > pwm-regulator driver would be viable or whether there are other things > we need to make these setups work. Till now I didn't do any changes on PWM regulator to check how it the current behavior with push-pull mode. Thank you, Claudiu Beznea > > Thierry >
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 1581f6ab1b1f..59a9df9120de 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -136,6 +136,7 @@ struct pwm_device * of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) { struct pwm_device *pwm; + int modebit; /* check, whether the driver supports a third cell for flags */ if (pc->of_pwm_n_cells < 3) @@ -154,9 +155,23 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) pwm->args.period = args->args[1]; pwm->args.polarity = PWM_POLARITY_NORMAL; + pwm->args.mode = pwm_mode_get_valid(pc, pwm); - if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) - pwm->args.polarity = PWM_POLARITY_INVERSED; + if (args->args_count > 2) { + if (args->args[2] & PWM_POLARITY_INVERTED) + pwm->args.polarity = PWM_POLARITY_INVERSED; + + for (modebit = PWMC_MODE_COMPLEMENTARY_BIT; + modebit < PWMC_MODE_CNT; modebit++) { + unsigned long mode = BIT(modebit); + + if ((args->args[2] & mode) && + pwm_mode_valid(pwm, mode)) { + pwm->args.mode = mode; + break; + } + } + } return pwm; } @@ -183,6 +198,7 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) return pwm; pwm->args.period = args->args[1]; + pwm->args.mode = pwm_mode_get_valid(pc, pwm); return pwm; } @@ -250,6 +266,97 @@ static bool pwm_ops_check(const struct pwm_ops *ops) } /** + * pwm_get_caps() - get PWM capabilities of a PWM device + * @chip: PWM chip + * @pwm: PWM device to get the capabilities for + * @caps: returned capabilities + * + * Returns: 0 on success or a negative error code on failure + */ +int pwm_get_caps(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_caps *caps) +{ + if (!chip || !pwm || !caps) + return -EINVAL; + + if (chip->ops && chip->ops->get_caps) + pwm->chip->ops->get_caps(chip, pwm, caps); + else if (chip->get_default_caps) + chip->get_default_caps(caps); + + return 0; +} +EXPORT_SYMBOL_GPL(pwm_get_caps); + +static void pwmchip_get_default_caps(struct pwm_caps *caps) +{ + static const struct pwm_caps default_caps = { + .modes = PWMC_MODE(NORMAL), + }; + + if (!caps) + return; + + *caps = default_caps; +} + +/** + * pwm_mode_get_valid() - get the first available valid mode for PWM + * @chip: PWM chip + * @pwm: PWM device to get the valid mode for + * + * Returns: first valid mode for PWM device + */ +unsigned long pwm_mode_get_valid(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct pwm_caps caps; + + if (pwm_get_caps(chip, pwm, &caps)) + return PWMC_MODE(NORMAL); + + return BIT(ffs(caps.modes) - 1); +} +EXPORT_SYMBOL_GPL(pwm_mode_get_valid); + +/** + * pwm_mode_valid() - check if mode is valid for PWM device + * @pwm: PWM device + * @mode: PWM mode to check if valid + * + * Returns: true if mode is valid and false otherwise + */ +bool pwm_mode_valid(struct pwm_device *pwm, unsigned long mode) +{ + struct pwm_caps caps; + + if (!pwm || !mode) + return false; + + if (hweight_long(mode) != 1 || ffs(mode) - 1 >= PWMC_MODE_CNT) + return false; + + if (pwm_get_caps(pwm->chip, pwm, &caps)) + return false; + + return (caps.modes & mode); +} +EXPORT_SYMBOL_GPL(pwm_mode_valid); + +const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode) +{ + static const char * const modes[] = { + "invalid", + "normal", + "complementary", + }; + + if (!pwm_mode_valid(pwm, mode)) + return modes[0]; + + return modes[ffs(mode)]; +} + +/** * pwmchip_add_with_polarity() - register a new PWM chip * @chip: the PWM chip to add * @polarity: initial polarity of PWM channels @@ -275,6 +382,8 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, mutex_lock(&pwm_lock); + chip->get_default_caps = pwmchip_get_default_caps; + ret = alloc_pwms(chip->base, chip->npwm); if (ret < 0) goto out; @@ -294,6 +403,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip, pwm->pwm = chip->base + i; pwm->hwpwm = i; pwm->state.polarity = polarity; + pwm->state.mode = pwm_mode_get_valid(chip, pwm); if (chip->ops->get_state) chip->ops->get_state(chip, pwm, &pwm->state); @@ -469,7 +579,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) int err; if (!pwm || !state || !state->period || - state->duty_cycle > state->period) + state->duty_cycle > state->period || + !pwm_mode_valid(pwm, state->mode)) return -EINVAL; if (!memcmp(state, &pwm->state, sizeof(*state))) @@ -530,6 +641,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state) pwm->state.enabled = state->enabled; } + + /* No mode support for non-atomic PWM. */ + pwm->state.mode = state->mode; } return 0; @@ -579,6 +693,8 @@ int pwm_adjust_config(struct pwm_device *pwm) pwm_get_args(pwm, &pargs); pwm_get_state(pwm, &state); + state.mode = pargs.mode; + /* * If the current period is zero it means that either the PWM driver * does not support initial state retrieval or the PWM has not yet @@ -850,6 +966,7 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id) pwm->args.period = chosen->period; pwm->args.polarity = chosen->polarity; + pwm->args.mode = pwm_mode_get_valid(chip, pwm); return pwm; } @@ -999,6 +1116,7 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) seq_printf(s, " duty: %u ns", state.duty_cycle); seq_printf(s, " polarity: %s", state.polarity ? "inverse" : "normal"); + seq_printf(s, " mode: %s", pwm_mode_desc(pwm, state.mode)); seq_puts(s, "\n"); } diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index 83f2b0b15712..785eda0b1e67 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -223,11 +223,71 @@ static ssize_t capture_show(struct device *child, return sprintf(buf, "%u %u\n", result.period, result.duty_cycle); } +static ssize_t mode_show(struct device *child, + struct device_attribute *attr, + char *buf) +{ + struct pwm_device *pwm = child_to_pwm_device(child); + struct pwm_state state; + unsigned long mode; + int modebit, len = 0; + + pwm_get_state(pwm, &state); + + for (modebit = PWMC_MODE_NORMAL_BIT; + modebit < PWMC_MODE_CNT; modebit++) { + mode = BIT(modebit); + if (pwm_mode_valid(pwm, mode)) { + if (state.mode == mode) + len += scnprintf(buf + len, + PAGE_SIZE - len, "[%s] ", + pwm_mode_desc(pwm, mode)); + else + len += scnprintf(buf + len, + PAGE_SIZE - len, "%s ", + pwm_mode_desc(pwm, mode)); + } + } + + len += scnprintf(buf + len, PAGE_SIZE - len, "\n"); + return len; +} + +static ssize_t mode_store(struct device *child, + struct device_attribute *attr, + const char *buf, size_t size) +{ + struct pwm_export *export = child_to_pwm_export(child); + struct pwm_device *pwm = export->pwm; + struct pwm_state state; + unsigned long mode; + int modebit, ret; + + for (modebit = PWMC_MODE_NORMAL_BIT; + modebit < PWMC_MODE_CNT; modebit++) { + mode = BIT(modebit); + if (sysfs_streq(buf, pwm_mode_desc(pwm, mode))) + break; + } + + if (modebit == PWMC_MODE_CNT) + return -EINVAL; + + mutex_lock(&export->lock); + pwm_get_state(pwm, &state); + state.mode = mode; + ret = pwm_apply_state(pwm, &state); + mutex_unlock(&export->lock); + + return ret ? : size; +} + static DEVICE_ATTR_RW(period); static DEVICE_ATTR_RW(duty_cycle); static DEVICE_ATTR_RW(enable); static DEVICE_ATTR_RW(polarity); static DEVICE_ATTR_RO(capture); +static DEVICE_ATTR_RW(mode); static struct attribute *pwm_attrs[] = { &dev_attr_period.attr, @@ -235,6 +295,7 @@ static struct attribute *pwm_attrs[] = { &dev_attr_enable.attr, &dev_attr_polarity.attr, &dev_attr_capture.attr, + &dev_attr_mode.attr, NULL }; ATTRIBUTE_GROUPS(pwm); diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 56518adc31dd..a4ce4ad7edf0 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -26,9 +26,32 @@ enum pwm_polarity { }; /** + * PWM modes capabilities + * @PWMC_MODE_NORMAL_BIT: PWM has one output + * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities + * @PWMC_MODE_CNT: PWM modes count + */ +enum { + PWMC_MODE_NORMAL_BIT, + PWMC_MODE_COMPLEMENTARY_BIT, + PWMC_MODE_CNT, +}; + +#define PWMC_MODE(name) BIT(PWMC_MODE_##name##_BIT) + +/** + * struct pwm_caps - PWM capabilities + * @modes: PWM modes + */ +struct pwm_caps { + unsigned long modes; +}; + +/** * struct pwm_args - board-dependent PWM arguments * @period: reference period * @polarity: reference polarity + * @mode: reference mode * * This structure describes board-dependent arguments attached to a PWM * device. These arguments are usually retrieved from the PWM lookup table or @@ -41,6 +64,7 @@ enum pwm_polarity { struct pwm_args { unsigned int period; enum pwm_polarity polarity; + unsigned long mode; }; enum { @@ -53,12 +77,14 @@ enum { * @period: PWM period (in nanoseconds) * @duty_cycle: PWM duty cycle (in nanoseconds) * @polarity: PWM polarity + * @mode: PWM mode * @enabled: PWM enabled status */ struct pwm_state { unsigned int period; unsigned int duty_cycle; enum pwm_polarity polarity; + unsigned long mode; bool enabled; }; @@ -181,6 +207,7 @@ static inline void pwm_init_state(const struct pwm_device *pwm, state->period = args.period; state->polarity = args.polarity; state->duty_cycle = 0; + state->mode = args.mode; } /** @@ -254,6 +281,7 @@ pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int duty_cycle, * @get_state: get the current PWM state. This function is only * called once per PWM device when the PWM chip is * registered. + * @get_caps: get PWM capabilities. * @dbg_show: optional routine to show contents in debugfs * @owner: helps prevent removal of modules exporting active PWMs */ @@ -272,6 +300,8 @@ struct pwm_ops { struct pwm_state *state); void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state); + void (*get_caps)(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_caps *caps); #ifdef CONFIG_DEBUG_FS void (*dbg_show)(struct pwm_chip *chip, struct seq_file *s); #endif @@ -287,6 +317,7 @@ struct pwm_ops { * @npwm: number of PWMs controlled by this chip * @pwms: array of PWM devices allocated by the framework * @of_xlate: request a PWM device given a device tree PWM specifier + * @get_default_caps: get default PWM capabilities * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier */ struct pwm_chip { @@ -300,6 +331,7 @@ struct pwm_chip { struct pwm_device * (*of_xlate)(struct pwm_chip *pc, const struct of_phandle_args *args); + void (*get_default_caps)(struct pwm_caps *caps); unsigned int of_pwm_n_cells; }; @@ -438,6 +470,12 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip, unsigned int index, const char *label); +int pwm_get_caps(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_caps *caps); +unsigned long pwm_mode_get_valid(struct pwm_chip *chip, + struct pwm_device *pwm); +bool pwm_mode_valid(struct pwm_device *pwm, unsigned long mode); +const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode); struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args); @@ -592,6 +630,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm) state.enabled = false; state.polarity = pwm->args.polarity; state.period = pwm->args.period; + state.mode = pwm->args.mode; pwm_apply_state(pwm, &state); }
Add basic PWM modes: normal and complementary. These modes should differentiate the single output PWM channels from two outputs PWM channels. These modes could be set as follow: 1. PWM channels with one output per channel: - normal mode 2. PWM channels with two outputs per channel: - normal mode - complementary mode Since users could use a PWM channel with two output as one output PWM channel, the PWM normal mode is allowed to be set for PWM channels with two outputs; in fact PWM normal mode should be supported by all PWMs. The PWM capabilities were implemented per PWM channel. Every PWM controller will register a function to get PWM capabilities. If this is not explicitly set by the driver a default function will be used to retrieve the PWM capabilities (in this case the PWM capabilities will contain only PWM normal mode). This function is set in pwmchip_add_with_polarity() as a member of "struct pwm_chip". To retrieve capabilities the pwm_get_caps() function could be used. Every PWM channel have associated a mode in the PWM state. Proper support was added to get/set PWM mode. The mode could also be set from DT via flag cells. The valid DT modes are located in include/dt-bindings/pwm/pwm.h. Only modes supported by PWM channel could be set. If nothing is specified for a PWM channel, via DT, the first available mode will be used (normally, this will be PWM normal mode). Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/pwm/core.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++-- drivers/pwm/sysfs.c | 61 ++++++++++++++++++++++++++ include/linux/pwm.h | 39 +++++++++++++++++ 3 files changed, 221 insertions(+), 3 deletions(-)