| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 28 Nov 2022 16:06:15 +0100
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Sasha Finkelstein <fnkl.kernel@...il.com>
Cc: thierry.reding@...il.com, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, marcan@...can.st,
sven@...npeter.dev, alyssa@...enzweig.io, asahi@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org, linux-pwm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND v3 2/4] pwm: Add Apple PWM controller
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@...il.com>
> Acked-by: Sven Peter <sven@...npeter.dev>
> ---
> 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/ |
Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)
Powered by blists - more mailing lists