| Message ID | 20200706175353.16404-8-michael@walle.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
Hello Michael, On Mon, Jul 06, 2020 at 07:53:47PM +0200, Michael Walle wrote: > diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c > new file mode 100644 > index 000000000000..8ee286b605bf > --- /dev/null > +++ b/drivers/pwm/pwm-sl28cpld.c > @@ -0,0 +1,187 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * sl28cpld PWM driver > + * > + * Copyright 2020 Kontron Europe GmbH > + */ Is there publically available documenation available? If so please add a link here. > + > +#include <linux/bitfield.h> > +#include <linux/kernel.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > + > +/* > + * PWM timer block registers. > + */ > +#define PWM_CTRL 0x00 > +#define PWM_ENABLE BIT(7) > +#define PWM_MODE_250HZ 0 > +#define PWM_MODE_500HZ 1 > +#define PWM_MODE_1KHZ 2 > +#define PWM_MODE_2KHZ 3 > +#define PWM_MODE_MASK GENMASK(1, 0) > +#define PWM_CYCLE 0x01 > +#define PWM_CYCLE_MAX 0x7f Please use a less generic prefix for your defines. Also I like having the defines for field names include register name. Something like: #define PWM_SL28CPLD_CTRL 0x00 #define PWM_SL28CPLD_CTRL_ENABLE BIT(7) #define PWM_SL28CPLD_CTRL_MODE_MASK GENMASK(1, 0) #define PWM_SL28CPLD_CTRL_MODE_250HZ FIELD_PREP(PWM_SL28CPLD_CTRL_MODE_MASK, 0) > +struct sl28cpld_pwm { > + struct pwm_chip pwm_chip; > + struct regmap *regmap; > + u32 offset; > +}; > + > +struct sl28cpld_pwm_periods { > + u8 ctrl; > + unsigned long duty_cycle; > +}; > + > +struct sl28cpld_pwm_config { > + unsigned long period_ns; > + u8 max_duty_cycle; > +}; > + > +static struct sl28cpld_pwm_config sl28cpld_pwm_config[] = { const ? (Or drop as the values can be easily computed, see below.) > + [PWM_MODE_250HZ] = { .period_ns = 4000000, .max_duty_cycle = 0x80 }, > + [PWM_MODE_500HZ] = { .period_ns = 2000000, .max_duty_cycle = 0x40 }, > + [PWM_MODE_1KHZ] = { .period_ns = 1000000, .max_duty_cycle = 0x20 }, > + [PWM_MODE_2KHZ] = { .period_ns = 500000, .max_duty_cycle = 0x10 }, > +}; > + > +static void sl28cpld_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); > + static struct sl28cpld_pwm_config *config; > + unsigned int reg; > + unsigned int mode; > + > + regmap_read(priv->regmap, priv->offset + PWM_CTRL, ®); > + > + state->enabled = reg & PWM_ENABLE; Would it be more consisted to use FIELD_GET here, too? > + > + mode = FIELD_GET(PWM_MODE_MASK, reg); > + config = &sl28cpld_pwm_config[mode]; > + state->period = config->period_ns; I wonder if this could be done more effectively without the above table. Something like: state->period = 4000000 >> mode. (with a #define for 4000000 of course). > + regmap_read(priv->regmap, priv->offset + PWM_CYCLE, ®); > + pwm_set_relative_duty_cycle(state, reg, config->max_duty_cycle); Oh, what a creative idea to use pwm_set_relative_duty_cycle here. Unfortunately it's using the wrong rounding strategy. Please enable PWM_DEBUG which should diagnose these problems (given enough testing). (Hmm, on second thought I'm not sure that rounding is relevant with the numbers of this hardware. Still it's wrong in general and I don't want to have others copy this.) > +} > + > +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); > + struct sl28cpld_pwm_config *config; > + unsigned int cycle; > + int ret; > + int mode; > + u8 ctrl; > + > + /* Get the configuration by comparing the period */ > + for (mode = 0; mode < ARRAY_SIZE(sl28cpld_pwm_config); mode++) { > + config = &sl28cpld_pwm_config[mode]; > + if (state->period == config->period_ns) > + break; > + } > + > + if (mode == ARRAY_SIZE(sl28cpld_pwm_config)) > + return -EINVAL; You're supposed to pick the biggest period that isn't bigger than the requested period. So something like: switch(period) { case 4000000 ... UINT_MAX: mode = 0; break; case 2000000 ... 3999999: mode = 1; break; ... } (or: if period >= 4000000: mode = 0 else: // I think ... please double-check mode = ilog2(4000000 / (period + 1)) + 1 if mode > 3: return -ERANGE; ) real_period = 4000000 >> mode; > + ctrl = FIELD_PREP(PWM_MODE_MASK, mode); > + if (state->enabled) > + ctrl |= PWM_ENABLE; > + > + cycle = pwm_get_relative_duty_cycle(state, config->max_duty_cycle); Again the rounding is wrong. You need need to round down the requested duty_cycle to the next possible value. So something like: duty_cycle = min(real_period, state->duty_cycle); cycle = duty_cycle * (0x80 >> mode) / (4000000 >> mode); which can be further simplified to cycle = duty_cycle / 31250 . > + /* > + * The hardware doesn't allow to set max_duty_cycle if the > + * 250Hz mode is enabled, thus we have to trap that here. > + * But because a 100% duty cycle is equal on all modes, i.e. It depends on how picky you are if you can agree here. Please document this in a Limitations paragraph at the top of the driver similar to drivers/pwm/pwm-rcar.c and others. > + * it is just a "all-high" output, we trap any case with a > + * 100% duty cycle and use the 500Hz mode. Please only trap on 250Hz mode. (Can be done using: if (cycle == 0x80) I think) > + */ > + if (cycle == config->max_duty_cycle) { > + ctrl &= ~PWM_MODE_MASK; > + ctrl |= FIELD_PREP(PWM_MODE_MASK, PWM_MODE_500HZ); > + cycle = PWM_CYCLE_MAX; I would have expected 0x40 here instead of 0x7f? > + } > + > + ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl); > + if (ret) > + return ret; > + > + return regmap_write(priv->regmap, priv->offset + PWM_CYCLE, (u8)cycle); I assume this can result in broken output? Consider the hardware runs with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle = 0x42: Can this result in a period that has mode = 0 & cycle = 0x23? If this cannot be avoided, please document this in the Limitations paragraph. > +} > + > +static const struct pwm_ops sl28cpld_pwm_ops = { > + .apply = sl28cpld_pwm_apply, > + .get_state = sl28cpld_pwm_get_state, > + .owner = THIS_MODULE, > +}; > + > +static int sl28cpld_pwm_probe(struct platform_device *pdev) > +{ > + struct sl28cpld_pwm *priv; > + struct pwm_chip *chip; > + int ret; > + > + if (!pdev->dev.parent) > + return -ENODEV; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->regmap = dev_get_regmap(pdev->dev.parent, NULL); > + if (!priv->regmap) > + return -ENODEV; > + > + ret = device_property_read_u32(&pdev->dev, "reg", &priv->offset); > + if (ret) > + return -EINVAL; > + > + /* Initialize the pwm_chip structure */ > + chip = &priv->pwm_chip; > + chip->dev = &pdev->dev; > + chip->ops = &sl28cpld_pwm_ops; > + chip->base = -1; > + chip->npwm = 1; > + > + ret = pwmchip_add(&priv->pwm_chip); > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, priv); > + > + return 0; > +} Please add error messages with some details for the error paths (preferable using %pe to indicate the error code). > +static int sl28cpld_pwm_remove(struct platform_device *pdev) > +{ > + struct sl28cpld_pwm *priv = platform_get_drvdata(pdev); > + > + return pwmchip_remove(&priv->pwm_chip); > +} > + > +static const struct of_device_id sl28cpld_pwm_of_match[] = { > + { .compatible = "kontron,sl28cpld-pwm" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, sl28cpld_pwm_of_match); > + > +static struct platform_driver sl28cpld_pwm_driver = { > + .probe = sl28cpld_pwm_probe, > + .remove = sl28cpld_pwm_remove, > + .driver = { > + .name = "sl28cpld-pwm", > + .of_match_table = sl28cpld_pwm_of_match, > + }, > +}; > +module_platform_driver(sl28cpld_pwm_driver); > + > +MODULE_DESCRIPTION("sl28cpld PWM Driver"); > +MODULE_AUTHOR("Michael Walle <michael@walle.cc>"); > +MODULE_LICENSE("GPL"); Best regards Uwe
Hi Uwe, first of all, thank you for that thorough review. Am 2020-07-09 10:50, schrieb Uwe Kleine-König: > On Mon, Jul 06, 2020 at 07:53:47PM +0200, Michael Walle wrote: >> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c >> new file mode 100644 >> index 000000000000..8ee286b605bf >> --- /dev/null >> +++ b/drivers/pwm/pwm-sl28cpld.c >> @@ -0,0 +1,187 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * sl28cpld PWM driver >> + * >> + * Copyright 2020 Kontron Europe GmbH >> + */ > > Is there publically available documenation available? If so please add > a > link here. Unfortunately not. But it should be easy enough and I'll describe it briefly in the header. >> + >> +#include <linux/bitfield.h> >> +#include <linux/kernel.h> >> +#include <linux/mod_devicetable.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/pwm.h> >> +#include <linux/regmap.h> >> + >> +/* >> + * PWM timer block registers. >> + */ >> +#define PWM_CTRL 0x00 >> +#define PWM_ENABLE BIT(7) >> +#define PWM_MODE_250HZ 0 >> +#define PWM_MODE_500HZ 1 >> +#define PWM_MODE_1KHZ 2 >> +#define PWM_MODE_2KHZ 3 >> +#define PWM_MODE_MASK GENMASK(1, 0) >> +#define PWM_CYCLE 0x01 >> +#define PWM_CYCLE_MAX 0x7f > > Please use a less generic prefix for your defines. Also I like having > the defines for field names include register name. Something like: > > #define PWM_SL28CPLD_CTRL 0x00 > #define PWM_SL28CPLD_CTRL_ENABLE BIT(7) > #define PWM_SL28CPLD_CTRL_MODE_MASK GENMASK(1, 0) Ok. > #define > PWM_SL28CPLD_CTRL_MODE_250HZ FIELD_PREP(PWM_SL28CPLD_CTRL_MODE_MASK, > 0) Shouldn't we just "#define ..MODE_250HZ 1" use FIELD_PREP inside the code, so you can actually use the normalized enumeration values, too? Actually, I'll rename the PWM_MODE to PWM_PRESCALER, because that is more accurate. >> +struct sl28cpld_pwm { >> + struct pwm_chip pwm_chip; >> + struct regmap *regmap; >> + u32 offset; >> +}; >> + >> +struct sl28cpld_pwm_periods { >> + u8 ctrl; >> + unsigned long duty_cycle; >> +}; >> + >> +struct sl28cpld_pwm_config { >> + unsigned long period_ns; >> + u8 max_duty_cycle; >> +}; >> + >> +static struct sl28cpld_pwm_config sl28cpld_pwm_config[] = { > > const ? (Or drop as the values can be easily computed, see below.) > >> + [PWM_MODE_250HZ] = { .period_ns = 4000000, .max_duty_cycle = 0x80 }, >> + [PWM_MODE_500HZ] = { .period_ns = 2000000, .max_duty_cycle = 0x40 }, >> + [PWM_MODE_1KHZ] = { .period_ns = 1000000, .max_duty_cycle = 0x20 }, >> + [PWM_MODE_2KHZ] = { .period_ns = 500000, .max_duty_cycle = 0x10 }, >> +}; >> + >> +static void sl28cpld_pwm_get_state(struct pwm_chip *chip, >> + struct pwm_device *pwm, >> + struct pwm_state *state) >> +{ >> + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); >> + static struct sl28cpld_pwm_config *config; >> + unsigned int reg; >> + unsigned int mode; >> + >> + regmap_read(priv->regmap, priv->offset + PWM_CTRL, ®); >> + >> + state->enabled = reg & PWM_ENABLE; > > Would it be more consisted to use FIELD_GET here, too? I had used FIELD_GET only for bit-fields with more than one bit, i.e. no flags. But that is just a matter of taste, I guess. I'd prefer to keep the simple "reg & PWM_ENABLE". If you insist on the FIELD_GET() I'll change it ;) >> + >> + mode = FIELD_GET(PWM_MODE_MASK, reg); >> + config = &sl28cpld_pwm_config[mode]; >> + state->period = config->period_ns; > > I wonder if this could be done more effectively without the above > table. > Something like: > > state->period = 4000000 >> mode. The reason I introduced a lookup table here was that I need a list of the supported modes; I wasn't aware of the rounding. See also below. > (with a #define for 4000000 of course). > >> + regmap_read(priv->regmap, priv->offset + PWM_CYCLE, ®); >> + pwm_set_relative_duty_cycle(state, reg, config->max_duty_cycle); > > Oh, what a creative idea to use pwm_set_relative_duty_cycle here. What is that helper for then? The former versions did the same calculations (i.e. DIV_ROUND_CLOSEST_ULL()) just open coded. But I guess then it was also rounding the wrong way. > Unfortunately it's using the wrong rounding strategy. Please enable > PWM_DEBUG which should diagnose these problems (given enough testing). Is there any written documentation on how to round, i.e. up or down? I had a look Documentation/driver-api/pwm.rst again. But couldn't find anything. A grep DIV_ROUND_CLOSEST_ULL() turns out that quite a few drivers use it, so I did the same ;) > (Hmm, on second thought I'm not sure that rounding is relevant with the > numbers of this hardware. Still it's wrong in general and I don't want > to have others copy this.) > >> +} >> + >> +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct >> pwm_device *pwm, >> + const struct pwm_state *state) >> +{ >> + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); >> + struct sl28cpld_pwm_config *config; >> + unsigned int cycle; >> + int ret; >> + int mode; >> + u8 ctrl; >> + >> + /* Get the configuration by comparing the period */ >> + for (mode = 0; mode < ARRAY_SIZE(sl28cpld_pwm_config); mode++) { >> + config = &sl28cpld_pwm_config[mode]; >> + if (state->period == config->period_ns) >> + break; >> + } >> + >> + if (mode == ARRAY_SIZE(sl28cpld_pwm_config)) >> + return -EINVAL; > > You're supposed to pick the biggest period that isn't bigger than the > requested period. So something like: > > switch(period) { > case 4000000 ... UINT_MAX: > mode = 0; > break; > case 2000000 ... 3999999: > mode = 1; > break; > ... > } > > (or: > > if period >= 4000000: > mode = 0 > else: > // I think ... please double-check > mode = ilog2(4000000 / (period + 1)) + 1 > > if mode > 3: > return -ERANGE; > ) I see. In this case I can of course drop the table. But the rounding will be then very coarse for this driver. And there is no way to get the value which is actually set, right? You can just read the cached value. So that value might be far off the actual one set in the hardware. During testing I've also found the following problem: Assume we set a period of 5000000ns; this will be rounded to 4000000ns and written to the hardware. But the usable duty cycle is still 0..5000000ns. The driver will translate this input in the following manner: - 0..4000000 -> 0%..100% - >4000000 -> 100% Is this behavior intended? Even for PWM hardware which supports finer grained frequencies there will be some upper and lower limits. Is the user of the PWM supposed to know these? > > real_period = 4000000 >> mode; > >> + ctrl = FIELD_PREP(PWM_MODE_MASK, mode); >> + if (state->enabled) >> + ctrl |= PWM_ENABLE; >> + >> + cycle = pwm_get_relative_duty_cycle(state, config->max_duty_cycle); > > Again the rounding is wrong. You need need to round down the requested > duty_cycle to the next possible value. So something like: > > duty_cycle = min(real_period, state->duty_cycle); > > cycle = duty_cycle * (0x80 >> mode) / (4000000 >> mode); > > which can be further simplified to > > cycle = duty_cycle / 31250 Mh, this made me think where that "magic" number is coming from. Turns out this is the NSECS_PE_SEC / base clock of the PWM. I guess I'll rework the get_state() and apply() to just use this base frequency, dropping the table etc. Btw what about the polarity. Do I have to support it or can I return an error code if its != PWM_POLARITY_NORMAL? If so, which error code? EINVAL? I know I could just invert the duty cycle in software, but shouldn't this be done in the core for any controller which doesn't support changing the polarity in hardware? > > . > >> + /* >> + * The hardware doesn't allow to set max_duty_cycle if the >> + * 250Hz mode is enabled, thus we have to trap that here. >> + * But because a 100% duty cycle is equal on all modes, i.e. > > It depends on how picky you are if you can agree here. why is that? The only drawback is that the mode is changed without the user seeing it. But the PWM subsystem returns the cached state, right? get_state() is called only on device request (and during debug it seems). Actually, enabling PWM_DEBUG might choke on this workaround (".apply didn't pick the best available period"). Is this ok? > Please document > this in a Limitations paragraph at the top of the driver similar to > drivers/pwm/pwm-rcar.c and others. sure. > >> + * it is just a "all-high" output, we trap any case with a >> + * 100% duty cycle and use the 500Hz mode. > > Please only trap on 250Hz mode. (Can be done using: if (cycle == 0x80) > I > think) you are correct. > >> + */ >> + if (cycle == config->max_duty_cycle) { >> + ctrl &= ~PWM_MODE_MASK; >> + ctrl |= FIELD_PREP(PWM_MODE_MASK, PWM_MODE_500HZ); >> + cycle = PWM_CYCLE_MAX; > > I would have expected 0x40 here instead of 0x7f? Yes, but technically, any value above 0x40 will do it. But you are correct, that is wrong and misleading. >> + } >> + >> + ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl); >> + if (ret) >> + return ret; >> + >> + return regmap_write(priv->regmap, priv->offset + PWM_CYCLE, >> (u8)cycle); > > I assume this can result in broken output? Consider the hardware runs > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle = > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23? Isn't that always the case if a write may fail and there are more than one register to configure? For example, have a look at pwm-iqs620a.c. Btw. the get_state might also fail, but there is no return value to return the error. > If this cannot be avoided, please document this in the Limitations > paragraph. Sure. There might be (or most likely are) gliches when you change the mode. > >> +} >> + >> +static const struct pwm_ops sl28cpld_pwm_ops = { >> + .apply = sl28cpld_pwm_apply, >> + .get_state = sl28cpld_pwm_get_state, >> + .owner = THIS_MODULE, >> +}; >> + >> +static int sl28cpld_pwm_probe(struct platform_device *pdev) >> +{ >> + struct sl28cpld_pwm *priv; >> + struct pwm_chip *chip; >> + int ret; >> + >> + if (!pdev->dev.parent) >> + return -ENODEV; >> + >> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + priv->regmap = dev_get_regmap(pdev->dev.parent, NULL); >> + if (!priv->regmap) >> + return -ENODEV; >> + >> + ret = device_property_read_u32(&pdev->dev, "reg", &priv->offset); >> + if (ret) >> + return -EINVAL; >> + >> + /* Initialize the pwm_chip structure */ >> + chip = &priv->pwm_chip; >> + chip->dev = &pdev->dev; >> + chip->ops = &sl28cpld_pwm_ops; >> + chip->base = -1; >> + chip->npwm = 1; >> + >> + ret = pwmchip_add(&priv->pwm_chip); >> + if (ret) >> + return ret; >> + >> + platform_set_drvdata(pdev, priv); >> + >> + return 0; >> +} > > Please add error messages with some details for the error paths > (preferable using %pe to indicate the error code). Ok. > >> +static int sl28cpld_pwm_remove(struct platform_device *pdev) >> +{ >> + struct sl28cpld_pwm *priv = platform_get_drvdata(pdev); >> + >> + return pwmchip_remove(&priv->pwm_chip); >> +} >> + >> +static const struct of_device_id sl28cpld_pwm_of_match[] = { >> + { .compatible = "kontron,sl28cpld-pwm" }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, sl28cpld_pwm_of_match); >> + >> +static struct platform_driver sl28cpld_pwm_driver = { >> + .probe = sl28cpld_pwm_probe, >> + .remove = sl28cpld_pwm_remove, >> + .driver = { >> + .name = "sl28cpld-pwm", >> + .of_match_table = sl28cpld_pwm_of_match, >> + }, >> +}; >> +module_platform_driver(sl28cpld_pwm_driver); >> + >> +MODULE_DESCRIPTION("sl28cpld PWM Driver"); >> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>"); >> +MODULE_LICENSE("GPL"); -michael
Hello Michael, On Sat, Jul 11, 2020 at 07:28:05PM +0200, Michael Walle wrote: > Am 2020-07-09 10:50, schrieb Uwe Kleine-König: > > On Mon, Jul 06, 2020 at 07:53:47PM +0200, Michael Walle wrote: > > > diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c > > > new file mode 100644 > > > index 000000000000..8ee286b605bf > > > --- /dev/null > > > +++ b/drivers/pwm/pwm-sl28cpld.c > > > @@ -0,0 +1,187 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +/* > > > + * sl28cpld PWM driver > > > + * > > > + * Copyright 2020 Kontron Europe GmbH > > > + */ > > > > Is there publically available documenation available? If so please add a > > link here. > > Unfortunately not. But it should be easy enough and I'll describe it > briefly in the header. That's fine. > > > +#include <linux/bitfield.h> > > > +#include <linux/kernel.h> > > > +#include <linux/mod_devicetable.h> > > > +#include <linux/module.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/pwm.h> > > > +#include <linux/regmap.h> > > > + > > > +/* > > > + * PWM timer block registers. > > > + */ > > > +#define PWM_CTRL 0x00 > > > +#define PWM_ENABLE BIT(7) > > > +#define PWM_MODE_250HZ 0 > > > +#define PWM_MODE_500HZ 1 > > > +#define PWM_MODE_1KHZ 2 > > > +#define PWM_MODE_2KHZ 3 > > > +#define PWM_MODE_MASK GENMASK(1, 0) > > > +#define PWM_CYCLE 0x01 > > > +#define PWM_CYCLE_MAX 0x7f > > > > Please use a less generic prefix for your defines. Also I like having > > the defines for field names include register name. Something like: > > > > #define PWM_SL28CPLD_CTRL 0x00 > > #define PWM_SL28CPLD_CTRL_ENABLE BIT(7) > > #define PWM_SL28CPLD_CTRL_MODE_MASK GENMASK(1, 0) > > Ok. > > > #define > > PWM_SL28CPLD_CTRL_MODE_250HZ FIELD_PREP(PWM_SL28CPLD_CTRL_MODE_MASK, > > 0) > > Shouldn't we just "#define ..MODE_250HZ 1" use FIELD_PREP inside the code, > so you can actually use the normalized enumeration values, too? yeah, looks sane. > Actually, I'll rename the PWM_MODE to PWM_PRESCALER, because that is > more accurate. Whatever suits you and is consistent is fine for me. > > > +struct sl28cpld_pwm { > > > + struct pwm_chip pwm_chip; > > > + struct regmap *regmap; > > > + u32 offset; > > > +}; > > > + > > > +struct sl28cpld_pwm_periods { > > > + u8 ctrl; > > > + unsigned long duty_cycle; > > > +}; > > > + > > > +struct sl28cpld_pwm_config { > > > + unsigned long period_ns; > > > + u8 max_duty_cycle; > > > +}; > > > + > > > +static struct sl28cpld_pwm_config sl28cpld_pwm_config[] = { > > > > const ? (Or drop as the values can be easily computed, see below.) > > > > > + [PWM_MODE_250HZ] = { .period_ns = 4000000, .max_duty_cycle = 0x80 }, > > > + [PWM_MODE_500HZ] = { .period_ns = 2000000, .max_duty_cycle = 0x40 }, > > > + [PWM_MODE_1KHZ] = { .period_ns = 1000000, .max_duty_cycle = 0x20 }, > > > + [PWM_MODE_2KHZ] = { .period_ns = 500000, .max_duty_cycle = 0x10 }, > > > +}; > > > + > > > +static void sl28cpld_pwm_get_state(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + struct pwm_state *state) > > > +{ > > > + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); > > > + static struct sl28cpld_pwm_config *config; > > > + unsigned int reg; > > > + unsigned int mode; > > > + > > > + regmap_read(priv->regmap, priv->offset + PWM_CTRL, ®); > > > + > > > + state->enabled = reg & PWM_ENABLE; > > > > Would it be more consisted to use FIELD_GET here, too? > > I had used FIELD_GET only for bit-fields with more than one bit, > i.e. no flags. But that is just a matter of taste, I guess. I'd > prefer to keep the simple "reg & PWM_ENABLE". If you insist on > the FIELD_GET() I'll change it ;) I think using FIELD_GET is more consistent, but I won't insist. > > > + mode = FIELD_GET(PWM_MODE_MASK, reg); > > > + config = &sl28cpld_pwm_config[mode]; > > > + state->period = config->period_ns; > > > > I wonder if this could be done more effectively without the above table. > > Something like: > > > > state->period = 4000000 >> mode. > > The reason I introduced a lookup table here was that I need a > list of the supported modes; I wasn't aware of the rounding. List of supported modes = [0, 1, 2, 3], isn't it? > See also below. > > > (with a #define for 4000000 of course). > > > > > + regmap_read(priv->regmap, priv->offset + PWM_CYCLE, ®); > > > + pwm_set_relative_duty_cycle(state, reg, config->max_duty_cycle); > > > > Oh, what a creative idea to use pwm_set_relative_duty_cycle here. > > What is that helper for then? The former versions did the same > calculations (i.e. DIV_ROUND_CLOSEST_ULL()) just open coded. But > I guess then it was also rounding the wrong way. Yes. In my book pwm_set_relative_duty_cycle is for consumers. And if DIV_ROUND_CLOSEST_ULL is the right thing for them depends on their use case. > > Unfortunately it's using the wrong rounding strategy. Please enable > > PWM_DEBUG which should diagnose these problems (given enough testing). > > Is there any written documentation on how to round, i.e. up or down? There are the checks implemented for PWM_DEBUG. I started to work on the documentation (https://patchwork.ozlabs.org/project/linux-pwm/patch/20191209213233.29574-2-u.kleine-koenig@pengutronix.de/) but didn't get feedback yet. (And the rounding rules are not included there.) > I had a look Documentation/driver-api/pwm.rst again. But couldn't find > anything. A grep DIV_ROUND_CLOSEST_ULL() turns out that quite a few > drivers use it, so I did the same ;) Yes, the rounding requirement is new and many driver's are not conforming (yet). > > (Hmm, on second thought I'm not sure that rounding is relevant with the > > numbers of this hardware. Still it's wrong in general and I don't want > > to have others copy this.) > > > > > +} > > > + > > > +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct > > > pwm_device *pwm, > > > + const struct pwm_state *state) > > > +{ > > > + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); > > > + struct sl28cpld_pwm_config *config; > > > + unsigned int cycle; > > > + int ret; > > > + int mode; > > > + u8 ctrl; > > > + > > > + /* Get the configuration by comparing the period */ > > > + for (mode = 0; mode < ARRAY_SIZE(sl28cpld_pwm_config); mode++) { > > > + config = &sl28cpld_pwm_config[mode]; > > > + if (state->period == config->period_ns) > > > + break; > > > + } > > > + > > > + if (mode == ARRAY_SIZE(sl28cpld_pwm_config)) > > > + return -EINVAL; > > > > You're supposed to pick the biggest period that isn't bigger than the > > requested period. So something like: > > > > switch(period) { > > case 4000000 ... UINT_MAX: > > mode = 0; > > break; > > case 2000000 ... 3999999: > > mode = 1; > > break; > > ... > > } > > > > (or: > > > > if period >= 4000000: > > mode = 0 > > else: > > // I think ... please double-check > > mode = ilog2(4000000 / (period + 1)) + 1 > > > > if mode > 3: > > return -ERANGE; > > ) > > I see. In this case I can of course drop the table. But the rounding > will be then very coarse for this driver. And there is no way to get > the value which is actually set, right? You can just read the cached > value. So that value might be far off the actual one set in the > hardware. Yes, we once changed pwm_get_rate to return the actually implemented setting, but this broke some stuff; see commit 40a6b9a00930fd6b59aa2eb6135abc2efe5440c3. I already thought about proposing pwm_get_rate_hw(), but for now there is (AFAICT) no user who would need it. And it's hard to know which variant is actually preferred by consumers. My expectation is that most don't even care. I also have a pwm_round_rate() function in mind that will give you the actual rate without applying it. This can then be used by consumers who care. But also there is no user who would need it today. > During testing I've also found the following problem: Assume we set > a period of 5000000ns; this will be rounded to 4000000ns and written > to the hardware. But the usable duty cycle is still 0..5000000ns. The > driver will translate this input in the following manner: > - 0..4000000 -> 0%..100% > - >4000000 -> 100% > Is this behavior intended? It's expected. > Even for PWM hardware which supports finer > grained frequencies there will be some upper and lower limits. Is > the user of the PWM supposed to know these? There is nothing we can do on hardware imposed limits. In practise it doesn't seem to matter. Also note that most consumers get a proposed period length. > > real_period = 4000000 >> mode; > > > > > + ctrl = FIELD_PREP(PWM_MODE_MASK, mode); > > > + if (state->enabled) > > > + ctrl |= PWM_ENABLE; > > > + > > > + cycle = pwm_get_relative_duty_cycle(state, config->max_duty_cycle); > > > > Again the rounding is wrong. You need need to round down the requested > > duty_cycle to the next possible value. So something like: > > > > duty_cycle = min(real_period, state->duty_cycle); > > > > cycle = duty_cycle * (0x80 >> mode) / (4000000 >> mode); > > > > which can be further simplified to > > > > cycle = duty_cycle / 31250 > > Mh, this made me think where that "magic" number is coming from. Turns > out this is the NSECS_PE_SEC / base clock of the PWM. It's a simplification of the line above, so it is 4000000 / 0x80. (But it's not by chance this matches NSECS_PER_SEC / base clock of course.) > I guess I'll rework the get_state() and apply() to just use this > base frequency, dropping the table etc. > > Btw what about the polarity. Do I have to support it or can I > return an error code if its != PWM_POLARITY_NORMAL? If so, which > error code? EINVAL? ..ooOO(Did I really miss that during review? Bummer) If your hardware only support normal polarity, only implement this and return -EINVAL for inverted polarity requests. > I know I could just invert the duty cycle in > software, but shouldn't this be done in the core for any controller > which doesn't support changing the polarity in hardware? Please don't. This should indeed be done at the framework level. (But I'm not convinced doing this unconditionally is a good idea.) > > > + /* > > > + * The hardware doesn't allow to set max_duty_cycle if the > > > + * 250Hz mode is enabled, thus we have to trap that here. > > > + * But because a 100% duty cycle is equal on all modes, i.e. > > > > It depends on how picky you are if you can agree here. > > why is that? The only drawback is that the mode is changed without > the user seeing it. Ideally periods are completed before they change. So if a user requests .period = .duty_cycle = 100ms with having the PWM disabled before and afterwards, the expectation is that it is active for (an integer multiple of) 100 ms. But honestly there are not many hardware implementation + driver combos that get this right. > But the PWM subsystem returns the cached state, > right? get_state() is called only on device request (and during > debug it seems). Actually, enabling PWM_DEBUG might choke on this > workaround (".apply didn't pick the best available period"). Is > this ok? hmm, I didn't consider this when writing the checks for PWM_DEBUG. According to the currently checked rules the expected configuration is to pick the 250Hz mode and use cycle = 0x7f. Hmm, I have to think about this. Maybe we should weaken the check to the cases with 0 < duty_cycle < period. Thierry, what do you think? Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f seems to be far from reality. (Is it?) > > > + ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl); > > > + if (ret) > > > + return ret; > > > + > > > + return regmap_write(priv->regmap, priv->offset + PWM_CYCLE, > > > (u8)cycle); > > > > I assume this can result in broken output? Consider the hardware runs > > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle = > > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23? > > Isn't that always the case if a write may fail and there are more than > one register to configure? Depending on hardware capabilities you might not be able to prevent this yes. Unfortunately this is quite common. But there are hardware implementations that are not prone to such failures. (E.g. the registers written can be only shadow values that are latched into hardware only when the last value is written.) > For example, have a look at pwm-iqs620a.c. > Btw. the get_state might also fail, but there is no return value to > return the error. Yes, changing this is on my todo list. > > If this cannot be avoided, please document this in the Limitations > > paragraph. > > Sure. There might be (or most likely are) gliches when you change the > mode. If you change only cycle but not mode, does the hardware complete the currently running period? What about disable()? Best regards Uwe
Hi Uwe, Am 2020-07-13 10:47, schrieb Uwe Kleine-König: > On Sat, Jul 11, 2020 at 07:28:05PM +0200, Michael Walle wrote: >> Am 2020-07-09 10:50, schrieb Uwe Kleine-König: >> > On Mon, Jul 06, 2020 at 07:53:47PM +0200, Michael Walle wrote: >> > > diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c >> > > new file mode 100644 >> > > index 000000000000..8ee286b605bf >> > > --- /dev/null >> > > +++ b/drivers/pwm/pwm-sl28cpld.c >> > > @@ -0,0 +1,187 @@ >> > > +// SPDX-License-Identifier: GPL-2.0-only >> > > +/* >> > > + * sl28cpld PWM driver >> > > + * >> > > + * Copyright 2020 Kontron Europe GmbH >> > > + */ >> > >> > Is there publically available documenation available? If so please add a >> > link here. >> >> Unfortunately not. But it should be easy enough and I'll describe it >> briefly in the header. > > That's fine. > >> > > +#include <linux/bitfield.h> >> > > +#include <linux/kernel.h> >> > > +#include <linux/mod_devicetable.h> >> > > +#include <linux/module.h> >> > > +#include <linux/platform_device.h> >> > > +#include <linux/pwm.h> >> > > +#include <linux/regmap.h> >> > > + >> > > +/* >> > > + * PWM timer block registers. >> > > + */ >> > > +#define PWM_CTRL 0x00 >> > > +#define PWM_ENABLE BIT(7) >> > > +#define PWM_MODE_250HZ 0 >> > > +#define PWM_MODE_500HZ 1 >> > > +#define PWM_MODE_1KHZ 2 >> > > +#define PWM_MODE_2KHZ 3 >> > > +#define PWM_MODE_MASK GENMASK(1, 0) >> > > +#define PWM_CYCLE 0x01 >> > > +#define PWM_CYCLE_MAX 0x7f >> > >> > Please use a less generic prefix for your defines. Also I like having >> > the defines for field names include register name. Something like: >> > >> > #define PWM_SL28CPLD_CTRL 0x00 >> > #define PWM_SL28CPLD_CTRL_ENABLE BIT(7) >> > #define PWM_SL28CPLD_CTRL_MODE_MASK GENMASK(1, 0) >> >> Ok. >> >> > #define >> > PWM_SL28CPLD_CTRL_MODE_250HZ FIELD_PREP(PWM_SL28CPLD_CTRL_MODE_MASK, >> > 0) >> >> Shouldn't we just "#define ..MODE_250HZ 1" use FIELD_PREP inside the >> code, >> so you can actually use the normalized enumeration values, too? > > yeah, looks sane. > >> Actually, I'll rename the PWM_MODE to PWM_PRESCALER, because that is >> more accurate. > > Whatever suits you and is consistent is fine for me. > >> > > +struct sl28cpld_pwm { >> > > + struct pwm_chip pwm_chip; >> > > + struct regmap *regmap; >> > > + u32 offset; >> > > +}; >> > > + >> > > +struct sl28cpld_pwm_periods { >> > > + u8 ctrl; >> > > + unsigned long duty_cycle; >> > > +}; >> > > + >> > > +struct sl28cpld_pwm_config { >> > > + unsigned long period_ns; >> > > + u8 max_duty_cycle; >> > > +}; >> > > + >> > > +static struct sl28cpld_pwm_config sl28cpld_pwm_config[] = { >> > >> > const ? (Or drop as the values can be easily computed, see below.) >> > >> > > + [PWM_MODE_250HZ] = { .period_ns = 4000000, .max_duty_cycle = 0x80 }, >> > > + [PWM_MODE_500HZ] = { .period_ns = 2000000, .max_duty_cycle = 0x40 }, >> > > + [PWM_MODE_1KHZ] = { .period_ns = 1000000, .max_duty_cycle = 0x20 }, >> > > + [PWM_MODE_2KHZ] = { .period_ns = 500000, .max_duty_cycle = 0x10 }, >> > > +}; >> > > + >> > > +static void sl28cpld_pwm_get_state(struct pwm_chip *chip, >> > > + struct pwm_device *pwm, >> > > + struct pwm_state *state) >> > > +{ >> > > + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); >> > > + static struct sl28cpld_pwm_config *config; >> > > + unsigned int reg; >> > > + unsigned int mode; >> > > + >> > > + regmap_read(priv->regmap, priv->offset + PWM_CTRL, ®); >> > > + >> > > + state->enabled = reg & PWM_ENABLE; >> > >> > Would it be more consisted to use FIELD_GET here, too? >> >> I had used FIELD_GET only for bit-fields with more than one bit, >> i.e. no flags. But that is just a matter of taste, I guess. I'd >> prefer to keep the simple "reg & PWM_ENABLE". If you insist on >> the FIELD_GET() I'll change it ;) > > I think using FIELD_GET is more consistent, but I won't insist. > >> > > + mode = FIELD_GET(PWM_MODE_MASK, reg); >> > > + config = &sl28cpld_pwm_config[mode]; >> > > + state->period = config->period_ns; >> > >> > I wonder if this could be done more effectively without the above table. >> > Something like: >> > >> > state->period = 4000000 >> mode. >> >> The reason I introduced a lookup table here was that I need a >> list of the supported modes; I wasn't aware of the rounding. > > List of supported modes = [0, 1, 2, 3], isn't it? Back then it was the list of supported periods. But because we do the rounding now, we won't need it anymore. >> See also below. >> >> > (with a #define for 4000000 of course). >> > >> > > + regmap_read(priv->regmap, priv->offset + PWM_CYCLE, ®); >> > > + pwm_set_relative_duty_cycle(state, reg, config->max_duty_cycle); >> > >> > Oh, what a creative idea to use pwm_set_relative_duty_cycle here. >> >> What is that helper for then? The former versions did the same >> calculations (i.e. DIV_ROUND_CLOSEST_ULL()) just open coded. But >> I guess then it was also rounding the wrong way. > > Yes. In my book pwm_set_relative_duty_cycle is for consumers. And if > DIV_ROUND_CLOSEST_ULL is the right thing for them depends on their use > case. > >> > Unfortunately it's using the wrong rounding strategy. Please enable >> > PWM_DEBUG which should diagnose these problems (given enough testing). >> >> Is there any written documentation on how to round, i.e. up or down? > > There are the checks implemented for PWM_DEBUG. I started to work on > the > documentation > (https://patchwork.ozlabs.org/project/linux-pwm/patch/20191209213233.29574-2-u.kleine-koenig@pengutronix.de/) > but didn't get feedback yet. (And the rounding rules are not included > there.) > >> I had a look Documentation/driver-api/pwm.rst again. But couldn't find >> anything. A grep DIV_ROUND_CLOSEST_ULL() turns out that quite a few >> drivers use it, so I did the same ;) > > Yes, the rounding requirement is new and many driver's are not > conforming (yet). ok, I'll then compute everything then and drop the table. >> > (Hmm, on second thought I'm not sure that rounding is relevant with the >> > numbers of this hardware. Still it's wrong in general and I don't want >> > to have others copy this.) >> > >> > > +} >> > > + >> > > +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct >> > > pwm_device *pwm, >> > > + const struct pwm_state *state) >> > > +{ >> > > + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); >> > > + struct sl28cpld_pwm_config *config; >> > > + unsigned int cycle; >> > > + int ret; >> > > + int mode; >> > > + u8 ctrl; >> > > + >> > > + /* Get the configuration by comparing the period */ >> > > + for (mode = 0; mode < ARRAY_SIZE(sl28cpld_pwm_config); mode++) { >> > > + config = &sl28cpld_pwm_config[mode]; >> > > + if (state->period == config->period_ns) >> > > + break; >> > > + } >> > > + >> > > + if (mode == ARRAY_SIZE(sl28cpld_pwm_config)) >> > > + return -EINVAL; >> > >> > You're supposed to pick the biggest period that isn't bigger than the >> > requested period. So something like: >> > >> > switch(period) { >> > case 4000000 ... UINT_MAX: >> > mode = 0; >> > break; >> > case 2000000 ... 3999999: >> > mode = 1; >> > break; >> > ... >> > } >> > >> > (or: >> > >> > if period >= 4000000: >> > mode = 0 >> > else: >> > // I think ... please double-check >> > mode = ilog2(4000000 / (period + 1)) + 1 >> > >> > if mode > 3: >> > return -ERANGE; >> > ) >> >> I see. In this case I can of course drop the table. But the rounding >> will be then very coarse for this driver. And there is no way to get >> the value which is actually set, right? You can just read the cached >> value. So that value might be far off the actual one set in the >> hardware. > > Yes, we once changed pwm_get_rate to return the actually implemented > setting, but this broke some stuff; see commit > 40a6b9a00930fd6b59aa2eb6135abc2efe5440c3. > > I already thought about proposing pwm_get_rate_hw(), but for now there > is (AFAICT) no user who would need it. And it's hard to know which > variant is actually preferred by consumers. My expectation is that most > don't even care. > > I also have a pwm_round_rate() function in mind that will give you the > actual rate without applying it. This can then be used by consumers who > care. But also there is no user who would need it today. Ok. I take it that all such improvements are still in the making ;) >> During testing I've also found the following problem: Assume we set >> a period of 5000000ns; this will be rounded to 4000000ns and written >> to the hardware. But the usable duty cycle is still 0..5000000ns. The >> driver will translate this input in the following manner: >> - 0..4000000 -> 0%..100% >> - >4000000 -> 100% >> Is this behavior intended? > > It's expected. ok >> Even for PWM hardware which supports finer >> grained frequencies there will be some upper and lower limits. Is >> the user of the PWM supposed to know these? > > There is nothing we can do on hardware imposed limits. In practise it > doesn't seem to matter. Also note that most consumers get a proposed > period length. > >> > real_period = 4000000 >> mode; >> > >> > > + ctrl = FIELD_PREP(PWM_MODE_MASK, mode); >> > > + if (state->enabled) >> > > + ctrl |= PWM_ENABLE; >> > > + >> > > + cycle = pwm_get_relative_duty_cycle(state, config->max_duty_cycle); >> > >> > Again the rounding is wrong. You need need to round down the requested >> > duty_cycle to the next possible value. So something like: >> > >> > duty_cycle = min(real_period, state->duty_cycle); >> > >> > cycle = duty_cycle * (0x80 >> mode) / (4000000 >> mode); >> > >> > which can be further simplified to >> > >> > cycle = duty_cycle / 31250 >> >> Mh, this made me think where that "magic" number is coming from. Turns >> out this is the NSECS_PE_SEC / base clock of the PWM. > > It's a simplification of the line above, so it is 4000000 / 0x80. (But > it's not by chance this matches NSECS_PER_SEC / base clock of course.) > >> I guess I'll rework the get_state() and apply() to just use this >> base frequency, dropping the table etc. >> >> Btw what about the polarity. Do I have to support it or can I >> return an error code if its != PWM_POLARITY_NORMAL? If so, which >> error code? EINVAL? > > ..ooOO(Did I really miss that during review? Bummer) > > If your hardware only support normal polarity, only implement this and > return -EINVAL for inverted polarity requests. ok >> I know I could just invert the duty cycle in >> software, but shouldn't this be done in the core for any controller >> which doesn't support changing the polarity in hardware? > > Please don't. This should indeed be done at the framework level. (But > I'm not convinced doing this unconditionally is a good idea.) > >> > > + /* >> > > + * The hardware doesn't allow to set max_duty_cycle if the >> > > + * 250Hz mode is enabled, thus we have to trap that here. >> > > + * But because a 100% duty cycle is equal on all modes, i.e. >> > >> > It depends on how picky you are if you can agree here. >> >> why is that? The only drawback is that the mode is changed without >> the user seeing it. > > Ideally periods are completed before they change. So if a user requests > .period = .duty_cycle = 100ms with having the PWM disabled before and > afterwards, the expectation is that it is active for (an integer > multiple of) 100 ms. But honestly there are not many hardware > implementation + driver combos that get this right. > >> But the PWM subsystem returns the cached state, >> right? get_state() is called only on device request (and during >> debug it seems). Actually, enabling PWM_DEBUG might choke on this >> workaround (".apply didn't pick the best available period"). Is >> this ok? > > hmm, I didn't consider this when writing the checks for PWM_DEBUG. > According to the currently checked rules the expected configuration is > to pick the 250Hz mode and use cycle = 0x7f. Not to use 0x80, which is the max_duty_cycle? Like its 0x40 for the 500Hz mode. > Hmm, I have to think about > this. Maybe we should weaken the check to the cases with > 0 < duty_cycle < period. Thierry, what do you think? > > Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f > seems > to be far from reality. (Is it?) If you mean by insisting to clip at 0x7f, yeah thats bad IMHO, because the user wants an all-high line, but in the end it would be a toggling line. It wouldn't be that bad for anything in between 0% and 100% but IMHO its bad for exactly 0% and 100%. You could also ask the driver about known quirks, like special 0% and 100% handling and exclude it from the tests accordingly. >> > > + ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl); >> > > + if (ret) >> > > + return ret; >> > > + >> > > + return regmap_write(priv->regmap, priv->offset + PWM_CYCLE, >> > > (u8)cycle); >> > >> > I assume this can result in broken output? Consider the hardware runs >> > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle = >> > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23? >> >> Isn't that always the case if a write may fail and there are more than >> one register to configure? > > Depending on hardware capabilities you might not be able to prevent > this yes. Unfortunately this is quite common. > > But there are hardware implementations that are not prone to such > failures. (E.g. the registers written can be only shadow values that > are > latched into hardware only when the last value is written.) Maybe this could be improved in the future. > >> For example, have a look at pwm-iqs620a.c. >> Btw. the get_state might also fail, but there is no return value to >> return the error. > > Yes, changing this is on my todo list. > >> > If this cannot be avoided, please document this in the Limitations >> > paragraph. >> >> Sure. There might be (or most likely are) gliches when you change the >> mode. > > If you change only cycle but not mode, does the hardware complete the > currently running period? No it does not. > What about disable()? Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can fix that (in hardware), not much we can do in the driver here. We are _very_ constraint in size, therefore all that little edge cases fall off the table. I'll post a new version soon. -michael
Hello Michael, On Tue, Jul 14, 2020 at 01:31:13PM +0200, Michael Walle wrote: > Am 2020-07-13 10:47, schrieb Uwe Kleine-König: > > I already thought about proposing pwm_get_rate_hw(), but for now there > > is (AFAICT) no user who would need it. And it's hard to know which > > variant is actually preferred by consumers. My expectation is that most > > don't even care. > > > > I also have a pwm_round_rate() function in mind that will give you the > > actual rate without applying it. This can then be used by consumers who > > care. But also there is no user who would need it today. > > Ok. I take it that all such improvements are still in the making ;) If you have a real use case, present it, then I give it a boost on my todo list. > > > But the PWM subsystem returns the cached state, > > > right? get_state() is called only on device request (and during > > > debug it seems). Actually, enabling PWM_DEBUG might choke on this > > > workaround (".apply didn't pick the best available period"). Is > > > this ok? > > > > hmm, I didn't consider this when writing the checks for PWM_DEBUG. > > According to the currently checked rules the expected configuration is > > to pick the 250Hz mode and use cycle = 0x7f. > > Not to use 0x80, which is the max_duty_cycle? Like its 0x40 for the 500Hz > mode. No, when I thought about a sane set of rules (and so checks for PWM_DEBUG) I didn't consider a PWM that can implement 100% but not for all otherwise available period lengths. I'm still amazed sometimes how different the capabilities of different implementations for something so seemingly easy like a PWM are. > > Hmm, I have to think about > > this. Maybe we should weaken the check to the cases with > > 0 < duty_cycle < period. Thierry, what do you think? > > > > Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f seems > > to be far from reality. (Is it?) > > If you mean by insisting to clip at 0x7f, yeah thats bad IMHO, because > the user wants an all-high line, but in the end it would be a toggling > line. It wouldn't be that bad for anything in between 0% and 100% but > IMHO its bad for exactly 0% and 100%. > > You could also ask the driver about known quirks, like special 0% and > 100% handling and exclude it from the tests accordingly. Do you care enough to propose a patch? You're in the situation to test it. > > > > > + ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + return regmap_write(priv->regmap, priv->offset + PWM_CYCLE, > > > > > (u8)cycle); > > > > > > > > I assume this can result in broken output? Consider the hardware runs > > > > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle = > > > > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23? > > > > > > Isn't that always the case if a write may fail and there are more than > > > one register to configure? > > > > Depending on hardware capabilities you might not be able to prevent > > this yes. Unfortunately this is quite common. > > > > But there are hardware implementations that are not prone to such > > failures. (E.g. the registers written can be only shadow values that are > > latched into hardware only when the last value is written.) > > Maybe this could be improved in the future. We should somewhere describe, what an ideal PWM can do. My wishlist (just as it comes to my mind, so no guarantee of completeness): - can do 0% duty cycle for all supported period lengths - can do 100% duty cycle for all supported period lengths - supports both polarities - supports immediate change of configuration and after completion of the currently running period - atomic update (i.e. if you go from configuration A to configuration B the hardware guarantees to only emit periods of type A and then type B. (Depending on the item above, the last A period might be cut off.) - emits an irq when configuration changes > > If you change only cycle but not mode, does the hardware complete the > > currently running period? > > No it does not. Please document this as a Limitation. > > What about disable()? > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can > fix that (in hardware), not much we can do in the driver here. We are > _very_ constraint in size, therefore all that little edge cases fall off > the table. You're saying that on disable the hardware emits a constant high level for one cycle? I hope not ... I never programmed a CPLD to emulate a hardware PWM, but I wonder if these are really edge cases that increase the size of the binary?! Best regards Uwe
Hi Uwe, Am 2020-07-14 18:08, schrieb Uwe Kleine-König: > On Tue, Jul 14, 2020 at 01:31:13PM +0200, Michael Walle wrote: >> Am 2020-07-13 10:47, schrieb Uwe Kleine-König: >> > I already thought about proposing pwm_get_rate_hw(), but for now there >> > is (AFAICT) no user who would need it. And it's hard to know which >> > variant is actually preferred by consumers. My expectation is that most >> > don't even care. >> > >> > I also have a pwm_round_rate() function in mind that will give you the >> > actual rate without applying it. This can then be used by consumers who >> > care. But also there is no user who would need it today. >> >> Ok. I take it that all such improvements are still in the making ;) > > If you have a real use case, present it, then I give it a boost on my > todo list. Not really. >> > > But the PWM subsystem returns the cached state, >> > > right? get_state() is called only on device request (and during >> > > debug it seems). Actually, enabling PWM_DEBUG might choke on this >> > > workaround (".apply didn't pick the best available period"). Is >> > > this ok? >> > >> > hmm, I didn't consider this when writing the checks for PWM_DEBUG. >> > According to the currently checked rules the expected configuration is >> > to pick the 250Hz mode and use cycle = 0x7f. >> >> Not to use 0x80, which is the max_duty_cycle? Like its 0x40 for the >> 500Hz >> mode. > > No, when I thought about a sane set of rules (and so checks for > PWM_DEBUG) I didn't consider a PWM that can implement 100% but not for > all otherwise available period lengths. I'm still amazed sometimes how > different the capabilities of different implementations for something > so > seemingly easy like a PWM are. > >> > Hmm, I have to think about >> > this. Maybe we should weaken the check to the cases with >> > 0 < duty_cycle < period. Thierry, what do you think? >> > >> > Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f seems >> > to be far from reality. (Is it?) >> >> If you mean by insisting to clip at 0x7f, yeah thats bad IMHO, because >> the user wants an all-high line, but in the end it would be a toggling >> line. It wouldn't be that bad for anything in between 0% and 100% but >> IMHO its bad for exactly 0% and 100%. >> >> You could also ask the driver about known quirks, like special 0% and >> 100% handling and exclude it from the tests accordingly. > > Do you care enough to propose a patch? You're in the situation to test > it. Ok. I'll come up with something outside of this patch series. >> > > > > + ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl); >> > > > > + if (ret) >> > > > > + return ret; >> > > > > + >> > > > > + return regmap_write(priv->regmap, priv->offset + PWM_CYCLE, >> > > > > (u8)cycle); >> > > > >> > > > I assume this can result in broken output? Consider the hardware runs >> > > > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle = >> > > > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23? >> > > >> > > Isn't that always the case if a write may fail and there are more than >> > > one register to configure? >> > >> > Depending on hardware capabilities you might not be able to prevent >> > this yes. Unfortunately this is quite common. >> > >> > But there are hardware implementations that are not prone to such >> > failures. (E.g. the registers written can be only shadow values that are >> > latched into hardware only when the last value is written.) >> >> Maybe this could be improved in the future. > > We should somewhere describe, what an ideal PWM can do. > My wishlist (just as it comes to my mind, so no guarantee of > completeness): > > - can do 0% duty cycle for all supported period lengths > - can do 100% duty cycle for all supported period lengths > - supports both polarities > - supports immediate change of configuration and after completion of > the currently running period > - atomic update (i.e. if you go from configuration A to configuration > B > the hardware guarantees to only emit periods of type A and then type > B. (Depending on the item above, the last A period might be cut > off.) We actually discussed this, because the implementation would be easier. But if the change takes place immediately you might end up with a longer duty cycle. Assume the PWM runs at 80% duty cycle and starts with the on-period. If you now change that to 50% you might end up with one successive duty cycle of "130%". Eg. the 80% of the old and right after that you switch to the new 50% and then you'd have a high output which corresponds to a 130% cycle. I don't know if that is acceptable for all applications. > - emits an irq when configuration changes Why would you need the interrupt? > >> > If you change only cycle but not mode, does the hardware complete the >> > currently running period? >> >> No it does not. > > Please document this as a Limitation. I've discussed this internally, for now its a limitation. In the worst case you'd do one 100% duty cycle. Maybe we can fix the hardware. I acknowledge that this is a severe limitation, esp. if you use the PWM for controlling stuff (for now its only LCD backlight.. so thats ok). >> > What about disable()? >> >> Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can >> fix that (in hardware), not much we can do in the driver here. We are >> _very_ constraint in size, therefore all that little edge cases fall >> off >> the table. > > You're saying that on disable the hardware emits a constant high level > for one cycle? I hope not ... Mh, I was mistaken, disabling the PWM will turn it off immediately, but one 100% duty cycle may happen if you change from a higher to a lower duty cycle setting. See above. > I never programmed a CPLD to emulate a hardware PWM, but I wonder if > these are really edge cases that increase the size of the binary?! At the moment there is only one 8bit register which stores the value which is used for matching. If you want to change that setting after a whole cycle, you'd use another 8bit register to cache the new value. So this would at least needs 8 additional flip-flops. This doesn't sound much, but we are already near 100% usage of the CPLD. So its hard to convince people why this is really necessary. -michael
Hello Michael, On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote: > > My wishlist (just as it comes to my mind, so no guarantee of > > completeness): > > > > - can do 0% duty cycle for all supported period lengths > > - can do 100% duty cycle for all supported period lengths > > - supports both polarities > > - supports immediate change of configuration and after completion of > > the currently running period > > - atomic update (i.e. if you go from configuration A to configuration B > > the hardware guarantees to only emit periods of type A and then type > > B. (Depending on the item above, the last A period might be cut off.) > > We actually discussed this, because the implementation would be easier. But > if the change takes place immediately you might end up with a longer duty > cycle. Assume the PWM runs at 80% duty cycle and starts with the on-period. > If you now change that to 50% you might end up with one successive duty > cycle of "130%". Eg. the 80% of the old and right after that you switch to > the new 50% and then you'd have a high output which corresponds to a 130% > cycle. I don't know if that is acceptable for all applications. I thought this is a "change takes place immediately" implementation?! So these problems are actually real here. (And this not happening is exactly my wish here. Is there a mis-understanding?) > > - emits an irq when configuration changes > > Why would you need the interrupt? To know that the new setting is active. Currently Thierry's ideal PWM implementation blocks in pwm_apply_state() until the new setting is active. So some signaling is nice. > > > > If you change only cycle but not mode, does the hardware complete the > > > > currently running period? > > > > > > No it does not. > > > > Please document this as a Limitation. > > I've discussed this internally, for now its a limitation. In the worst > case you'd do one 100% duty cycle. Maybe we can fix the hardware. I > acknowledge that this is a severe limitation, esp. if you use the PWM > for controlling stuff (for now its only LCD backlight.. so thats ok). That happens if you reduce the duty cycle from A to B and the counter is already bigger than B but smaller than A, right? The fix would be to compare for counter >= match instead of counter = match. (Which then would result in a period with a duty cycle bigger than B but smaller than A. Also not ideal, but probably better.) > > > > What about disable()? > > > > > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can > > > fix that (in hardware), not much we can do in the driver here. We are > > > _very_ constraint in size, therefore all that little edge cases fall > > > off > > > the table. > > > > You're saying that on disable the hardware emits a constant high level > > for one cycle? I hope not ... > > Mh, I was mistaken, disabling the PWM will turn it off immediately, but And does turn off mean, the output gets inactive? If so you might also disable the hardware if a 0% duty cycle is configured assuming this saves some energy without modifying the resulting wave form. > one 100% duty cycle may happen if you change from a higher to a lower > duty cycle setting. See above. > > > I never programmed a CPLD to emulate a hardware PWM, but I wonder if > > these are really edge cases that increase the size of the binary?! > > At the moment there is only one 8bit register which stores the value > which is used for matching. If you want to change that setting after > a whole cycle, you'd use another 8bit register to cache the new value. > So this would at least needs 8 additional flip-flops. This doesn't > sound much, but we are already near 100% usage of the CPLD. So its > hard to convince people why this is really necessary. OK. (Maybe there is enough space to allow implementing 100% for mode 0?) Best regards Uwe
Hi Uwe, Am 2020-07-15 18:36, schrieb Uwe Kleine-König: > On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote: >> > My wishlist (just as it comes to my mind, so no guarantee of >> > completeness): >> > >> > - can do 0% duty cycle for all supported period lengths >> > - can do 100% duty cycle for all supported period lengths >> > - supports both polarities >> > - supports immediate change of configuration and after completion of >> > the currently running period >> > - atomic update (i.e. if you go from configuration A to configuration B >> > the hardware guarantees to only emit periods of type A and then type >> > B. (Depending on the item above, the last A period might be cut off.) >> >> We actually discussed this, because the implementation would be >> easier. But >> if the change takes place immediately you might end up with a longer >> duty >> cycle. Assume the PWM runs at 80% duty cycle and starts with the >> on-period. >> If you now change that to 50% you might end up with one successive >> duty >> cycle of "130%". Eg. the 80% of the old and right after that you >> switch to >> the new 50% and then you'd have a high output which corresponds to a >> 130% >> cycle. I don't know if that is acceptable for all applications. > > I thought this is a "change takes place immediately" implementation?! > So > these problems are actually real here. (And this not happening is > exactly > my wish here. Is there a mis-understanding?) I wasn't talking about the sl28cpld btw. What is the difference between your proposed "change take place immediately" and "after the cycle". I understand how the after the cycle should work. But how would the immediate change work in your ideal PWM? >> > > > If you change only cycle but not mode, does the hardware complete the >> > > > currently running period? >> > > >> > > No it does not. >> > >> > Please document this as a Limitation. >> >> I've discussed this internally, for now its a limitation. In the worst >> case you'd do one 100% duty cycle. Maybe we can fix the hardware. I >> acknowledge that this is a severe limitation, esp. if you use the PWM >> for controlling stuff (for now its only LCD backlight.. so thats ok). > > That happens if you reduce the duty cycle from A to B and the counter > is > already bigger than B but smaller than A, right? That is correct. > The fix would be to > compare for counter >= match instead of counter = match. (Which then > would result in a period with a duty cycle bigger than B but smaller > than A. Also not ideal, but probably better.) This would actually work. I could imagine that comparing "less than" will take up more space again. But it would be worth a try; see also below. >> > > > What about disable()? >> > > >> > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can >> > > fix that (in hardware), not much we can do in the driver here. We are >> > > _very_ constraint in size, therefore all that little edge cases fall >> > > off >> > > the table. >> > >> > You're saying that on disable the hardware emits a constant high level >> > for one cycle? I hope not ... >> >> Mh, I was mistaken, disabling the PWM will turn it off immediately, >> but > > And does turn off mean, the output gets inactive? > If so you might also disable the hardware if a 0% duty cycle is > configured assuming this saves some energy without modifying the > resulting wave form. Disabling it has some side effects like switching to another function for this multi function pin. So I'd rather keep it on ;) >> one 100% duty cycle may happen if you change from a higher to a lower >> duty cycle setting. See above. >> >> > I never programmed a CPLD to emulate a hardware PWM, but I wonder if >> > these are really edge cases that increase the size of the binary?! >> >> At the moment there is only one 8bit register which stores the value >> which is used for matching. If you want to change that setting after >> a whole cycle, you'd use another 8bit register to cache the new value. >> So this would at least needs 8 additional flip-flops. This doesn't >> sound much, but we are already near 100% usage of the CPLD. So its >> hard to convince people why this is really necessary. > > OK. (Maybe there is enough space to allow implementing 100% for mode > 0?) Little bit here a little bit there ;) TBH there are some more critical bugs which would need to be fixed first. So this would need to be a limitation for now. -michael
On Wed, Jul 15, 2020 at 07:45:10PM +0200, Michael Walle wrote: > Hi Uwe, > > Am 2020-07-15 18:36, schrieb Uwe Kleine-König: > > On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote: > > > > My wishlist (just as it comes to my mind, so no guarantee of > > > > completeness): > > > > > > > > - can do 0% duty cycle for all supported period lengths > > > > - can do 100% duty cycle for all supported period lengths > > > > - supports both polarities > > > > - supports immediate change of configuration and after completion of > > > > the currently running period > > > > - atomic update (i.e. if you go from configuration A to configuration B > > > > the hardware guarantees to only emit periods of type A and then type > > > > B. (Depending on the item above, the last A period might be cut off.) > > > > > > We actually discussed this, because the implementation would be > > > easier. But > > > if the change takes place immediately you might end up with a longer > > > duty > > > cycle. Assume the PWM runs at 80% duty cycle and starts with the > > > on-period. > > > If you now change that to 50% you might end up with one successive > > > duty > > > cycle of "130%". Eg. the 80% of the old and right after that you > > > switch to > > > the new 50% and then you'd have a high output which corresponds to a > > > 130% > > > cycle. I don't know if that is acceptable for all applications. > > > > I thought this is a "change takes place immediately" implementation?! So > > these problems are actually real here. (And this not happening is > > exactly > > my wish here. Is there a mis-understanding?) > > I wasn't talking about the sl28cpld btw. What is the difference between > your proposed "change take place immediately" and "after the cycle". > I understand how the after the cycle should work. But how would the > immediate change work in your ideal PWM? If the PWM is running at 1/3 duty cycle and reconfigured for 2/3, then the two scenarios are (the * marks the moment where pwm_apply_state() is called, ^ marks the start of a period): immediately: __ __ _____ _____ / \_____/ \__/ \__/ ^ ^ ^ ^ * after the cycle __ __ _____ _____ / \_____/ \_____/ \__/ ^ ^ ^ ^ * and with my ideal PWM I can choose which of the two behaviours I want. > > > > > > What about disable()? > > > > > > > > > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can > > > > > fix that (in hardware), not much we can do in the driver here. We are > > > > > _very_ constraint in size, therefore all that little edge cases fall > > > > > off > > > > > the table. > > > > > > > > You're saying that on disable the hardware emits a constant high level > > > > for one cycle? I hope not ... > > > > > > Mh, I was mistaken, disabling the PWM will turn it off immediately, > > > but > > > > And does turn off mean, the output gets inactive? > > If so you might also disable the hardware if a 0% duty cycle is > > configured assuming this saves some energy without modifying the > > resulting wave form. > > Disabling it has some side effects like switching to another function > for this multi function pin. So I'd rather keep it on ;) So IMHO you should also keep it on when pwm_apply_state is called with state.enabled = false to ensure a low output. > > > one 100% duty cycle may happen if you change from a higher to a lower > > > duty cycle setting. See above. > > > > > > > I never programmed a CPLD to emulate a hardware PWM, but I wonder if > > > > these are really edge cases that increase the size of the binary?! > > > > > > At the moment there is only one 8bit register which stores the value > > > which is used for matching. If you want to change that setting after > > > a whole cycle, you'd use another 8bit register to cache the new value. > > > So this would at least needs 8 additional flip-flops. This doesn't > > > sound much, but we are already near 100% usage of the CPLD. So its > > > hard to convince people why this is really necessary. > > > > OK. (Maybe there is enough space to allow implementing 100% for mode 0?) > > Little bit here a little bit there ;) TBH there are some more critical > bugs which would need to be fixed first. So this would need to be a > limitation for now. Ok for me. Best regards Uwe
Hi Uwe, Am 2020-07-15 20:18, schrieb Uwe Kleine-König: > On Wed, Jul 15, 2020 at 07:45:10PM +0200, Michael Walle wrote: >> >> Am 2020-07-15 18:36, schrieb Uwe Kleine-König: >> > On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote: >> > > > My wishlist (just as it comes to my mind, so no guarantee of >> > > > completeness): >> > > > >> > > > - can do 0% duty cycle for all supported period lengths >> > > > - can do 100% duty cycle for all supported period lengths >> > > > - supports both polarities >> > > > - supports immediate change of configuration and after completion of >> > > > the currently running period >> > > > - atomic update (i.e. if you go from configuration A to configuration B >> > > > the hardware guarantees to only emit periods of type A and then type >> > > > B. (Depending on the item above, the last A period might be cut off.) >> > > >> > > We actually discussed this, because the implementation would be >> > > easier. But >> > > if the change takes place immediately you might end up with a longer >> > > duty >> > > cycle. Assume the PWM runs at 80% duty cycle and starts with the >> > > on-period. >> > > If you now change that to 50% you might end up with one successive >> > > duty >> > > cycle of "130%". Eg. the 80% of the old and right after that you >> > > switch to >> > > the new 50% and then you'd have a high output which corresponds to a >> > > 130% >> > > cycle. I don't know if that is acceptable for all applications. >> > >> > I thought this is a "change takes place immediately" implementation?! So >> > these problems are actually real here. (And this not happening is >> > exactly >> > my wish here. Is there a mis-understanding?) >> >> I wasn't talking about the sl28cpld btw. What is the difference >> between >> your proposed "change take place immediately" and "after the cycle". >> I understand how the after the cycle should work. But how would the >> immediate change work in your ideal PWM? > > If the PWM is running at 1/3 duty cycle and reconfigured for 2/3, then > the two scenarios are (the * marks the moment where pwm_apply_state() > is > called, ^ marks the start of a period): > > immediately: > > __ __ _____ _____ > / \_____/ \__/ \__/ > ^ ^ ^ ^ > * Ok lets assume 2/3 and change it to 1/3: ____ ______ __ / \___/ \____/ \____ ^ ^ ^ ^ * This will then have a longer on period than any of the settings. > and with my ideal PWM I can choose which of the two behaviours I want. Ahh, that I've missed. >> > > > > > What about disable()? >> > > > > >> > > > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can >> > > > > fix that (in hardware), not much we can do in the driver here. We are >> > > > > _very_ constraint in size, therefore all that little edge cases fall >> > > > > off >> > > > > the table. >> > > > >> > > > You're saying that on disable the hardware emits a constant high level >> > > > for one cycle? I hope not ... >> > > >> > > Mh, I was mistaken, disabling the PWM will turn it off immediately, >> > > but >> > >> > And does turn off mean, the output gets inactive? >> > If so you might also disable the hardware if a 0% duty cycle is >> > configured assuming this saves some energy without modifying the >> > resulting wave form. >> >> Disabling it has some side effects like switching to another function >> for this multi function pin. So I'd rather keep it on ;) > > So IMHO you should also keep it on when pwm_apply_state is called with > state.enabled = false to ensure a low output. That won't work either, because that is how you would turn on that multi function. Ie. it is GPIO (default input) as long as the PWM is not enabled, otherwise its PWM. -michael
Hello Michael, On Wed, Jul 15, 2020 at 10:41:25PM +0200, Michael Walle wrote: > Am 2020-07-15 20:18, schrieb Uwe Kleine-König: > > On Wed, Jul 15, 2020 at 07:45:10PM +0200, Michael Walle wrote: > > > > > > Am 2020-07-15 18:36, schrieb Uwe Kleine-König: > > > > On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote: > > > > > > My wishlist (just as it comes to my mind, so no guarantee of > > > > > > completeness): > > > > > > > > > > > > - can do 0% duty cycle for all supported period lengths > > > > > > - can do 100% duty cycle for all supported period lengths > > > > > > - supports both polarities > > > > > > - supports immediate change of configuration and after completion of > > > > > > the currently running period > > > > > > - atomic update (i.e. if you go from configuration A to configuration B > > > > > > the hardware guarantees to only emit periods of type A and then type > > > > > > B. (Depending on the item above, the last A period might be cut off.) > > > > > > > > > > We actually discussed this, because the implementation would be > > > > > easier. But > > > > > if the change takes place immediately you might end up with a longer > > > > > duty > > > > > cycle. Assume the PWM runs at 80% duty cycle and starts with the > > > > > on-period. > > > > > If you now change that to 50% you might end up with one successive > > > > > duty > > > > > cycle of "130%". Eg. the 80% of the old and right after that you > > > > > switch to > > > > > the new 50% and then you'd have a high output which corresponds to a > > > > > 130% > > > > > cycle. I don't know if that is acceptable for all applications. > > > > > > > > I thought this is a "change takes place immediately" implementation?! So > > > > these problems are actually real here. (And this not happening is > > > > exactly > > > > my wish here. Is there a mis-understanding?) > > > > > > I wasn't talking about the sl28cpld btw. What is the difference > > > between > > > your proposed "change take place immediately" and "after the cycle". > > > I understand how the after the cycle should work. But how would the > > > immediate change work in your ideal PWM? > > > > If the PWM is running at 1/3 duty cycle and reconfigured for 2/3, then > > the two scenarios are (the * marks the moment where pwm_apply_state() is > > called, ^ marks the start of a period): > > > > immediately: > > > > __ __ _____ _____ > > / \_____/ \__/ \__/ > > ^ ^ ^ ^ > > * > > Ok lets assume 2/3 and change it to 1/3: > > ____ ______ __ > / \___/ \____/ \____ > ^ ^ ^ ^ > * > This will then have a longer on period than any of the settings. I think we agree here. With an immediate change to the new setting both too long and too short signals can heppen. How bad this is depends on the use. The consumers currently in the kernel probably don't care too much. > > > > > > > > What about disable()? > > > > > > > > > > > > > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can > > > > > > > fix that (in hardware), not much we can do in the driver here. We are > > > > > > > _very_ constraint in size, therefore all that little edge cases fall > > > > > > > off > > > > > > > the table. > > > > > > > > > > > > You're saying that on disable the hardware emits a constant high level > > > > > > for one cycle? I hope not ... > > > > > > > > > > Mh, I was mistaken, disabling the PWM will turn it off immediately, > > > > > but > > > > > > > > And does turn off mean, the output gets inactive? > > > > If so you might also disable the hardware if a 0% duty cycle is > > > > configured assuming this saves some energy without modifying the > > > > resulting wave form. > > > > > > Disabling it has some side effects like switching to another function > > > for this multi function pin. So I'd rather keep it on ;) > > > > So IMHO you should also keep it on when pwm_apply_state is called with > > state.enabled = false to ensure a low output. > > That won't work either, because that is how you would turn on that multi > function. Ie. it is GPIO (default input) as long as the PWM is not enabled, > otherwise its PWM. I think you misunderstood what I wrote. The intended behaviour for a disabled PWM (as in: pwm_apply_state() was called with state.enabled = false) is that the output is a constant low (assuming a normal polarity). If disabling your hardware results in something else, don't disable the hardware. That's another item in the Limitations paragraph. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 7dbcf6973d33..a0d50d70c3b9 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -428,6 +428,16 @@ config PWM_SIFIVE To compile this driver as a module, choose M here: the module will be called pwm-sifive. +config PWM_SL28CPLD + tristate "Kontron sl28cpld PWM support" + select MFD_SIMPLE_MFD_I2C + help + Generic PWM framework driver for board management controller + found on the Kontron sl28 CPLD. + + To compile this driver as a module, choose M here: the module + will be called pwm-sl28cpld. + config PWM_SPEAR tristate "STMicroelectronics SPEAr PWM support" depends on PLAT_SPEAR || COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 2c2ba0a03557..cbdcd55d69ee 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -40,6 +40,7 @@ obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o +obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o obj-$(CONFIG_PWM_STI) += pwm-sti.o diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c new file mode 100644 index 000000000000..8ee286b605bf --- /dev/null +++ b/drivers/pwm/pwm-sl28cpld.c @@ -0,0 +1,187 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * sl28cpld PWM driver + * + * Copyright 2020 Kontron Europe GmbH + */ + +#include <linux/bitfield.h> +#include <linux/kernel.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/regmap.h> + +/* + * PWM timer block registers. + */ +#define PWM_CTRL 0x00 +#define PWM_ENABLE BIT(7) +#define PWM_MODE_250HZ 0 +#define PWM_MODE_500HZ 1 +#define PWM_MODE_1KHZ 2 +#define PWM_MODE_2KHZ 3 +#define PWM_MODE_MASK GENMASK(1, 0) +#define PWM_CYCLE 0x01 +#define PWM_CYCLE_MAX 0x7f + +struct sl28cpld_pwm { + struct pwm_chip pwm_chip; + struct regmap *regmap; + u32 offset; +}; + +struct sl28cpld_pwm_periods { + u8 ctrl; + unsigned long duty_cycle; +}; + +struct sl28cpld_pwm_config { + unsigned long period_ns; + u8 max_duty_cycle; +}; + +static struct sl28cpld_pwm_config sl28cpld_pwm_config[] = { + [PWM_MODE_250HZ] = { .period_ns = 4000000, .max_duty_cycle = 0x80 }, + [PWM_MODE_500HZ] = { .period_ns = 2000000, .max_duty_cycle = 0x40 }, + [PWM_MODE_1KHZ] = { .period_ns = 1000000, .max_duty_cycle = 0x20 }, + [PWM_MODE_2KHZ] = { .period_ns = 500000, .max_duty_cycle = 0x10 }, +}; + +static void sl28cpld_pwm_get_state(struct pwm_chip *chip, + struct pwm_device *pwm, + struct pwm_state *state) +{ + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); + static struct sl28cpld_pwm_config *config; + unsigned int reg; + unsigned int mode; + + regmap_read(priv->regmap, priv->offset + PWM_CTRL, ®); + + state->enabled = reg & PWM_ENABLE; + + mode = FIELD_GET(PWM_MODE_MASK, reg); + config = &sl28cpld_pwm_config[mode]; + state->period = config->period_ns; + + regmap_read(priv->regmap, priv->offset + PWM_CYCLE, ®); + pwm_set_relative_duty_cycle(state, reg, config->max_duty_cycle); +} + +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); + struct sl28cpld_pwm_config *config; + unsigned int cycle; + int ret; + int mode; + u8 ctrl; + + /* Get the configuration by comparing the period */ + for (mode = 0; mode < ARRAY_SIZE(sl28cpld_pwm_config); mode++) { + config = &sl28cpld_pwm_config[mode]; + if (state->period == config->period_ns) + break; + } + + if (mode == ARRAY_SIZE(sl28cpld_pwm_config)) + return -EINVAL; + + ctrl = FIELD_PREP(PWM_MODE_MASK, mode); + if (state->enabled) + ctrl |= PWM_ENABLE; + + cycle = pwm_get_relative_duty_cycle(state, config->max_duty_cycle); + + /* + * The hardware doesn't allow to set max_duty_cycle if the + * 250Hz mode is enabled, thus we have to trap that here. + * But because a 100% duty cycle is equal on all modes, i.e. + * it is just a "all-high" output, we trap any case with a + * 100% duty cycle and use the 500Hz mode. + */ + if (cycle == config->max_duty_cycle) { + ctrl &= ~PWM_MODE_MASK; + ctrl |= FIELD_PREP(PWM_MODE_MASK, PWM_MODE_500HZ); + cycle = PWM_CYCLE_MAX; + } + + ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl); + if (ret) + return ret; + + return regmap_write(priv->regmap, priv->offset + PWM_CYCLE, (u8)cycle); +} + +static const struct pwm_ops sl28cpld_pwm_ops = { + .apply = sl28cpld_pwm_apply, + .get_state = sl28cpld_pwm_get_state, + .owner = THIS_MODULE, +}; + +static int sl28cpld_pwm_probe(struct platform_device *pdev) +{ + struct sl28cpld_pwm *priv; + struct pwm_chip *chip; + int ret; + + if (!pdev->dev.parent) + return -ENODEV; + + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + priv->regmap = dev_get_regmap(pdev->dev.parent, NULL); + if (!priv->regmap) + return -ENODEV; + + ret = device_property_read_u32(&pdev->dev, "reg", &priv->offset); + if (ret) + return -EINVAL; + + /* Initialize the pwm_chip structure */ + chip = &priv->pwm_chip; + chip->dev = &pdev->dev; + chip->ops = &sl28cpld_pwm_ops; + chip->base = -1; + chip->npwm = 1; + + ret = pwmchip_add(&priv->pwm_chip); + if (ret) + return ret; + + platform_set_drvdata(pdev, priv); + + return 0; +} + +static int sl28cpld_pwm_remove(struct platform_device *pdev) +{ + struct sl28cpld_pwm *priv = platform_get_drvdata(pdev); + + return pwmchip_remove(&priv->pwm_chip); +} + +static const struct of_device_id sl28cpld_pwm_of_match[] = { + { .compatible = "kontron,sl28cpld-pwm" }, + {} +}; +MODULE_DEVICE_TABLE(of, sl28cpld_pwm_of_match); + +static struct platform_driver sl28cpld_pwm_driver = { + .probe = sl28cpld_pwm_probe, + .remove = sl28cpld_pwm_remove, + .driver = { + .name = "sl28cpld-pwm", + .of_match_table = sl28cpld_pwm_of_match, + }, +}; +module_platform_driver(sl28cpld_pwm_driver); + +MODULE_DESCRIPTION("sl28cpld PWM Driver"); +MODULE_AUTHOR("Michael Walle <michael@walle.cc>"); +MODULE_LICENSE("GPL");
Add support for the PWM controller of the sl28cpld board management controller. This is part of a multi-function device driver. The controller has one PWM channel and can just generate four distinct frequencies. Signed-off-by: Michael Walle <michael@walle.cc> --- Changes since v4: - update copyright year - remove #include <linux/of_device.h>, suggested by Andy. - make the pwm mode table look nicer, suggested by Lee. - use dev_get_drvdata(chip->dev) instead of container_of(), suggested by Lee. - use whole sentence in comments, suggested by Lee. - renamed the local "struct sl28cpld_pwm" variable to "priv" everywhere, suggested by Lee. - use pwm_{get,set}_relative_duty_cycle(), suggested by Andy. - make the comment about the 250Hz hardware limitation clearer - don't use "if (ret < 0)", but only "if (ret)", suggested by Andy. - don't use KBUID_MODNAME - remove comma in terminator line of the compatible strings list - remove the platform device table Changes since v3: - see cover letter drivers/pwm/Kconfig | 10 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sl28cpld.c | 187 +++++++++++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+) create mode 100644 drivers/pwm/pwm-sl28cpld.c