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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 11852 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]