Re: [PATCH RESEND v3 2/4] pwm: Add Apple PWM controller

[Uwe Kleine-König]

Hello,

On Mon, Nov 21, 2022 at 08:42:26PM +0300, Sasha Finkelstein wrote:
> Adds the Apple PWM controller driver.
> 
> Signed-off-by: Sasha Finkelstein <fnkl.kernel@xxxxxxxxx>
> Acked-by: Sven Peter <sven@xxxxxxxxxxxxx>
> ---
>  drivers/pwm/Kconfig     |  12 ++++
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-apple.c | 127 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 140 insertions(+)
>  create mode 100644 drivers/pwm/pwm-apple.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 60d13a949bc5..c3be11468414 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -51,6 +51,18 @@ config PWM_AB8500
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-ab8500.
>  
> +config PWM_APPLE
> +	tristate "Apple SoC PWM support"
> +	depends on ARCH_APPLE || COMPILE_TEST
> +	help
> +	  Generic PWM framework driver for PWM controller present on
> +	  Apple SoCs
> +
> +	  Say Y here if you have an ARM Apple laptop, otherwise say N
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-apple.
> +
>  config PWM_ATMEL
>  	tristate "Atmel PWM support"
>  	depends on ARCH_AT91 || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 7bf1a29f02b8..19899b912e00 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -2,6 +2,7 @@
>  obj-$(CONFIG_PWM)		+= core.o
>  obj-$(CONFIG_PWM_SYSFS)		+= sysfs.o
>  obj-$(CONFIG_PWM_AB8500)	+= pwm-ab8500.o
> +obj-$(CONFIG_PWM_APPLE)		+= pwm-apple.o
>  obj-$(CONFIG_PWM_ATMEL)		+= pwm-atmel.o
>  obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM)	+= pwm-atmel-hlcdc.o
>  obj-$(CONFIG_PWM_ATMEL_TCB)	+= pwm-atmel-tcb.o
> diff --git a/drivers/pwm/pwm-apple.c b/drivers/pwm/pwm-apple.c
> new file mode 100644
> index 000000000000..b0c3f86fd578
> --- /dev/null
> +++ b/drivers/pwm/pwm-apple.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0 OR MIT
> +/*
> + * Driver for the Apple SoC PWM controller
> + *
> + * Copyright The Asahi Linux Contributors
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pwm.h>
> +
> +#define PWM_CONTROL     0x00
> +#define PWM_ON_CYCLES   0x1c
> +#define PWM_OFF_CYCLES  0x18
> +
> +#define CTRL_ENABLE        BIT(0)
> +#define CTRL_MODE          BIT(2)
> +#define CTRL_UPDATE        BIT(5)
> +#define CTRL_TRIGGER       BIT(9)
> +#define CTRL_INVERT        BIT(10)
> +#define CTRL_OUTPUT_ENABLE BIT(14)

Please use a driver specific prefix on these defines to make it obvious
that they are driver specific.

> +struct apple_pwm {
> +	struct pwm_chip chip;
> +	void __iomem *base;
> +	u64 clkrate;
> +};
> +
> +static int apple_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   const struct pwm_state *state)
> +{
> +	struct apple_pwm *fpwm;
> +	u64 on_cycles, off_cycles;
> +
> +	fpwm = container_of(chip, struct apple_pwm, chip);

Please introduce a macro or static inline for that.

> +	if (state->enabled) {
> +		on_cycles = mul_u64_u64_div_u64(fpwm->clkrate,
> +						state->duty_cycle, NSEC_PER_SEC);

This might overflow for big values of clkrate and duty_cycle. The
usual approach is to check for clkrate <= NSEC_PER_SEC. See
pwm-lpc18xx-sct.c for an example.

> +		off_cycles = mul_u64_u64_div_u64(fpwm->clkrate,
> +						 state->period, NSEC_PER_SEC) - on_cycles;
> +		writel(on_cycles, fpwm->base + PWM_ON_CYCLES);

You're assuming on_cycles to fit into an u32 here. Please ensure that's
a valid claim.

> +		writel(off_cycles, fpwm->base + PWM_OFF_CYCLES);
> +		writel(CTRL_ENABLE | CTRL_OUTPUT_ENABLE | CTRL_UPDATE,
> +		       fpwm->base + PWM_CONTROL);

How does the hardware behave on updates? Are the register values
shadowed until PWM_CONTROL is written? Or until the next period starts?

Please document this at the top of the driver file, in the same format
as e.g. pwm-sl28cpld.c does. (The relevant section is called
"Limitations", actually "Hardware properties" would be a better name,
but please stick to the former for easier greppability.)

> +	} else {
> +		writel(0, fpwm->base + PWM_CONTROL);
> +	}
> +	return 0;
> +}
> +
> +static void apple_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   struct pwm_state *state)
> +{
> +	struct apple_pwm *fpwm;
> +	u32 on_cycles, off_cycles, ctrl;
> +
> +	fpwm = container_of(chip, struct apple_pwm, chip);
> +
> +	ctrl = readl(fpwm->base + PWM_CONTROL);
> +	on_cycles = readl(fpwm->base + PWM_ON_CYCLES);
> +	off_cycles = readl(fpwm->base + PWM_OFF_CYCLES);
> +
> +	state->enabled = (ctrl & CTRL_ENABLE) && (ctrl & CTRL_OUTPUT_ENABLE);
> +	state->polarity = PWM_POLARITY_NORMAL;
> +	state->duty_cycle = div_u64(on_cycles, fpwm->clkrate) * NSEC_PER_SEC;
> +	state->period = div_u64(off_cycles + on_cycles, fpwm->clkrate) * NSEC_PER_SEC;

You're loosing precision here, always do the division last. Also the
rounding is wrong. Enabling PWM_DEBUG + some non-trivial testing should
tell you that.

> +}
> +
> +static const struct pwm_ops apple_pwm_ops = {
> +	.apply = apple_pwm_apply,
> +	.get_state = apple_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int apple_pwm_probe(struct platform_device *pdev)
> +{
> +	struct apple_pwm *pwm;

The name "pwm" is usually (only) used for struct pwm_device variables.

Please pick another name (something like ddata or pc are usual names),
above you picked "fpwm", which I could live with, too.

> +	struct clk *clk;
> +	int ret;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pwm->base))
> +		return PTR_ERR(pwm->base);
> +
> +	platform_set_drvdata(pdev, pwm);

This is unused.

> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(clk))

Error message please, preferably using dev_err_probe.

> +		return PTR_ERR(clk);
> +
> +	pwm->clkrate = clk_get_rate(clk);
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.npwm = 1;
> +	pwm->chip.ops = &apple_pwm_ops;
> +
> +	ret = devm_pwmchip_add(&pdev->dev, &pwm->chip);

Ditto.

> +	return ret;
> +}
> +
> +static const struct of_device_id apple_pwm_of_match[] = {
> +	{ .compatible = "apple,s5l-fpwm" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, apple_pwm_of_match);
> +
> +static struct platform_driver apple_pwm_driver = {
> +	.probe = apple_pwm_probe,
> +	.driver = {
> +		.name = "apple-pwm",
> +		.owner = THIS_MODULE,
> +		.of_match_table = apple_pwm_of_match,
> +	},
> +};
> +module_platform_driver(apple_pwm_driver);
> +
> +MODULE_DESCRIPTION("Apple SoC PWM driver");
> +MODULE_LICENSE("Dual MIT/GPL");

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature