Re: [PATCH v7 2/3] pwm: add Andes PWM driver support

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

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

Hello,

On Wed, Aug 12, 2026 at 11:24:05AM +0800, Ben Zong-You Xie via B4 Relay wrote:
> +static int andes_pwm_config(struct pwm_chip *chip, unsigned int channel,
> +			    const struct pwm_state *state)
> +{
> +	struct andes_pwm *ap = andes_pwm_from_chip(chip);
> +	unsigned int clk_rate = ap->extclk_rate;
> +	unsigned int ctrl = ANDES_PWM_CH_CTRL_MODE_PWM;
> +	bool use_pclk = false;
> +	u64 high_cycles;
> +	u64 low_cycles;
> +	u64 period_cycles;
> +	u64 duty_cycles;
> +	u32 reload;
> +
> +	/*
> +	 * Reload register for PWM mode:
> +	 *
> +	 *		31 : 16    15 : 0
> +	 *		PWM16_Hi | PWM16_Lo
> +	 *
> +	 * The high duration is (PWM16_Hi + 1) cycles and the low duration is
> +	 * (PWM16_Lo + 1) cycles, so each phase spans ANDES_PWM_CYCLE_MIN to
> +	 * ANDES_PWM_CYCLE_MAX cycles. The hardware period (their sum) can reach
> +	 * 2 * ANDES_PWM_CYCLE_MAX cycles, but the PWM core requires the period
> +	 * to be chosen from the requested period alone, independent of the duty
> +	 * cycle. That holds only while both phases stay within
> +	 * ANDES_PWM_CYCLE_MAX for every duty split, so the usable period is
> +	 * capped at ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN cycles.
> +	 *
> +	 * The controller has two clock sources, the APB clock and an external
> +	 * clock. Since the external clock frequency must be slower than the APB
> +	 * clock, it is tried first for its wider period range; the APB clock is
> +	 * used only when the external clock resolves fewer than two cycles (or
> +	 * is absent) and is itself usable.
> +	 */
> +	period_cycles = mul_u64_u64_div_u64(clk_rate, state->period,
> +					    NSEC_PER_SEC);
> +	if (period_cycles < 2 * ANDES_PWM_CYCLE_MIN && ap->pclk_rate) {
> +		use_pclk = true;
> +		clk_rate = ap->pclk_rate;
> +		period_cycles = mul_u64_u64_div_u64(clk_rate, state->period,
> +						    NSEC_PER_SEC);

This only helps if ap->pclk_rate > clk_rate. So maybe check that instead
of ap->pclk_rate != 0?

There is still room for improvement: Consider

	ap->extclk_rate = 10000
	ap->pclk_rate = 20000

	state->period = 3100000
	state->duty_cycle = 1550000

Then your algo picks extclk as period_cycles = 31 with it and then
result in duty_cyles = 1500000 while with pclk you could achieve an
exact match. But I think the way the parameters are picked is consistent
and so it's fine to keep it as is.

> +	}
> +
> +	/*
> +	 * A single cycle is still usable: the duty cycle can then only be zero
> +	 * or one cycle, and both are emitted by parking the channel below.
> +	 */
> +	if (!period_cycles)
> +		return -EINVAL;
> +
> +	/*
> +	 * Round the period down to the largest value representable for every
> +	 * duty cycle, so the chosen period depends on the requested period
> +	 * alone. With both phases capped at ANDES_PWM_CYCLE_MAX, that bound is
> +	 * ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN cycles.
> +	 */
> +	period_cycles = min_t(u64, period_cycles,
> +			      ANDES_PWM_CYCLE_MAX + ANDES_PWM_CYCLE_MIN);
> +
> +	/* The duty cycle cannot exceed the (possibly clamped) period. */
> +	duty_cycles = mul_u64_u64_div_u64(clk_rate, state->duty_cycle,
> +					  NSEC_PER_SEC);
> +	duty_cycles = min_t(u64, duty_cycles, period_cycles);
> +
> +	if (state->polarity == PWM_POLARITY_INVERSED) {
> +		low_cycles = duty_cycles;
> +		high_cycles = period_cycles - low_cycles;
> +	} else {
> +		high_cycles = duty_cycles;
> +		low_cycles = period_cycles - high_cycles;
> +	}
> +
> +	/*
> +	 * A zero-length phase means a 0% or 100% relative duty cycle, which the
> +	 * hardware cannot run. Emit the matching constant level by parking the
> +	 * channel: high_cycles == 0 stays low, low_cycles == 0 stays high.
> +	 */
> +	if (!high_cycles)
> +		return andes_pwm_park(chip, channel, false);
> +	if (!low_cycles)
> +		return andes_pwm_park(chip, channel, true);
> +
> +	/*
> +	 * A change of clock source takes effect immediately, modifying the
> +	 * current output. Otherwise there is no glitch as the currently running
> +	 * period is completed before the new settings take effect.
> +	 */
> +	ctrl |= use_pclk ? ANDES_PWM_CH_CTRL_CLK : 0;
> +	ctrl |= (state->polarity == PWM_POLARITY_INVERSED) ?
> +		ANDES_PWM_CH_CTRL_PARK : 0;

So with the PARK bit set the ANDES_PWM_CH_RELOAD_HIGH bitmap actually
configures the time (in ticks) the signal is low? If that isn't the
case, don't claim to support inversed polarity.

> +	regmap_update_bits(ap->regmap, ANDES_PWM_CH_CTRL(channel),
> +			   ANDES_PWM_CH_CTRL_MASK, ctrl);
> +	reload = FIELD_PREP(ANDES_PWM_CH_RELOAD_HIGH, high_cycles - 1) |
> +		 FIELD_PREP(ANDES_PWM_CH_RELOAD_LOW, low_cycles - 1);
> +	regmap_write(ap->regmap, ANDES_PWM_CH_RELOAD(channel), reload);
> +
> +	return andes_pwm_enable(chip, channel, true);
> +}

Best regards
Uwe

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