Re: [PATCH v17] pwm: airoha: Add support for EN7581 SoC

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Jun 25, 2025 at 09:49:01PM +0200, Christian Marangi wrote:
> From: Benjamin Larsson <benjamin.larsson@genexis.eu>
> 
> Introduce driver for PWM module available on EN7581 SoC.

Almost from my perspective.
If you address the below as suggested, feel free to add
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

...

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2022 Markus Gothe <markus.gothe@genexis.eu>

On my calendar 2025 is. Also MODULE_AUTHOR() list disagrees with this
(you should add yourself somewhere here).

> + *  Limitations:
> + *  - Only 8 concurrent waveform generators are available for 8 combinations of
> + *    duty_cycle and period. Waveform generators are shared between 16 GPIO
> + *    pins and 17 SIPO GPIO pins.
> + *  - Supports only normal polarity.
> + *  - On configuration the currently running period is completed.
> + *  - Minimum supported period is 4ms
> + *  - Maximum supported period is 1s
> + */

...

> +#define AIROHA_PWM_SGPIO_CLK_DIVR_32		FIELD_PREP_CONST(AIROHA_PWM_SGPIO_CLK_DIVR, 0x3)
> +#define AIROHA_PWM_SGPIO_CLK_DIVR_16		FIELD_PREP_CONST(AIROHA_PWM_SGPIO_CLK_DIVR, 0x2)
> +#define AIROHA_PWM_SGPIO_CLK_DIVR_8		FIELD_PREP_CONST(AIROHA_PWM_SGPIO_CLK_DIVR, 0x1)
> +#define AIROHA_PWM_SGPIO_CLK_DIVR_4		FIELD_PREP_CONST(AIROHA_PWM_SGPIO_CLK_DIVR, 0x0)

Remove 0x:s, they are just noise and moreover the list here is plain numbers,
not bit combinations or so. In such cases we use decimal numbers.

...

> +/* Cycle cfg handles 4 generators in one register */

I am a bit lost in the meaning of "cycle cfg". Can you elaborate in
the comment that it's readable in plain English?

> +#define AIROHA_PWM_BUCKET_PER_CYCLE_CFG		4

...

> +static u32 airoha_pwm_get_duty_ticks_from_ns(u32 period_ns, u32 duty_ns)
> +{
> +	return mul_u64_u32_div(duty_ns, AIROHA_PWM_DUTY_FULL,
> +			       period_ns);

It's less than 80 if put on one line!

> +}

...

> +static int airoha_pwm_get_bucket(struct airoha_pwm *pc, int bucket,
> +				 u64 *period_ns, u64 *duty_ns)
> +{
> +	u32 period_tick, duty_tick;
> +	unsigned int offset;
> +	u32 shift, val;
> +	int ret;
> +
> +	offset = bucket / AIROHA_PWM_BUCKET_PER_CYCLE_CFG;
> +	shift = bucket % AIROHA_PWM_BUCKET_PER_CYCLE_CFG;
> +	shift = AIROHA_PWM_REG_CYCLE_CFG_SHIFT(shift);
> +
> +	ret = regmap_read(pc->regmap, AIROHA_PWM_REG_CYCLE_CFG_VALUE(offset),
> +			  &val);

You may shorten the line and the code (as below the same can be applied)
by introducing

	struct regmap *map = pc->regmap;

> +	if (ret)
> +		return ret;
> +
> +	period_tick = FIELD_GET(AIROHA_PWM_WAVE_GEN_CYCLE, val >> shift);
> +	*period_ns = period_tick * AIROHA_PWM_PERIOD_TICK_NS;
> +
> +	offset = bucket / AIROHA_PWM_BUCKET_PER_FLASH_PROD;
> +	shift = bucket % AIROHA_PWM_BUCKET_PER_FLASH_PROD;
> +	shift = AIROHA_PWM_REG_GPIO_FLASH_PRD_SHIFT(shift);
> +
> +	ret = regmap_read(pc->regmap, AIROHA_PWM_REG_GPIO_FLASH_PRD_SET(offset),
> +			  &val);
> +	if (ret)
> +		return ret;
> +
> +	duty_tick = FIELD_GET(AIROHA_PWM_GPIO_FLASH_PRD_HIGH, val >> shift);
> +	/*
> +	 * Overflow can't occur in multiplication as duty_tick is just 8 bit
> +	 * and period_ns is clamped to AIROHA_PWM_PERIOD_MAX_NS and fit in a
> +	 * u64.
> +	 */
> +	*duty_ns = DIV_U64_ROUND_UP(duty_tick * *period_ns, AIROHA_PWM_DUTY_FULL);
> +
> +	return 0;
> +}

