| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aF1RIVzVNcdsU1DB@smile.fi.intel.com>
Date: Thu, 26 Jun 2025 16:54:41 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Uwe Kleine-König <ukleinek@...nel.org>,
linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
Benjamin Larsson <benjamin.larsson@...exis.eu>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Lorenzo Bianconi <lorenzo@...nel.org>
Subject: Re: [PATCH v17] pwm: airoha: Add support for EN7581 SoC
On Wed, Jun 25, 2025 at 09:49:01PM +0200, Christian Marangi wrote:
> From: Benjamin Larsson <benjamin.larsson@...exis.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@...ux.intel.com>
...
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2022 Markus Gothe <markus.gothe@...exis.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
Powered by blists - more mailing lists