| Message ID | 20200725231834.25642-7-michael@walle.cc |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
On Sun, Jul 26, 2020 at 01:18:27AM +0200, Michael Walle wrote: > 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 v5: > - added brief description of the PWM hardware implementation > - added hardware limitations > - dropped the frequency mode table, instead calculate the prescaler > value on the fly. > - round the requested parameters instead of support just distinct > periods. > - prefix the macros by SL28CPLD_ to make them less generic > - set polarity to PWM_POLARITY_NORMAL and reject inverted polarity > requests. > - apply the workaround just for prescaler value of 0. > - make errors during probing more verbose > > 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 | 223 +++++++++++++++++++++++++++++++++++++ > 3 files changed, 234 insertions(+) > create mode 100644 drivers/pwm/pwm-sl28cpld.c > > 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..956fa09f3aba > --- /dev/null > +++ b/drivers/pwm/pwm-sl28cpld.c > @@ -0,0 +1,223 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * sl28cpld PWM driver > + * > + * Copyright (c) 2020 Michael Walle <michael@walle.cc> > + * > + * There is no public datasheet available for this PWM core. But it is easy > + * enough to be briefly explained. It consists of one 8-bit counter. The PWM > + * supports four distinct frequencies by selecting when to reset the counter. > + * With the prescaler setting you can select which bit of the counter is used > + * to reset it. This implies that the higher the frequency the less remaining > + * bits are available for the actual counter. > + * > + * Let cnt[7:0] be the counter, clocked at 32kHz: > + * +-----------+--------+--------------+-----------+ > + * | prescaler | reset | counter bits | frequency | > + * +-----------+--------+--------------+-----------+ > + * | 0 | cnt[7] | cnt[6:0] | 250Hz | > + * | 1 | cnt[6] | cnt[5:0] | 500Hz | > + * | 2 | cnt[5] | cnt[4:0] | 1kHz | > + * | 3 | cnt[4] | cnt[3:0] | 2kHz | > + * +-----------+--------+--------------+-----------+ > + * > + * Limitations: > + * - The hardware cannot generate a 100% duty cycle if the prescaler is 0. > + * - The hardware cannot atomically set the prescaler and the counter value, > + * which might lead to glitches and inconsistent states if a write fails. > + * - The counter is not reset if you switch the prescaler which leads > + * to glitches, too. > + * - The duty cycle will switch immediately and not after a complete cycle. > + * - Depending on the actual implementation, disabling the PWM might have > + * side effects. For example, if the output pin is shared with a GPIO pin > + * it will automatically switch back to GPIO mode. > + */ > + > +#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 SL28CPLD_PWM_CTRL 0x00 > +#define SL28CPLD_PWM_CTRL_ENABLE BIT(7) > +#define SL28CPLD_PWM_CTRL_PRESCALER_MASK GENMASK(1, 0) > +#define SL28CPLD_PWM_CYCLE 0x01 > +#define SL28CPLD_PWM_CYCLE_MAX GENMASK(6, 0) > + > +#define SL28CPLD_PWM_CLK 32000 /* 32 kHz */ > +#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler) (1 << (7 - (prescaler))) > +#define SL28CPLD_PWM_PERIOD(prescaler) \ > + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)) > + > +/* > + * We calculate the duty cycle like this: > + * duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle > + * > + * With > + * max_period_ns = (1 << 7 - prescaler) / pwm_clk * NSEC_PER_SEC > + * max_duty_cycle = 1 << (7 - prescaler) > + * this then simplifies to: > + * duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC > + */ > +#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \ > + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg)) > +#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \ > + (DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK)) > + > +struct sl28cpld_pwm { > + struct pwm_chip pwm_chip; > + struct regmap *regmap; > + u32 offset; > +}; > + > +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); > + unsigned int reg; > + int prescaler; > + > + regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ®); > + > + state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE; > + > + prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg); > + state->period = SL28CPLD_PWM_PERIOD(prescaler); > + > + regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, ®); > + state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg); > + state->polarity = PWM_POLARITY_NORMAL; > +} > + > +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); > + unsigned int cycle, prescaler; > + int ret; > + u8 ctrl; > + > + /* Polarity inversion is not supported */ > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; Just a note to myself since this just occurred to me: in the legacy API we used to have a ->set_polarity() callback that indicated whether or not a controller supports inversion. Since that criterion can no longer be used with the atomic API we may want to consider adding some sort of capability flags so that these checks can be performed in the core. > + > + /* > + * Calculate the prescaler. Pick the the biggest period that isn't > + * bigger than the requested period. > + */ > + prescaler = DIV_ROUND_UP_ULL(SL28CPLD_PWM_PERIOD(0), state->period); > + prescaler = order_base_2(prescaler); > + > + if (prescaler > field_max(SL28CPLD_PWM_CTRL_PRESCALER_MASK)) > + return -ERANGE; > + > + ctrl = FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, prescaler); > + if (state->enabled) > + ctrl |= SL28CPLD_PWM_CTRL_ENABLE; > + > + cycle = SL28CPLD_PWM_FROM_DUTY_CYCLE(state->duty_cycle); > + cycle = min_t(unsigned int, cycle, SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)); > + > + /* > + * Work around the hardware limitation. See also above. Trap 100% duty > + * cycle if the prescaler is 0. Set prescaler to 1 instead. We don't > + * care about the frequency because its "all-one" in either case. > + * > + * We don't need to check the actual prescaler setting, because only > + * if the prescaler is 0 we can have this particular value. > + */ > + if (cycle == SL28CPLD_PWM_MAX_DUTY_CYCLE(0)) { > + ctrl &= ~SL28CPLD_PWM_CTRL_PRESCALER_MASK; > + ctrl |= FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, 1); > + cycle = SL28CPLD_PWM_MAX_DUTY_CYCLE(1); > + } > + > + ret = regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ctrl); > + if (ret) > + return ret; > + > + return regmap_write(priv->regmap, priv->offset + SL28CPLD_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) { > + dev_err(&pdev->dev, "no 'reg' property found (%pe)\n", > + ERR_PTR(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) { > + dev_err(&pdev->dev, "failed to add PWM chip (%pe)", > + ERR_PTR(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"); Looks good to me. I assume Lee will want to merge this along with the other changes through the MFD tree? If so: Acked-by: Thierry Reding <thierry.reding@gmail.com>
[stripping recipents as most probably won't be interested in this] Hello Thierry, On Mon, Jul 27, 2020 at 09:30:27AM +0200, Thierry Reding wrote: > On Sun, Jul 26, 2020 at 01:18:27AM +0200, Michael Walle wrote: > > + /* Polarity inversion is not supported */ > > + if (state->polarity != PWM_POLARITY_NORMAL) > > + return -EINVAL; > > Just a note to myself since this just occurred to me: in the legacy API > we used to have a ->set_polarity() callback that indicated whether or > not a controller supports inversion. Since that criterion can no longer > be used with the atomic API we may want to consider adding some sort of > capability flags so that these checks can be performed in the core. I'm not (yet?) convinced this is a good idea because these concepts are somewhat blurry. For example a controller that only supports normal polarity (probably) still can generate the wave corresponding to: .enabled = true, .polarity = PWM_POLARITY_INVERSED, .duty_cycle = 0, .period = 5000, and unless we refuse this request at the core level (should we?) the driver must be aware of .polarity anyhow. Ditto for .enabled = false. > Looks good to me. I assume Lee will want to merge this along with the > other changes through the MFD tree? If so: I'd like to have another go through the patch. Will do so (hopefully) later today. So please don't apply yet. Best regards Uwe
Hello, just a few minor issues left: On Sun, Jul 26, 2020 at 01:18:27AM +0200, Michael Walle wrote: > diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c > new file mode 100644 > index 000000000000..956fa09f3aba > --- /dev/null > +++ b/drivers/pwm/pwm-sl28cpld.c > @@ -0,0 +1,223 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * sl28cpld PWM driver > + * > + * Copyright (c) 2020 Michael Walle <michael@walle.cc> > + * > + * There is no public datasheet available for this PWM core. But it is easy > + * enough to be briefly explained. It consists of one 8-bit counter. The PWM > + * supports four distinct frequencies by selecting when to reset the counter. > + * With the prescaler setting you can select which bit of the counter is used > + * to reset it. This implies that the higher the frequency the less remaining > + * bits are available for the actual counter. > + * > + * Let cnt[7:0] be the counter, clocked at 32kHz: > + * +-----------+--------+--------------+-----------+ > + * | prescaler | reset | counter bits | frequency | > + * +-----------+--------+--------------+-----------+ > + * | 0 | cnt[7] | cnt[6:0] | 250Hz | > + * | 1 | cnt[6] | cnt[5:0] | 500Hz | > + * | 2 | cnt[5] | cnt[4:0] | 1kHz | > + * | 3 | cnt[4] | cnt[3:0] | 2kHz | > + * +-----------+--------+--------------+-----------+ Very nice. I'd add a "period length" column, as this is what the PWM core uses. For your convenience (and as I created that table anyhow for further checking of the formulas below): * +-----------+--------+--------------+-----------+--------+ * | prescaler | reset | counter bits | frequency | period | * | | | | | length | * +-----------+--------+--------------+-----------+--------+ * | 0 | cnt[7] | cnt[6:0] | 250Hz | 4000ns | * | 1 | cnt[6] | cnt[5:0] | 500Hz | 2000ns | * | 2 | cnt[5] | cnt[4:0] | 1kHz | 1000ns | * | 3 | cnt[4] | cnt[3:0] | 2kHz | 500ns | * +-----------+--------+--------------+-----------+--------+ > + * > + * Limitations: > + * - The hardware cannot generate a 100% duty cycle if the prescaler is 0. > + * - The hardware cannot atomically set the prescaler and the counter value, > + * which might lead to glitches and inconsistent states if a write fails. > + * - The counter is not reset if you switch the prescaler which leads > + * to glitches, too. > + * - The duty cycle will switch immediately and not after a complete cycle. > + * - Depending on the actual implementation, disabling the PWM might have > + * side effects. For example, if the output pin is shared with a GPIO pin > + * it will automatically switch back to GPIO mode. > + */ > + > +#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 SL28CPLD_PWM_CTRL 0x00 > +#define SL28CPLD_PWM_CTRL_ENABLE BIT(7) > +#define SL28CPLD_PWM_CTRL_PRESCALER_MASK GENMASK(1, 0) > +#define SL28CPLD_PWM_CYCLE 0x01 > +#define SL28CPLD_PWM_CYCLE_MAX GENMASK(6, 0) > + > +#define SL28CPLD_PWM_CLK 32000 /* 32 kHz */ > +#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler) (1 << (7 - (prescaler))) > +#define SL28CPLD_PWM_PERIOD(prescaler) \ > + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)) > + > +/* > + * We calculate the duty cycle like this: > + * duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle > + * > + * With > + * max_period_ns = (1 << 7 - prescaler) / pwm_clk * NSEC_PER_SEC > + * max_duty_cycle = 1 << (7 - prescaler) If you don't need parenthesis in the max_period_ns around 7 - prescaler, you don't need them either in the max_duty_cycle line. > + * this then simplifies to: > + * duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC > + */ > +#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \ > + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg)) For those who copy from your driver maybe add a comment like: * NSEC_PER_SEC / SL28CPLD_PWM_CLK is integer here, so we're not loosing * precision by doing the division first. > +#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \ > + (DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK)) > + > +struct sl28cpld_pwm { > + struct pwm_chip pwm_chip; > + struct regmap *regmap; > + u32 offset; > +}; > + > +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); > + unsigned int reg; > + int prescaler; > + > + regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ®); Would it make sense to hide this using e.g.: #define sl28cpkd_pwm_read(priv, reg, val) regmap_read((priv)->regmap, (priv)->offset + (reg), val) The line would then become: sl28cpkd_pwm_read(priv, SL28CPLD_PWM_CTRL, ®); which is a bit prettier. Up to you to decide. If you do it, please do the same for write > + state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE; > + > + prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg); > + state->period = SL28CPLD_PWM_PERIOD(prescaler); > + > + regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, ®); > + state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg); > + state->polarity = PWM_POLARITY_NORMAL; > +} > + > +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); > + unsigned int cycle, prescaler; > + int ret; > + u8 ctrl; > + > + /* Polarity inversion is not supported */ > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EINVAL; > + > + /* > + * Calculate the prescaler. Pick the the biggest period that isn't > + * bigger than the requested period. > + */ > + prescaler = DIV_ROUND_UP_ULL(SL28CPLD_PWM_PERIOD(0), state->period); > + prescaler = order_base_2(prescaler); > + > + if (prescaler > field_max(SL28CPLD_PWM_CTRL_PRESCALER_MASK)) > + return -ERANGE; The calculation looks right. Did you check the generated code? Maybe using an if or switch here is more effective? (optional task for bonus points :-) > + ctrl = FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, prescaler); > + if (state->enabled) > + ctrl |= SL28CPLD_PWM_CTRL_ENABLE; > + > + cycle = SL28CPLD_PWM_FROM_DUTY_CYCLE(state->duty_cycle); > + cycle = min_t(unsigned int, cycle, SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)); > + > + /* > + * Work around the hardware limitation. See also above. Trap 100% duty > + * cycle if the prescaler is 0. Set prescaler to 1 instead. We don't > + * care about the frequency because its "all-one" in either case. > + * > + * We don't need to check the actual prescaler setting, because only > + * if the prescaler is 0 we can have this particular value. > + */ > + if (cycle == SL28CPLD_PWM_MAX_DUTY_CYCLE(0)) { > + ctrl &= ~SL28CPLD_PWM_CTRL_PRESCALER_MASK; > + ctrl |= FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, 1); > + cycle = SL28CPLD_PWM_MAX_DUTY_CYCLE(1); > + } > + > + ret = regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ctrl); > + if (ret) > + return ret; > + > + return regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, (u8)cycle); This cast isn't needed, is it? > +} > + > +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) Error message here? > + return -ENODEV; > + > + ret = device_property_read_u32(&pdev->dev, "reg", &priv->offset); > + if (ret) { > + dev_err(&pdev->dev, "no 'reg' property found (%pe)\n", > + ERR_PTR(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) { > + dev_err(&pdev->dev, "failed to add PWM chip (%pe)", > + ERR_PTR(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"); Thanks Uwe
Hi, Am 2020-07-28 09:43, schrieb Uwe Kleine-König: > Hello, > > just a few minor issues left: thanks for the review. > > On Sun, Jul 26, 2020 at 01:18:27AM +0200, Michael Walle wrote: >> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c >> new file mode 100644 >> index 000000000000..956fa09f3aba >> --- /dev/null >> +++ b/drivers/pwm/pwm-sl28cpld.c >> @@ -0,0 +1,223 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * sl28cpld PWM driver >> + * >> + * Copyright (c) 2020 Michael Walle <michael@walle.cc> >> + * >> + * There is no public datasheet available for this PWM core. But it >> is easy >> + * enough to be briefly explained. It consists of one 8-bit counter. >> The PWM >> + * supports four distinct frequencies by selecting when to reset the >> counter. >> + * With the prescaler setting you can select which bit of the counter >> is used >> + * to reset it. This implies that the higher the frequency the less >> remaining >> + * bits are available for the actual counter. >> + * >> + * Let cnt[7:0] be the counter, clocked at 32kHz: >> + * +-----------+--------+--------------+-----------+ >> + * | prescaler | reset | counter bits | frequency | >> + * +-----------+--------+--------------+-----------+ >> + * | 0 | cnt[7] | cnt[6:0] | 250Hz | >> + * | 1 | cnt[6] | cnt[5:0] | 500Hz | >> + * | 2 | cnt[5] | cnt[4:0] | 1kHz | >> + * | 3 | cnt[4] | cnt[3:0] | 2kHz | >> + * +-----------+--------+--------------+-----------+ > > Very nice. I'd add a "period length" column, as this is what the PWM > core uses. > > For your convenience (and as I created that table anyhow for further > checking of the formulas below): > > * +-----------+--------+--------------+-----------+--------+ > * | prescaler | reset | counter bits | frequency | period | > * | | | | | length | > * +-----------+--------+--------------+-----------+--------+ > * | 0 | cnt[7] | cnt[6:0] | 250Hz | 4000ns | > * | 1 | cnt[6] | cnt[5:0] | 500Hz | 2000ns | > * | 2 | cnt[5] | cnt[4:0] | 1kHz | 1000ns | > * | 3 | cnt[4] | cnt[3:0] | 2kHz | 500ns | > * +-----------+--------+--------------+-----------+--------+ sure :) > >> + * >> + * Limitations: >> + * - The hardware cannot generate a 100% duty cycle if the prescaler >> is 0. >> + * - The hardware cannot atomically set the prescaler and the counter >> value, >> + * which might lead to glitches and inconsistent states if a write >> fails. >> + * - The counter is not reset if you switch the prescaler which leads >> + * to glitches, too. >> + * - The duty cycle will switch immediately and not after a complete >> cycle. >> + * - Depending on the actual implementation, disabling the PWM might >> have >> + * side effects. For example, if the output pin is shared with a >> GPIO pin >> + * it will automatically switch back to GPIO mode. >> + */ >> + >> +#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 SL28CPLD_PWM_CTRL 0x00 >> +#define SL28CPLD_PWM_CTRL_ENABLE BIT(7) >> +#define SL28CPLD_PWM_CTRL_PRESCALER_MASK GENMASK(1, 0) >> +#define SL28CPLD_PWM_CYCLE 0x01 >> +#define SL28CPLD_PWM_CYCLE_MAX GENMASK(6, 0) >> + >> +#define SL28CPLD_PWM_CLK 32000 /* 32 kHz */ >> +#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler) (1 << (7 - >> (prescaler))) >> +#define SL28CPLD_PWM_PERIOD(prescaler) \ >> + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * >> SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)) >> + >> +/* >> + * We calculate the duty cycle like this: >> + * duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle >> + * >> + * With >> + * max_period_ns = (1 << 7 - prescaler) / pwm_clk * NSEC_PER_SEC >> + * max_duty_cycle = 1 << (7 - prescaler) > > If you don't need parenthesis in the max_period_ns around 7 - > prescaler, > you don't need them either in the max_duty_cycle line. mhh this should be "1 << (7 - prescaler)" in both cases. So max_period_ns is wrong: max_period_ns = 1 << (7 - prescaler) / pwm_clk * NSEC_PER_SEC >> + * this then simplifies to: >> + * duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC >> + */ >> +#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \ >> + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg)) > > For those who copy from your driver maybe add a comment like: > > * NSEC_PER_SEC / SL28CPLD_PWM_CLK is integer here, so we're not > loosing > * precision by doing the division first. ok. >> +#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \ >> + (DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK)) >> + >> +struct sl28cpld_pwm { >> + struct pwm_chip pwm_chip; >> + struct regmap *regmap; >> + u32 offset; >> +}; >> + >> +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); >> + unsigned int reg; >> + int prescaler; >> + >> + regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ®); > > Would it make sense to hide this using e.g.: > > #define sl28cpkd_pwm_read(priv, reg, val) regmap_read((priv)->regmap, > (priv)->offset + (reg), val) > > The line would then become: > > sl28cpkd_pwm_read(priv, SL28CPLD_PWM_CTRL, ®); > > which is a bit prettier. Up to you to decide. If you do it, please do > the same for write I don't have a strong opinion on that. I can change it. Although there will be checkpatch warning about multiple uses of the macro argument, I'd presume. >> + state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE; >> + >> + prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg); >> + state->period = SL28CPLD_PWM_PERIOD(prescaler); >> + >> + regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, ®); >> + state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg); >> + state->polarity = PWM_POLARITY_NORMAL; >> +} >> + >> +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); >> + unsigned int cycle, prescaler; >> + int ret; >> + u8 ctrl; >> + >> + /* Polarity inversion is not supported */ >> + if (state->polarity != PWM_POLARITY_NORMAL) >> + return -EINVAL; >> + >> + /* >> + * Calculate the prescaler. Pick the the biggest period that isn't >> + * bigger than the requested period. >> + */ >> + prescaler = DIV_ROUND_UP_ULL(SL28CPLD_PWM_PERIOD(0), state->period); >> + prescaler = order_base_2(prescaler); >> + >> + if (prescaler > field_max(SL28CPLD_PWM_CTRL_PRESCALER_MASK)) >> + return -ERANGE; > > The calculation looks right. > Did you check the generated code? Maybe using an if or switch here is > more effective? (optional task for bonus points :-) I varied between this and some if/switch. This hard to read IMHO (as was your your ilog(n+1)+1), but you could easily change the range of the prescaler without having to change this. Also if/switch looked ugly too *g*. I'll check again. > >> + ctrl = FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, prescaler); >> + if (state->enabled) >> + ctrl |= SL28CPLD_PWM_CTRL_ENABLE; >> + >> + cycle = SL28CPLD_PWM_FROM_DUTY_CYCLE(state->duty_cycle); >> + cycle = min_t(unsigned int, cycle, >> SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)); >> + >> + /* >> + * Work around the hardware limitation. See also above. Trap 100% >> duty >> + * cycle if the prescaler is 0. Set prescaler to 1 instead. We don't >> + * care about the frequency because its "all-one" in either case. >> + * >> + * We don't need to check the actual prescaler setting, because only >> + * if the prescaler is 0 we can have this particular value. >> + */ >> + if (cycle == SL28CPLD_PWM_MAX_DUTY_CYCLE(0)) { >> + ctrl &= ~SL28CPLD_PWM_CTRL_PRESCALER_MASK; >> + ctrl |= FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, 1); >> + cycle = SL28CPLD_PWM_MAX_DUTY_CYCLE(1); >> + } >> + >> + ret = regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, >> ctrl); >> + if (ret) >> + return ret; >> + >> + return regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, >> (u8)cycle); > > This cast isn't needed, is it? Due to the clamping, it is not, correct. I'll remove it. >> +} >> + >> +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) > > Error message here? This shouldn't really happen and I put it into the same category as the two above and report no error. But I can add it. Generally, it looked to me that more and more drivers don't really report errors anymore, but just return with an -EWHATEVER. So if someone can shed some light here, I'm all ears. -michael
Hallo, On Tue, Jul 28, 2020 at 10:21:22AM +0200, Michael Walle wrote: > Am 2020-07-28 09:43, schrieb Uwe Kleine-König: > > On Sun, Jul 26, 2020 at 01:18:27AM +0200, Michael Walle wrote: > > > +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) > > > > Error message here? > > This shouldn't really happen and I put it into the same category > as the two above and report no error. But I can add it. For kzalloc it is right to not emit an error because a failing kzalloc is already loud on its own. I missed the first error path, that should get a message, too. > Generally, it looked to me that more and more drivers don't > really report errors anymore, but just return with an -EWHATEVER. > So if someone can shed some light here, I'm all ears. IMHO it's wrong not to add error messages. At one point in time it will fail and then you're happy if you don't have to add printks all over the place first to debug that. Best regards Uwe
On Tue, 28 Jul 2020, Uwe Kleine-König wrote: > On Tue, Jul 28, 2020 at 10:21:22AM +0200, Michael Walle wrote: > > Am 2020-07-28 09:43, schrieb Uwe Kleine-König: > > > On Sun, Jul 26, 2020 at 01:18:27AM +0200, Michael Walle wrote: > > > > +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) > > > > > > Error message here? > > > > This shouldn't really happen and I put it into the same category > > as the two above and report no error. But I can add it. > > For kzalloc it is right to not emit an error because a failing kzalloc > is already loud on its own. I missed the first error path, that should > get a message, too. > > > Generally, it looked to me that more and more drivers don't > > really report errors anymore, but just return with an -EWHATEVER. > > So if someone can shed some light here, I'm all ears. > > IMHO it's wrong not to add error messages. At one point in time it will > fail and then you're happy if you don't have to add printks all over the > place first to debug that. Error messages should only be omitted for -ENOMEM and if something is already being printed out, ideally by the sub-system API.
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..956fa09f3aba --- /dev/null +++ b/drivers/pwm/pwm-sl28cpld.c @@ -0,0 +1,223 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * sl28cpld PWM driver + * + * Copyright (c) 2020 Michael Walle <michael@walle.cc> + * + * There is no public datasheet available for this PWM core. But it is easy + * enough to be briefly explained. It consists of one 8-bit counter. The PWM + * supports four distinct frequencies by selecting when to reset the counter. + * With the prescaler setting you can select which bit of the counter is used + * to reset it. This implies that the higher the frequency the less remaining + * bits are available for the actual counter. + * + * Let cnt[7:0] be the counter, clocked at 32kHz: + * +-----------+--------+--------------+-----------+ + * | prescaler | reset | counter bits | frequency | + * +-----------+--------+--------------+-----------+ + * | 0 | cnt[7] | cnt[6:0] | 250Hz | + * | 1 | cnt[6] | cnt[5:0] | 500Hz | + * | 2 | cnt[5] | cnt[4:0] | 1kHz | + * | 3 | cnt[4] | cnt[3:0] | 2kHz | + * +-----------+--------+--------------+-----------+ + * + * Limitations: + * - The hardware cannot generate a 100% duty cycle if the prescaler is 0. + * - The hardware cannot atomically set the prescaler and the counter value, + * which might lead to glitches and inconsistent states if a write fails. + * - The counter is not reset if you switch the prescaler which leads + * to glitches, too. + * - The duty cycle will switch immediately and not after a complete cycle. + * - Depending on the actual implementation, disabling the PWM might have + * side effects. For example, if the output pin is shared with a GPIO pin + * it will automatically switch back to GPIO mode. + */ + +#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 SL28CPLD_PWM_CTRL 0x00 +#define SL28CPLD_PWM_CTRL_ENABLE BIT(7) +#define SL28CPLD_PWM_CTRL_PRESCALER_MASK GENMASK(1, 0) +#define SL28CPLD_PWM_CYCLE 0x01 +#define SL28CPLD_PWM_CYCLE_MAX GENMASK(6, 0) + +#define SL28CPLD_PWM_CLK 32000 /* 32 kHz */ +#define SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler) (1 << (7 - (prescaler))) +#define SL28CPLD_PWM_PERIOD(prescaler) \ + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)) + +/* + * We calculate the duty cycle like this: + * duty_cycle_ns = pwm_cycle_reg * max_period_ns / max_duty_cycle + * + * With + * max_period_ns = (1 << 7 - prescaler) / pwm_clk * NSEC_PER_SEC + * max_duty_cycle = 1 << (7 - prescaler) + * this then simplifies to: + * duty_cycle_ns = pwm_cycle_reg / pwm_clk * NSEC_PER_SEC + */ +#define SL28CPLD_PWM_TO_DUTY_CYCLE(reg) \ + (NSEC_PER_SEC / SL28CPLD_PWM_CLK * (reg)) +#define SL28CPLD_PWM_FROM_DUTY_CYCLE(duty_cycle) \ + (DIV_ROUND_DOWN_ULL((duty_cycle), NSEC_PER_SEC / SL28CPLD_PWM_CLK)) + +struct sl28cpld_pwm { + struct pwm_chip pwm_chip; + struct regmap *regmap; + u32 offset; +}; + +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); + unsigned int reg; + int prescaler; + + regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ®); + + state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE; + + prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg); + state->period = SL28CPLD_PWM_PERIOD(prescaler); + + regmap_read(priv->regmap, priv->offset + SL28CPLD_PWM_CYCLE, ®); + state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg); + state->polarity = PWM_POLARITY_NORMAL; +} + +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); + unsigned int cycle, prescaler; + int ret; + u8 ctrl; + + /* Polarity inversion is not supported */ + if (state->polarity != PWM_POLARITY_NORMAL) + return -EINVAL; + + /* + * Calculate the prescaler. Pick the the biggest period that isn't + * bigger than the requested period. + */ + prescaler = DIV_ROUND_UP_ULL(SL28CPLD_PWM_PERIOD(0), state->period); + prescaler = order_base_2(prescaler); + + if (prescaler > field_max(SL28CPLD_PWM_CTRL_PRESCALER_MASK)) + return -ERANGE; + + ctrl = FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, prescaler); + if (state->enabled) + ctrl |= SL28CPLD_PWM_CTRL_ENABLE; + + cycle = SL28CPLD_PWM_FROM_DUTY_CYCLE(state->duty_cycle); + cycle = min_t(unsigned int, cycle, SL28CPLD_PWM_MAX_DUTY_CYCLE(prescaler)); + + /* + * Work around the hardware limitation. See also above. Trap 100% duty + * cycle if the prescaler is 0. Set prescaler to 1 instead. We don't + * care about the frequency because its "all-one" in either case. + * + * We don't need to check the actual prescaler setting, because only + * if the prescaler is 0 we can have this particular value. + */ + if (cycle == SL28CPLD_PWM_MAX_DUTY_CYCLE(0)) { + ctrl &= ~SL28CPLD_PWM_CTRL_PRESCALER_MASK; + ctrl |= FIELD_PREP(SL28CPLD_PWM_CTRL_PRESCALER_MASK, 1); + cycle = SL28CPLD_PWM_MAX_DUTY_CYCLE(1); + } + + ret = regmap_write(priv->regmap, priv->offset + SL28CPLD_PWM_CTRL, ctrl); + if (ret) + return ret; + + return regmap_write(priv->regmap, priv->offset + SL28CPLD_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) { + dev_err(&pdev->dev, "no 'reg' property found (%pe)\n", + ERR_PTR(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) { + dev_err(&pdev->dev, "failed to add PWM chip (%pe)", + ERR_PTR(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 v5: - added brief description of the PWM hardware implementation - added hardware limitations - dropped the frequency mode table, instead calculate the prescaler value on the fly. - round the requested parameters instead of support just distinct periods. - prefix the macros by SL28CPLD_ to make them less generic - set polarity to PWM_POLARITY_NORMAL and reject inverted polarity requests. - apply the workaround just for prescaler value of 0. - make errors during probing more verbose 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 | 223 +++++++++++++++++++++++++++++++++++++ 3 files changed, 234 insertions(+) create mode 100644 drivers/pwm/pwm-sl28cpld.c