...

> +static int airoha_pwm_get_generator(struct airoha_pwm *pc, u32 duty_ticks,
> +				    u32 period_ticks)
> +{
> +	int best = -ENOENT, unused = -ENOENT;
> +	u32 best_period_ticks = 0;
> +	u32 best_duty_ticks = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(pc->buckets); i++) {

You want array_size.h.

> +		struct airoha_pwm_bucket *bucket = &pc->buckets[i];
> +		u32 bucket_period_ticks = bucket->period_ticks;
> +		u32 bucket_duty_ticks = bucket->duty_ticks;

> +		/* If found, save an unused bucket to return it later */
> +		if (refcount_read(&bucket->used) == 0) {
> +			unused = i;
> +			continue;
> +		}
> +
> +		/* We found a matching bucket, exit early */
> +		if (duty_ticks == bucket_duty_ticks &&
> +		    period_ticks == bucket_period_ticks)
> +			return i;
> +
> +		/*
> +		 * Unlike duty cycle zero, which can be handled by
> +		 * disabling PWM, a generator is needed for full duty
> +		 * cycle but it can be reused regardless of period
> +		 */
> +		if (duty_ticks == AIROHA_PWM_DUTY_FULL &&
> +		    bucket_duty_ticks == AIROHA_PWM_DUTY_FULL)
> +			return i;
> +
> +		/*
> +		 * With an unused bucket available, skip searching for
> +		 * a bucket to recycle (closer to the requested period/duty)
> +		 */
> +		if (unused != -ENOENT)

Can unused be negative and have different error code or so?
If not, simplify here (and in return below) to

		if (unused >= 0)

> +			continue;
> +
> +		/* Ignore bucket with invalid configs */
> +		if (bucket_period_ticks > period_ticks ||
> +		    bucket_duty_ticks > duty_ticks)
> +			continue;
> +
> +		/*
> +		 * Search for a bucket closer to the requested period/duty
> +		 * that has the maximal possible period that isn't bigger
> +		 * than the requested period. For that period pick the maximal
> +		 * duty_cycle that isn't bigger than the requested duty_cycle.

duty cycle

(with underscore I haven't found a variable in this function the comment refers to)

> +		 */
> +		if (bucket_period_ticks > best_period_ticks ||
> +		    (bucket_period_ticks == best_period_ticks &&
> +		     bucket_duty_ticks > best_duty_ticks)) {
> +			best_period_ticks = bucket_period_ticks;
> +			best_duty_ticks = bucket_duty_ticks;
> +			best = i;
> +		}
> +	}
> +
> +	/* With no unused bucket, return the best one found (if ever) */
> +	return unused == -ENOENT ? best : unused;
> +}

...

> +static int airoha_pwm_consume_generator(struct airoha_pwm *pc,
> +					u32 duty_ticks, u32 period_ticks,
> +					unsigned int hwpwm)
> +{
> +	int bucket, ret;
> +
> +	/*
> +	 * Search for a bucket that already satisfies duty and period
> +	 * or an unused one.
> +	 * If not found, -ENOENT is returned.
> +	 */
> +	bucket = airoha_pwm_get_generator(pc, duty_ticks, period_ticks);
> +	if (bucket < 0)
> +		return bucket;
> +
> +	/* Release previous used bucket (if any) */
> +	airoha_pwm_release_bucket_config(pc, hwpwm);

> +	if (refcount_read(&pc->buckets[bucket].used) == 0) {
> +		pc->buckets[bucket].period_ticks = period_ticks;
> +		pc->buckets[bucket].duty_ticks = duty_ticks;
> +		ret = airoha_pwm_apply_bucket_config(pc, bucket,
> +						     duty_ticks,
> +						     period_ticks);
> +		if (ret)
> +			return ret;
> +
> +		refcount_set(&pc->buckets[bucket].used, 1);

What happens if refcount is updated in between? This is wrong use of atomics.

> +	} else {
> +		refcount_inc(&pc->buckets[bucket].used);

Ditto.

You probably wanted _inc_and_test() variant.

> +	}
> +
> +	return bucket;
> +}

