diff mbox series

[v6,06/13] pwm: add support for sl28cpld PWM controller

Message ID 20200725231834.25642-7-michael@walle.cc (mailing list archive)
State Not Applicable
Headers show
Series Add support for Kontron sl28cpld | expand

Commit Message

Michael Walle July 25, 2020, 11:18 p.m. UTC 
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

Comments

Thierry Reding July 27, 2020, 7:30 a.m. UTC | #1 
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, &reg);
> +
> +	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, &reg);
> +	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>
Uwe Kleine-König July 28, 2020, 7:43 a.m. UTC | #2 
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, &reg);

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, &reg);

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, &reg);
> +	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
Michael Walle July 28, 2020, 8:21 a.m. UTC | #3 
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, &reg);
> 
> 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, &reg);
> 
> 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, &reg);
>> +	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
Uwe Kleine-König July 28, 2020, 9:47 a.m. UTC | #4 
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
Lee Jones July 28, 2020, 10:51 a.m. UTC | #5 
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 mbox series

Patch

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, &reg);
+
+	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, &reg);
+	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");