Re: [PATCH 2/2] pwm: add Axiado AX3000 PWM driver

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

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

Hello Petar,

Just a very high-level review:

On Thu, Jun 18, 2026 at 05:26:57AM -0700, Petar Stepanovic wrote:
> The Axiado AX3000 and AX3005 SoCs include PWM controllers that can be
> used to generate configurable PWM output signals.
> 
> Add a PWM driver with support for configuring period, duty cycle, and
> enable state through the Linux PWM framework.
> 
> Signed-off-by: Petar Stepanovic <pstepanovic@axiado.com>
> ---
>  MAINTAINERS              |   1 +
>  drivers/pwm/Kconfig      |  11 +++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-axiado.c | 193 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 206 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 394c4a3527e8..db93fc235c32 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4319,6 +4319,7 @@ M:	Prasad Bolisetty <pbolisetty@axiado.com>
>  L:	linux-pwm@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/pwm/axiado,ax3000-pwm.yaml
> +F:	drivers/pwm/pwm-axiado.c
>  
>  AXIS ARTPEC ARM64 SoC SUPPORT
>  M:	Jesper Nilsson <jesper.nilsson@axis.com>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 6f3147518376..76f6c04b0e23 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -129,6 +129,17 @@ config PWM_ATMEL_TCB
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-atmel-tcb.
>  
> +config PWM_AXIADO
> +	tristate "Axiado PWM support"
> +	depends on ARCH_AXIADO || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  PWM framework driver for the PWM controller found on Axiado
> +	  AX3000 and AX3005 SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-axiado.
> +
>  config PWM_AXI_PWMGEN
>  	tristate "Analog Devices AXI PWM generator"
>  	depends on MICROBLAZE || NIOS2 || ARCH_ZYNQ || ARCH_ZYNQMP || ARCH_INTEL_SOCFPGA || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 0dc0d2b69025..4466a29e780a 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_PWM_ARGON_FAN_HAT)	+= pwm-argon-fan-hat.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
> +obj-$(CONFIG_PWM_AXIADO)	+= pwm-axiado.o
>  obj-$(CONFIG_PWM_AXI_PWMGEN)	+= pwm-axi-pwmgen.o
>  obj-$(CONFIG_PWM_BCM2835)	+= pwm-bcm2835.o
>  obj-$(CONFIG_PWM_BCM_IPROC)	+= pwm-bcm-iproc.o
> diff --git a/drivers/pwm/pwm-axiado.c b/drivers/pwm/pwm-axiado.c
> new file mode 100644
> index 000000000000..db197886c5c4
> --- /dev/null
> +++ b/drivers/pwm/pwm-axiado.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2021-2026 Axiado Corporation.

Please add a Limitations paragraph here like the ones found in the newer
driver files. It should answer:

 - Is a period completed on configuration change?
 - Is a period completed on disable?
 - How does the output behave when disabled? (Low? Inactive? Freeze? High-Z?)

Also mention special properties, like being unable to set 0% or 100%
relative duty.

> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +/* Register offsets */
> +#define AX_PWM_CNTRL_REG     0x0000
> +#define AX_PWM_PERIOD_REG    0x0004
> +#define AX_PWM_HIGH_REG      0x0008
> +
> +/* PWM channels */
> +#define AX_PWM_NUM 1

This is only used once, and having

	chip = devm_pwmchip_alloc(dev, 1, sizeof(*axpwm));

below simplifies grepping for channel numbers.

> +
> +/* Period and duty cycle limits */
> +#define AX_PWM_PERIOD_MIN       2
> +#define AX_PWM_PERIOD_MAX       0xfffffffeU
> +#define AX_PWM_DUTY_MIN         1
> +#define AX_PWM_DUTY_MAX         0xfffffffdU

The U suffix is not needed for hex constants (AFAIK).

> +
> +/* Control register bits */
> +#define AX_PWM_CTRL_ENABLE BIT(0)
> +#define AX_PWM_CTRL_DISABLE 0x0
> +
> +struct axiado_pwm_chip {
> +	struct clk *clk;
> +	void __iomem *base;
> +};

If you use axiado_pwm_ as prefix for structs and functions, please use
AXIADO_PWM_ as prefix for #defines.