...

> +static int airoha_pwm_config(struct airoha_pwm *pc, struct pwm_device *pwm,
> +			     u32 duty_ticks, u32 period_ticks)
> +{
> +	unsigned int hwpwm = pwm->hwpwm;
> +	int bucket, ret;
> +
> +	bucket = airoha_pwm_consume_generator(pc, duty_ticks, period_ticks,
> +					      hwpwm);
> +	if (bucket < 0)
> +		return bucket;
> +
> +	ret = airoha_pwm_config_flash_map(pc, hwpwm, bucket);
> +	if (ret) {
> +		refcount_dec(&pc->buckets[bucket].used);
> +		return ret;
> +	}

> +	set_bit(hwpwm, pc->initialized);

Does it need to be atomic? (Just asking) If not, use non-atomic variant, i.e.
__set_bit().

> +	pc->channel_bucket[hwpwm] = bucket;
> +
> +	/*
> +	 * SIPO are special GPIO attached to a shift register chip. The handling
> +	 * of this chip is internal to the SoC that takes care of applying the
> +	 * values based on the flash map. To apply a new flash map, it's needed
> +	 * to trigger a refresh on the shift register chip.
> +	 * If a SIPO is getting configuring , always reinit the shift register
> +	 * chip to make sure the correct flash map is applied.
> +	 * Skip reconfiguring the shift register if the related hwpwm
> +	 * is disabled (as it doesn't need to be mapped).
> +	 */
> +	if (hwpwm >= AIROHA_PWM_NUM_GPIO) {
> +		ret = airoha_pwm_sipo_init(pc);
> +		if (ret) {
> +			airoha_pwm_release_bucket_config(pc, hwpwm);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

...

> +static void airoha_pwm_disable(struct airoha_pwm *pc, struct pwm_device *pwm)
> +{
> +	/* Disable PWM and release the bucket */
> +	airoha_pwm_config_flash_map(pc, pwm->hwpwm, -1);
> +	airoha_pwm_release_bucket_config(pc, pwm->hwpwm);

> +	clear_bit(pwm->hwpwm, pc->initialized);

As per above.

> +	/* If no SIPO is used, disable the shift register chip */
> +	if (find_next_bit(pc->initialized, AIROHA_PWM_MAX_CHANNELS,
> +			  AIROHA_PWM_NUM_GPIO) >= AIROHA_PWM_NUM_GPIO)
> +		regmap_clear_bits(pc->regmap, AIROHA_PWM_REG_SIPO_FLASH_MODE_CFG,
> +				  AIROHA_PWM_SERIAL_GPIO_FLASH_MODE);
> +}

...

> +static int airoha_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct airoha_pwm *pc;
> +	struct pwm_chip *chip;
> +	unsigned int i;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, AIROHA_PWM_MAX_CHANNELS, sizeof(*pc));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	chip->ops = &airoha_pwm_ops;
> +	pc = pwmchip_get_drvdata(chip);
> +
> +	pc->regmap = device_node_to_regmap(dev->parent->of_node);

Please, use dev_of_node(dev->parent).

> +	if (IS_ERR(pc->regmap))
> +		return dev_err_probe(dev, PTR_ERR(pc->regmap), "Failed to get PWM regmap\n");
> +
> +	for (i = 0; i < ARRAY_SIZE(pc->buckets); i++)
> +		refcount_set(&pc->buckets[i].used, 0);
> +
> +	ret = devm_pwmchip_add(&pdev->dev, chip);

Seems you have dev, why not use it?

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko