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

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

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

On Mon, Sep 21, 2026 at 12:21:31AM -0700, Petar Stepanovic wrote:
> 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..781053e255f0
> --- /dev/null
> +++ b/drivers/pwm/pwm-axiado.c
> @@ -0,0 +1,277 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * SPDX-FileCopyrightText: 2021-2026 Axiado Corporation.
> + */
> +

Is there a public reference manual for that hardware? If so, adding a
link here would be awesome.

> +/*
> + * Limitations:
> + * - Only normal polarity is supported.
> + * - The hardware has no shadow registers, so configuration changes take
> + *   effect immediately without waiting for the current period to complete.
> + * - The output is driven high while the hardware is disabled.
> + * - Supported period range: 2 through 0xfffffffe PWM input clock cycles;
> + *   0xffffffff is reserved by the hardware for a constant low output.
> + *   Longer periods are clamped to the maximum.
> + * - A 0% duty cycle is emitted using the constant low encoding because the
> + *   hardware interprets a zero high time as a constant high output. The
> + *   requested period is lost in that case and is reported as a single clock
> + *   cycle.
> + * - A 100% duty cycle is supported and emits a constant high output.
> + */
> [...]
> +static int
> +axiado_pwm_round_waveform_tohw(struct pwm_chip *chip,
> +			       struct pwm_device *pwm,
> +			       const struct pwm_waveform *wf,
> +			       void *_wfhw)
> +{
> +	struct axiado_pwm_chip *axpwm = pwmchip_get_drvdata(chip);
> +	struct axiado_pwm_waveform *wfhw = _wfhw;
> +	u64 period;
> +	u64 duty;
> +	int ret = 0;
> +
> +	/* Encode a disabled request as a zeroed hardware waveform. */
> +	if (!wf->period_length_ns) {
> +		*wfhw = (struct axiado_pwm_waveform) { };
> +
> +		return 0;
> +	}
> +
> +	period = mul_u64_u64_div_u64(wf->period_length_ns, axpwm->rate,
> +				     NSEC_PER_SEC);
> +	duty = mul_u64_u64_div_u64(wf->duty_length_ns, axpwm->rate,
> +				   NSEC_PER_SEC);
> +
> +	if (!duty) {
> +		/*
> +		 * A zero high time makes the hardware emit a constant high
> +		 * output, so use the constant low period encoding for a 0%
> +		 * duty cycle. The high time must stay non-zero because it
> +		 * takes precedence over that encoding. The requested period
> +		 * is lost, .round_waveform_fromhw() reports a single clock
> +		 * cycle instead.
> +		 */
> +		*wfhw = (struct axiado_pwm_waveform) {
> +			.period = AXIADO_PWM_PERIOD_CONST_LOW,
> +			.duty = AXIADO_PWM_DUTY_MIN,
> +			.enabled = true,
> +		};
> +
> +		/* That single cycle is longer than a sub-cycle request. */
> +		return period ? 0 : 1;
> +	}
> +
> +	if (period < AXIADO_PWM_PERIOD_MIN) {
> +		period = AXIADO_PWM_PERIOD_MIN;
> +		ret = 1;
> +	} else if (period > AXIADO_PWM_PERIOD_MAX) {
> +		period = AXIADO_PWM_PERIOD_MAX;
> +	}
> +
> +	/*
> +	 * Period clamping can leave the converted duty greater than the
> +	 * final hardware period. In that case, clamp it to 100% duty.
> +	 */
> +	if (duty > period)
> +		duty = period;

So with period in [AXIADO_PWM_PERIOD_MIN, AXIADO_PWM_PERIOD_MAX], duty =
period yields a constant high output? If so, why is there a special
encoding for const high?

> +	*wfhw = (struct axiado_pwm_waveform) {
> +		.period = period,
> +		.duty = duty,
> +		.enabled = true,
> +	};
> +
> +	return ret;
> +}
> +
> +static int
> +axiado_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> +				 struct pwm_device *pwm,
> +				 const void *_wfhw,
> +				 struct pwm_waveform *wf)
> +{
> +	struct axiado_pwm_chip *axpwm = pwmchip_get_drvdata(chip);
> +	const struct axiado_pwm_waveform *wfhw = _wfhw;
> +	u32 period = wfhw->period;
> +	u32 duty = wfhw->duty;
> +
> +	if (!wfhw->enabled) {
> +		*wf = (struct pwm_waveform) {
> +			.period_length_ns = 0,
> +		};
> +
> +		return 0;
> +	}
> +
> +	/*
> +	 * The constant low encoding doesn't hold a period. Report the
> +	 * shortest one, the output is low for all of it either way.
> +	 */
> +	if (period == AXIADO_PWM_PERIOD_CONST_LOW) {
> +		period = 1;
> +		duty = 0;
> +	} else if (duty > period) {
> +		duty = period;
> +	}

You don't handle wfhw->duty == 0 here. I think this is what Sashiko
pointed out. While .round_waveform_tohw() doens't produce this setting,
it would still be good to handle that in case the driver is loaded with
this setting applied in hardware.

> +	*wf = (struct pwm_waveform) {
> +		.period_length_ns =
> +			mul_u64_u64_div_u64_roundup(period, NSEC_PER_SEC,
> +						    axpwm->rate),
> +		.duty_length_ns =
> +			mul_u64_u64_div_u64_roundup(duty, NSEC_PER_SEC,
> +						    axpwm->rate),
> +		.duty_offset_ns = 0,
> +	};
> +
> +	return 0;
> +}

Best regards
Uwe

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