Re: [PATCH v9 2/3] pwm: rp1: Add RP1 PWM controller driver

["Gary Guo" <gary@garyguo.net>]

On Fri Sep 18, 2026 at 10:59 AM BST, Andrea della Porta wrote:
> From: Naushir Patuck <naush@raspberrypi.com>
>
> 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 <naush@raspberrypi.com>
> Co-developed-by: Stanimir Varbanov <svarbanov@suse.de>
> Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
> Signed-off-by: Andrea della Porta <andrea.porta@suse.com>

Put yourself a Co-developed-by perhaps? You made significant changes.

I've tested the device on my Pi 5, and the fan is indeed spinning now :)

> ---
>  drivers/pwm/Kconfig                       |   8 +
>  drivers/pwm/Makefile                      |   1 +
>  drivers/pwm/pwm-rp1.c                     | 499 ++++++++++++++++++++++
>  include/soc/bcm2835/raspberrypi-pwm-rp1.h |  10 +
>  4 files changed, 518 insertions(+)
>  create mode 100644 drivers/pwm/pwm-rp1.c
>  create mode 100644 include/soc/bcm2835/raspberrypi-pwm-rp1.h
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7297760868790..e1baec8d294d1 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -637,6 +637,14 @@ config PWM_ROCKCHIP
>  	  Generic PWM framework driver for the PWM controller found on
>  	  Rockchip SoCs.
>  
> +config PWM_RASPBERRYPI_RP1
> +	tristate "RP1 PWM support"
> +	depends on MISC_RP1 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	select REGMAP_MMIO
> +	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 5630a521a7cff..c07fd24f69f39 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -57,6 +57,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..cfd38e46cc589
> --- /dev/null
> +++ b/drivers/pwm/pwm-rp1.c
> +
> +int rp1_pwm_read_tachometer(struct device *dev)
> +{
> +	struct pwm_chip *chip;
> +	struct rp1_pwm *rp1;
> +	u32 tach_val;
> +	int ret;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	device_lock(dev);

You should use the device link mechanism for synchronizing unbind / runtime PM.
That can be done in your RP1 fan driver, and it doesn't need locking on this
driver.

So Sashiko is kinda reporting a false positive here.

> +
> +	chip = dev_get_drvdata(dev);
> +	if (!chip) {
> +		ret = -ENODEV;
> +		goto err_dev_unlock;
> +	}
> +
> +	rp1 = pwmchip_get_drvdata(chip);
> +	if (!rp1) {
> +		ret = -ENODEV;
> +		goto err_dev_unlock;
> +	}
> +
> +	mutex_lock(&rp1->lock);
> +	if (!rp1->clk_enabled) {
> +		ret = -EBUSY;
> +		goto err_clk_unlock;
> +	}
> +
> +	ret = regmap_read(rp1->regmap, RP1_PWM_PHASE(2), &tach_val);
> +	if (ret)
> +		goto err_clk_unlock;
> +
> +	ret = (int)tach_val;
> +
> +err_clk_unlock:
> +	mutex_unlock(&rp1->lock);
> +err_dev_unlock:
> +	device_unlock(dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(rp1_pwm_read_tachometer, "RP1_PWM_FAN");
> +
> +static int rp1_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	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);
> +	ret = devm_mutex_init(dev, &rp1->lock);
> +	if (ret)
> +		return ret;
> +
> +	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");

You mentioned "rework regmap error paths" in cover letter.

But the issue is that regmap doesn't need be used here at all. RP1 is on PCIe
so there is no need for bus abstraction, direct use of MMIO is sufficient.
MMIO accessors have no error paths, so you're saying yourself from having to
handle that, and also reduce the overhead by not having to go through an
abstraction w/ indirect funicton calls.

I suppose regmap was used when syscon was there; but it's not needed anymore.

Best,
Gary

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