> +
> +static int axiado_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     u64 duty_ns, u64 period_ns)
> +{
> +	struct axiado_pwm_chip *axpwm = pwmchip_get_drvdata(chip);
> +	unsigned long rate;
> +	u64 period_cycles, duty_cycles;
> +
> +	/*
> +	 * The hardware does not support a zero period, 0% duty cycle, or
> +	 * 100% duty cycle. The caller should handle 0% duty cycle by
> +	 * disabling the PWM.
> +	 */
> +	if (!period_ns || !duty_ns || duty_ns >= period_ns)
> +		return -EINVAL;
> +
> +	rate = clk_get_rate(axpwm->clk);
> +	if (!rate)
> +		return -EINVAL;
> +
> +	period_cycles = mul_u64_u64_div_u64(period_ns, rate, NSEC_PER_SEC);
> +	if (period_cycles < AX_PWM_PERIOD_MIN ||
> +	    period_cycles > AX_PWM_PERIOD_MAX)
> +		return -EINVAL;
> +
> +	duty_cycles = mul_u64_u64_div_u64(duty_ns, rate, NSEC_PER_SEC);
> +	if (duty_cycles < AX_PWM_DUTY_MIN ||
> +	    duty_cycles > AX_PWM_DUTY_MAX)
> +		return -EINVAL;
> +
> +	if (duty_cycles >= period_cycles)
> +		return -EINVAL;
> +
> +	writel((u32)period_cycles, axpwm->base + AX_PWM_PERIOD_REG);
> +	writel((u32)duty_cycles, axpwm->base + AX_PWM_HIGH_REG);
> +
> +	return 0;
> +}
> +
> +static int axiado_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	struct axiado_pwm_chip *axpwm = pwmchip_get_drvdata(chip);
> +	int ret;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	if (!state->enabled || !state->duty_cycle) {
> +		if (pwm->state.enabled)
> +			writel(AX_PWM_CTRL_DISABLE, axpwm->base + AX_PWM_CNTRL_REG);
> +
> +		return 0;
> +	}
> +
> +	ret = axiado_pwm_config(chip, pwm, state->duty_cycle, state->period);
> +	if (ret)
> +		return ret;
> +
> +	if (!pwm->state.enabled)

Ideally check hardware state and not PWM internal variables.

> +		writel(AX_PWM_CTRL_ENABLE, axpwm->base + AX_PWM_CNTRL_REG);
> +
> +	return 0;
> +}
> +
> +static int axiado_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				struct pwm_state *state)
> +{
> +	struct axiado_pwm_chip *axpwm = pwmchip_get_drvdata(chip);
> +	unsigned long rate;
> +	u32 period_cycles;
> +	u32 duty_cycles;
> +	u32 ctrl;
> +
> +	rate = clk_get_rate(axpwm->clk);
> +	if (!rate)
> +		return -EINVAL;
> +
> +	ctrl = readl(axpwm->base + AX_PWM_CNTRL_REG);
> +	period_cycles = readl(axpwm->base + AX_PWM_PERIOD_REG);
> +	duty_cycles = readl(axpwm->base + AX_PWM_HIGH_REG);
> +
> +	state->enabled = !!(ctrl & AX_PWM_CTRL_ENABLE);
> +	state->period = mul_u64_u64_div_u64(period_cycles, NSEC_PER_SEC, rate);
> +	state->duty_cycle = mul_u64_u64_div_u64(duty_cycles, NSEC_PER_SEC, rate);
> +	state->polarity = PWM_POLARITY_NORMAL;

Please test your driver with PWM_DEBUG enabled, the rounding is wrong
here.

> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops axiado_pwm_ops = {
> +	.get_state = axiado_pwm_get_state,
> +	.apply = axiado_pwm_apply,

Please implement the waveform callbacke instead of .get_state() and
.apply()

> +};
> +
> +static void axiado_pwm_disable(void *data)
> +{
> +	struct axiado_pwm_chip *axpwm = data;
> +
> +	writel(AX_PWM_CTRL_DISABLE, axpwm->base + AX_PWM_CNTRL_REG);
> +}
> +
> +static int axiado_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct axiado_pwm_chip *axpwm;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, AX_PWM_NUM, sizeof(*axpwm));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	axpwm = pwmchip_get_drvdata(chip);
> +
> +	axpwm->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(axpwm->base))
> +		return dev_err_probe(dev, PTR_ERR(axpwm->base),
> +				     "failed to map registers\n");

Start error messages with a capital letter please.

> +
> +	ret = devm_add_action_or_reset(dev, axiado_pwm_disable, axpwm);
> +	if (ret)
> +		return ret;

This isn't supposed to happen. It's the responsibility of the consumer
to disable the PWM before it's freed.

> +
> +

Single empty line only.

> +	axpwm->clk = devm_clk_get_enabled(dev, "pwm");
> +	if (IS_ERR(axpwm->clk))
> +		return dev_err_probe(dev, PTR_ERR(axpwm->clk),
> +				     "failed to get/enable clock\n");

Please ensure that the clk rate doesn't change while the PWM is enabled.
Then you can cache the clk rate and set chip->atomic.

> +
> +	chip->ops = &axiado_pwm_ops;
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add PWM chip\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id axiado_pwm_match[] = {
> +	{ .compatible = "axiado,ax3000-pwm" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, axiado_pwm_match);
> +
> +static struct platform_driver axiado_pwm_driver = {
> +	.driver = {
> +		.name =  "axiado-pwm",
> +		.of_match_table = axiado_pwm_match,
> +	},
> +	.probe = axiado_pwm_probe,
> +};
> +
> +module_platform_driver(axiado_pwm_driver);

No empty line between the driver struct and the module_platform helper
please.

> +
> +MODULE_AUTHOR("Axiado Corporation");
> +MODULE_DESCRIPTION("Axiado PWM driver");
> +MODULE_LICENSE("GPL");

Best regards
Uwe

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