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

["Uwe Kleine-König" <ukleinek@kernel.org>]

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

Hello Andrea,

On Fri, Apr 03, 2026 at 04:31:55PM +0200, 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>
> ---
>  drivers/pwm/Kconfig   |  10 ++
>  drivers/pwm/Makefile  |   1 +
>  drivers/pwm/pwm-rp1.c | 244 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 255 insertions(+)
>  create mode 100644 drivers/pwm/pwm-rp1.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 6f3147518376a..22e4fc6385da2 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -625,6 +625,16 @@ config PWM_ROCKCHIP
>  	  Generic PWM framework driver for the PWM controller found on
>  	  Rockchip SoCs.
>  
> +config PWM_RP1

I prefer PWM_RASPBERRYPI1, or PWM_RASPBERRYPI_RP1 here.

> +	tristate "RP1 PWM support"
> +	depends on MISC_RP1 || COMPILE_TEST
> +	depends on HWMON
> +	help
> +	  PWM framework driver for Raspberry Pi RP1 controller
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-rp1.
> +
>  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..895a7c42fe9c0 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_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..0a1c1c1dd27e9
> --- /dev/null
> +++ b/drivers/pwm/pwm-rp1.c
> @@ -0,0 +1,244 @@
> +// 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 <bart.tanghe@thomasmore.be>
> + */

Please add a paragraph here named "Limitations" in the same format as
several other drivers describing how the driver behaves on disable and
configuration changes (can glitches occur? Is the currently running
period completed or aborted?)

> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +#define PWM_GLOBAL_CTRL		0x000
> +#define PWM_CHANNEL_CTRL(x)	(0x014 + ((x) * 0x10))
> +#define PWM_RANGE(x)		(0x018 + ((x) * 0x10))
> +#define PWM_PHASE(x)		(0x01C + ((x) * 0x10))
> +#define PWM_DUTY(x)		(0x020 + ((x) * 0x10))
> +
> +/* 8:FIFO_POP_MASK + 0:Trailing edge M/S modulation */
> +#define PWM_CHANNEL_DEFAULT	(BIT(8) + BIT(0))
> +#define PWM_CHANNEL_ENABLE(x)	BIT(x)
> +#define PWM_POLARITY		BIT(3)
> +#define SET_UPDATE		BIT(31)
> +#define PWM_MODE_MASK		GENMASK(1, 0)
> +
> +#define NUM_PWMS		4

Please prefix all #defines by something driver specific (e.g. RP1_PWM_).

> +
> +struct rp1_pwm {
> +	void __iomem	*base;
> +	struct clk	*clk;
> +};
> +
> +static const struct hwmon_channel_info * const rp1_fan_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
> +	NULL
> +};
> +
> +static umode_t rp1_fan_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
> +					u32 attr, int channel)
> +{
> +	umode_t mode = 0;
> +
> +	if (type == hwmon_fan && attr == hwmon_fan_input)
> +		mode = 0444;
> +
> +	return mode;
> +}
> +
> +static int rp1_fan_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *val)
> +{
> +	struct rp1_pwm *rp1 = dev_get_drvdata(dev);
> +
> +	if (type != hwmon_fan || attr != hwmon_fan_input)
> +		return -EOPNOTSUPP;
> +
> +	*val = readl(rp1->base + PWM_PHASE(2));
> +
> +	return 0;
> +}

I don't like having hwmon bits in pwm drivers. Is the PWM only usable
for a fan? I guess the hwmon parts should be dropped and a pwm-fan
defined in dt.

> +static const struct hwmon_ops rp1_fan_hwmon_ops = {
> +	.is_visible = rp1_fan_hwmon_is_visible,
> +	.read = rp1_fan_hwmon_read,
> +};
> +
> +static const struct hwmon_chip_info rp1_fan_hwmon_chip_info = {
> +	.ops = &rp1_fan_hwmon_ops,
> +	.info = rp1_fan_hwmon_info,
> +};
> +
> +static void rp1_pwm_apply_config(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	u32 value;
> +
> +	value = readl(rp1->base + PWM_GLOBAL_CTRL);
> +	value |= SET_UPDATE;
> +	writel(value, rp1->base + PWM_GLOBAL_CTRL);
> +}
> +
> +static int rp1_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +
> +	writel(PWM_CHANNEL_DEFAULT, rp1->base + PWM_CHANNEL_CTRL(pwm->hwpwm));

Please add a comment about what this does.

> +	return 0;
> +}
> +
> +static void rp1_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	u32 value;
> +
> +	value = readl(rp1->base + PWM_CHANNEL_CTRL(pwm->hwpwm));
> +	value &= ~PWM_MODE_MASK;
> +	writel(value, rp1->base + PWM_CHANNEL_CTRL(pwm->hwpwm));
> +
> +	rp1_pwm_apply_config(chip, pwm);

