Hello Andrea, nice work for a v2! On Fri, Apr 10, 2026 at 04:09:58PM +0200, Andrea della Porta wrote: > From: Naushir Patuck > > The Raspberry Pi RP1 southbridge features an embedded PWM > controller with 4 output channels, alongside an RPM interface > to read the fan speed on the Raspberry Pi 5. > > Add the supporting driver. > > Signed-off-by: Naushir Patuck > Co-developed-by: Stanimir Varbanov > Signed-off-by: Stanimir Varbanov > Signed-off-by: Andrea della Porta > --- > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-rp1.c | 344 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 354 insertions(+) > create mode 100644 drivers/pwm/pwm-rp1.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 6f3147518376a..32031f2af75af 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -625,6 +625,15 @@ config PWM_ROCKCHIP > Generic PWM framework driver for the PWM controller found on > Rockchip SoCs. > > +config PWM_RASPBERRYPI_RP1 > + bool "RP1 PWM support" > + depends on MISC_RP1 || COMPILE_TEST > + depends on HAS_IOMEM > + select REGMAP_MMIO > + select MFD_SYSCON > + help > + PWM framework driver for Raspberry Pi RP1 controller. > + > config PWM_SAMSUNG > tristate "Samsung PWM support" > depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 0dc0d2b69025d..59f29f60f9123 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -56,6 +56,7 @@ obj-$(CONFIG_PWM_RENESAS_RZG2L_GPT) += pwm-rzg2l-gpt.o > obj-$(CONFIG_PWM_RENESAS_RZ_MTU3) += pwm-rz-mtu3.o > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > +obj-$(CONFIG_PWM_RASPBERRYPI_RP1) += pwm-rp1.o > obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o > obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o > obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o > diff --git a/drivers/pwm/pwm-rp1.c b/drivers/pwm/pwm-rp1.c > new file mode 100644 > index 0000000000000..b88c697d9567e > --- /dev/null > +++ b/drivers/pwm/pwm-rp1.c > @@ -0,0 +1,344 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * pwm-rp1.c > + * > + * Raspberry Pi RP1 PWM. > + * > + * Copyright © 2026 Raspberry Pi Ltd. > + * > + * Author: Naushir Patuck (naush@raspberrypi.com) > + * > + * Based on the pwm-bcm2835 driver by: > + * Bart Tanghe > + * > + * Datasheet: https://pip-assets.raspberrypi.com/categories/892-raspberry-pi-5/documents/RP-008370-DS-1-rp1-peripherals.pdf?disposition=inline > + * > + * Limitations: > + * - Channels can be enabled/disabled and their duty cycle and period can > + * be updated glitchlessly. Update are synchronized with the next strobe > + * at the end of the current period of the respective channel, once the > + * update bit is set. The update flag is global, not per-channel. > + * - Channels are phase-capable, but on RPi5, the firmware can use a channel > + * phase register to report the RPM of the fan connected to that PWM > + * channel. As a result, phase control will be ignored for now. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define RP1_PWM_GLOBAL_CTRL 0x000 > +#define RP1_PWM_CHANNEL_CTRL(x) (0x014 + ((x) * 0x10)) > +#define RP1_PWM_RANGE(x) (0x018 + ((x) * 0x10)) > +#define RP1_PWM_PHASE(x) (0x01C + ((x) * 0x10)) > +#define RP1_PWM_DUTY(x) (0x020 + ((x) * 0x10)) > + > +/* 8:FIFO_POP_MASK + 0:Trailing edge M/S modulation */ > +#define RP1_PWM_CHANNEL_DEFAULT (BIT(8) + BIT(0)) Please add a #define for BIT(8) and then use that and FIELD_PREP(RP1_PWM_MODE, RP1_PWM_MODE_SOMENICENAME) to define the constant. Also I would define it below the register defines. > +#define RP1_PWM_CHANNEL_ENABLE(x) BIT(x) > +#define RP1_PWM_POLARITY BIT(3) > +#define RP1_PWM_SET_UPDATE BIT(31) > +#define RP1_PWM_MODE_MASK GENMASK(1, 0) s/_MASK// please It would be great if the bitfield's names started with the register name. > + > +#define RP1_PWM_NUM_PWMS 4 > + > +struct rp1_pwm { > + struct regmap *regmap; > + struct clk *clk; > + unsigned long clk_rate; > + bool clk_enabled; > +}; > + > +struct rp1_pwm_waveform { > + u32 period_ticks; > + u32 duty_ticks; > + bool enabled; > + bool inverted_polarity; > +}; > + > +static const struct regmap_config rp1_pwm_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = 0x60, I'm not a fan of aligning the = in a struct, still more if it fails like here. Please consistently align all =s, or even better, use a single space before each =. (Same for the struct definitions above, but I won't insist.) > +}; > + > +static void rp1_pwm_apply_config(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > + u32 value; > + > + /* update the changed registers on the next strobe to avoid glitches */ > + regmap_read(rp1->regmap, RP1_PWM_GLOBAL_CTRL, &value); > + value |= RP1_PWM_SET_UPDATE; > + regmap_write(rp1->regmap, RP1_PWM_GLOBAL_CTRL, value); I assume there is a glitch if I update two channels and the old configuration of the first channel ends while I'm in the middle of configuring the second? > +} > + > +static int rp1_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > + > + /* init channel to reset defaults */ > + regmap_write(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), RP1_PWM_CHANNEL_DEFAULT); > + return 0; > +} > + > +static int rp1_pwm_round_waveform_tohw(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const struct pwm_waveform *wf, > + void *_wfhw) > +{ > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > + struct rp1_pwm_waveform *wfhw = _wfhw; > + u64 clk_rate = rp1->clk_rate; > + u64 ticks; if (!wf->period_length_ns) wfhw->enabled = false return 0; > + ticks = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC); To ensure this doesn't overflow please fail to probe the driver if clk_rate > 1 GHz with an explaining comment. (Or alternatively calculate the length of period_ticks = U32_MAX and skip the calculation if wf->period_length_ns is bigger.) > + if (ticks > U32_MAX) > + ticks = U32_MAX; > + wfhw->period_ticks = ticks; What happens if wf->period_length_ns > 0 but ticks == 0? > + if (wf->duty_offset_ns + wf->duty_length_ns >= wf->period_length_ns) { The maybe surprising effect here is that in the two cases wf->duty_offset_ns == wf->period_length_ns and wf->duty_length_ns == 0 and wf->duty_length_ns == wf->period_length_ns and wf->duty_offset_ns == 0 you're configuring inverted polarity. I doesn't matter technically because the result is the same, but for consumers still using pwm_state this is irritating. That's why pwm-stm32 uses inverted polarity only if also wf->duty_length_ns and wf->duty_offset_ns are non-zero. > + ticks = mul_u64_u64_div_u64(wf->period_length_ns - wf->duty_length_ns, > + clk_rate, NSEC_PER_SEC); The rounding is wrong here. You should pick the biggest duty_length not bigger than wf->duty_length_ns, so you have to use ticks = wfhw->period_ticks - mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC): . I see this is a hole in the pwmtestperf coverage. > + wfhw->inverted_polarity = true; > + } else { > + ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC); > + wfhw->inverted_polarity = false; > + } > + > + if (ticks > wfhw->period_ticks) > + ticks = wfhw->period_ticks; You can and should assume that wf->duty_length_ns <= wf->period_length_ns. Then the if condition can never become true. > + wfhw->duty_ticks = ticks; > + > + wfhw->enabled = !!wfhw->duty_ticks; > + > + return 0; > +} > + > +static int rp1_pwm_round_waveform_fromhw(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const void *_wfhw, > + struct pwm_waveform *wf) > +{ > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > + const struct rp1_pwm_waveform *wfhw = _wfhw; > + u64 clk_rate = rp1->clk_rate; > + u32 ticks; > + > + memset(wf, 0, sizeof(*wf)); wf = (struct pwm_waveform){ }; is usually more efficient. > + if (!wfhw->enabled) > + return 0; > + > + wf->period_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->period_ticks * NSEC_PER_SEC, clk_rate); > + > + if (wfhw->inverted_polarity) { > + wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC, > + clk_rate); > + } else { > + wf->duty_offset_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC, > + clk_rate); > + ticks = wfhw->period_ticks - wfhw->duty_ticks; > + wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)ticks * NSEC_PER_SEC, clk_rate); > + } This needs adaption after the rounding issue in tohw is fixed. > + return 0; > +} > + > +static int rp1_pwm_write_waveform(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const void *_wfhw) > +{ > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > + const struct rp1_pwm_waveform *wfhw = _wfhw; > + u32 value; > + > + /* set period and duty cycle */ > + regmap_write(rp1->regmap, > + RP1_PWM_RANGE(pwm->hwpwm), wfhw->period_ticks); > + regmap_write(rp1->regmap, > + RP1_PWM_DUTY(pwm->hwpwm), wfhw->duty_ticks); > + > + /* set polarity */ > + regmap_read(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), &value); > + if (!wfhw->inverted_polarity) > + value &= ~RP1_PWM_POLARITY; > + else > + value |= RP1_PWM_POLARITY; > + regmap_write(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), value); > + > + /* enable/disable */ > + regmap_read(rp1->regmap, RP1_PWM_GLOBAL_CTRL, &value); > + if (wfhw->enabled) > + value |= RP1_PWM_CHANNEL_ENABLE(pwm->hwpwm); > + else > + value &= ~RP1_PWM_CHANNEL_ENABLE(pwm->hwpwm); > + regmap_write(rp1->regmap, RP1_PWM_GLOBAL_CTRL, value); You can exit early if wfhw->enabled is false after clearing the channel enable bit. > + rp1_pwm_apply_config(chip, pwm); > + > + return 0; > +} > + > +static int rp1_pwm_read_waveform(struct pwm_chip *chip, > + struct pwm_device *pwm, > + void *_wfhw) > +{ > + struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip); > + struct rp1_pwm_waveform *wfhw = _wfhw; > + u32 value; > + > + regmap_read(rp1->regmap, RP1_PWM_GLOBAL_CTRL, &value); > + wfhw->enabled = !!(value & RP1_PWM_CHANNEL_ENABLE(pwm->hwpwm)); > + > + regmap_read(rp1->regmap, RP1_PWM_CHANNEL_CTRL(pwm->hwpwm), &value); > + wfhw->inverted_polarity = !!(value & RP1_PWM_POLARITY); > + > + if (wfhw->enabled) { > + regmap_read(rp1->regmap, RP1_PWM_RANGE(pwm->hwpwm), &wfhw->period_ticks); > + regmap_read(rp1->regmap, RP1_PWM_DUTY(pwm->hwpwm), &wfhw->duty_ticks); > + } else { > + wfhw->period_ticks = 0; > + wfhw->duty_ticks = 0; > + } > + > + return 0; > +} > + > +static const struct pwm_ops rp1_pwm_ops = { > + .sizeof_wfhw = sizeof(struct rp1_pwm_waveform), > + .request = rp1_pwm_request, > + .round_waveform_tohw = rp1_pwm_round_waveform_tohw, > + .round_waveform_fromhw = rp1_pwm_round_waveform_fromhw, > + .read_waveform = rp1_pwm_read_waveform, > + .write_waveform = rp1_pwm_write_waveform, > +}; > + > +static int rp1_pwm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + unsigned long clk_rate; > + struct pwm_chip *chip; > + void __iomem *base; > + struct rp1_pwm *rp1; > + int ret; > + > + chip = devm_pwmchip_alloc(dev, RP1_PWM_NUM_PWMS, sizeof(*rp1)); > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + > + rp1 = pwmchip_get_drvdata(chip); > + > + base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + rp1->regmap = devm_regmap_init_mmio(dev, base, &rp1_pwm_regmap_config); > + if (IS_ERR(rp1->regmap)) > + return dev_err_probe(dev, PTR_ERR(rp1->regmap), "Cannot initialize regmap\n"); > + > + ret = of_syscon_register_regmap(np, rp1->regmap); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to register syscon\n"); > + > + rp1->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(rp1->clk)) > + return dev_err_probe(dev, PTR_ERR(rp1->clk), "Clock not found\n"); > + > + ret = clk_prepare_enable(rp1->clk); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to enable clock\n"); > + rp1->clk_enabled = true; > + > + ret = devm_clk_rate_exclusive_get(dev, rp1->clk); > + if (ret) { > + dev_err_probe(dev, ret, "Fail to get exclusive rate\n"); s/Fail/Failed/ > + goto err_disable_clk; > + } > + > + clk_rate = clk_get_rate(rp1->clk); > + if (!clk_rate) { > + ret = dev_err_probe(dev, -EINVAL, "Failed to get clock rate\n"); > + goto err_disable_clk; > + } > + rp1->clk_rate = clk_rate; > + > + chip->ops = &rp1_pwm_ops; > + > + platform_set_drvdata(pdev, chip); > + > + ret = devm_pwmchip_add(dev, chip); > + if (ret) { > + dev_err_probe(dev, ret, "Failed to register PWM chip\n"); > + goto err_disable_clk; > + } > + > + return 0; > + > +err_disable_clk: > + clk_disable_unprepare(rp1->clk); > + > + return ret; > +} On remove you miss to balance the call to clk_prepare_enable() (if no failed call to clk_prepare_enable() in rp1_pwm_resume() happend). > + > +static int rp1_pwm_suspend(struct device *dev) > +{ > + struct rp1_pwm *rp1 = dev_get_drvdata(dev); > + > + if (rp1->clk_enabled) { > + clk_disable_unprepare(rp1->clk); > + rp1->clk_enabled = false; > + } > + > + return 0; > +} > + > +static int rp1_pwm_resume(struct device *dev) > +{ > + struct rp1_pwm *rp1 = dev_get_drvdata(dev); > + int ret; > + > + ret = clk_prepare_enable(rp1->clk); > + if (ret) { > + dev_err(dev, "Failed to enable clock on resume: %d\n", ret); Please use %pe for error codes. > + return ret; > + } > + > + rp1->clk_enabled = true; > + > + return 0; > +} Best regards Uwe