What is the purpose of this call?

> +}
> +
> +static int rp1_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	unsigned long clk_rate = clk_get_rate(rp1->clk);
> +	unsigned long clk_period;
> +	u32 value;
> +
> +	if (!clk_rate) {
> +		dev_err(&chip->dev, "failed to get clock rate\n");
> +		return -EINVAL;
> +	}
> +
> +	/* set period and duty cycle */
> +	clk_period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, clk_rate);

DIV_ROUND_CLOSEST is wrong here. (I don't go into details as .apply()
should be dropped.)

> +	writel(DIV_ROUND_CLOSEST(state->duty_cycle, clk_period),

Dividing by the result of a division loses precision.

> +	       rp1->base + PWM_DUTY(pwm->hwpwm));
> +
> +	writel(DIV_ROUND_CLOSEST(state->period, clk_period),
> +	       rp1->base + PWM_RANGE(pwm->hwpwm));
> +
> +	/* set polarity */
> +	value = readl(rp1->base + PWM_CHANNEL_CTRL(pwm->hwpwm));
> +	if (state->polarity == PWM_POLARITY_NORMAL)
> +		value &= ~PWM_POLARITY;
> +	else
> +		value |= PWM_POLARITY;
> +	writel(value, rp1->base + PWM_CHANNEL_CTRL(pwm->hwpwm));
> +
> +	/* enable/disable */
> +	value = readl(rp1->base + PWM_GLOBAL_CTRL);
> +	if (state->enabled)
> +		value |= PWM_CHANNEL_ENABLE(pwm->hwpwm);
> +	else
> +		value &= ~PWM_CHANNEL_ENABLE(pwm->hwpwm);
> +	writel(value, rp1->base + PWM_GLOBAL_CTRL);
> +
> +	rp1_pwm_apply_config(chip, pwm);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops rp1_pwm_ops = {
> +	.request = rp1_pwm_request,
> +	.free = rp1_pwm_free,
> +	.apply = rp1_pwm_apply,

Please implement the waveform callbacks instead of .apply().

> +};
> +
> +static int rp1_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device *hwmon_dev;
> +	struct pwm_chip *chip;
> +	struct rp1_pwm *rp1;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, NUM_PWMS, sizeof(*rp1));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	rp1 = pwmchip_get_drvdata(chip);
> +
> +	rp1->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rp1->base))
> +		return PTR_ERR(rp1->base);
> +
> +	rp1->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(rp1->clk))
> +		return dev_err_probe(dev, PTR_ERR(rp1->clk), "clock not found\n");

Please start error messages with a capital letter.

> +
> +	ret = devm_clk_rate_exclusive_get(dev, rp1->clk);

After this call you can determine the rate just once and fail if it's ==
0.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "fail to get exclusive rate\n");
> +
> +	chip->ops = &rp1_pwm_ops;
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register PWM chip\n");
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "rp1_fan_tach", rp1,
> +							 &rp1_fan_hwmon_chip_info,
> +							 NULL);
> +
> +	if (IS_ERR(hwmon_dev))
> +		return dev_err_probe(dev, PTR_ERR(hwmon_dev),
> +				     "failed to register hwmon fan device\n");
> +
> +	return 0;
> +}
> +
> +static int rp1_pwm_suspend(struct device *dev)
> +{
> +	struct rp1_pwm *rp1 = dev_get_drvdata(dev);
> +
> +	clk_disable_unprepare(rp1->clk);
> +
> +	return 0;
> +}
> +
> +static int rp1_pwm_resume(struct device *dev)
> +{
> +	struct rp1_pwm *rp1 = dev_get_drvdata(dev);
> +
> +	return clk_prepare_enable(rp1->clk);

Hmm, if this fails and then the driver is unbound, the clk operations
are not balanced.

> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(rp1_pwm_pm_ops, rp1_pwm_suspend, rp1_pwm_resume);
> +
> +static const struct of_device_id rp1_pwm_of_match[] = {
> +	{ .compatible = "raspberrypi,rp1-pwm" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, rp1_pwm_of_match);
> +
> +static struct platform_driver rp1_pwm_driver = {
> +	.probe = rp1_pwm_probe,
> +	.driver = {
> +		.name = "rp1-pwm",
> +		.of_match_table = rp1_pwm_of_match,
> +		.pm = pm_ptr(&rp1_pwm_pm_ops),
> +	},
> +};
> +module_platform_driver(rp1_pwm_driver);
> +
> +MODULE_DESCRIPTION("RP1 PWM driver");
> +MODULE_AUTHOR("Naushir Patuck <naush@raspberrypi.com>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.35.3
> 

Best regards
Uwe

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