| 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: <CAHp75VcEJ0w5rcyq_DSHHunYanU5S9OgnRz1t8XervXqGQCX4w@mail.gmail.com>
Date: Tue, 24 Jun 2025 09:37:26 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Uwe Kleine-König <ukleinek@...nel.org>,
Lukas Wunner <lukas@...ner.de>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Herbert Xu <herbert@...dor.apana.org.au>, Andy Shevchenko <andy@...nel.org>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>, linux-kernel@...r.kernel.org,
linux-pwm@...r.kernel.org, Benjamin Larsson <benjamin.larsson@...exis.eu>,
Lorenzo Bianconi <lorenzo@...nel.org>
Subject: Re: [PATCH v15 2/2] pwm: airoha: Add support for EN7581 SoC
On Tue, Jun 24, 2025 at 12:11 AM Christian Marangi <ansuelsmth@...il.com> wrote:
>
> Introduce driver for PWM module available on EN7581 SoC.
...
> Signed-off-by: Benjamin Larsson <benjamin.larsson@...exis.eu>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> Co-developed-by: Lorenzo Bianconi <lorenzo@...nel.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> Co-developed-by: Christian Marangi <ansuelsmth@...il.com>
> Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> ---
> Changes v15:
> - Fix compilation error for 64bit division on 32bit (patch 01)
> - Add prefer async probe
Wow, I am impressed!
...
> +config PWM_AIROHA
> + tristate "Airoha PWM support"
> + depends on ARCH_AIROHA || COMPILE_TEST
> + depends on OF
There is nothing dependent on this. If you want to enable run-time,
why not using this in conjunction with the COMPILE_TEST?
> + select REGMAP_MMIO
...
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
Have you had a chance to read the top of that header file?
No, just no. This header must not be used in the new code.
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/math64.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
Nothing is used from this header. You actually missed mod_devicetable.h.
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
Missing headers, such as types.h.
Please, follow the IWYU principle.
...
> +struct airoha_pwm {
> + struct regmap *regmap;
> + u64 initialized;
Is it bitmap? This looks really weird, at least a comment is a must to
explain why 64-bit for the variable that suggests (by naming) only a
single bit.
> + struct airoha_pwm_bucket buckets[AIROHA_PWM_NUM_BUCKETS];
> +
> + /* Cache bucket used by each pwm channel */
> + u8 channel_bucket[AIROHA_PWM_MAX_CHANNELS];
> +};
...
> +static u32 airoha_pwm_get_duty_ticks_from_ns(u64 period_ns, u64 duty_ns)
> +{
> + return mul_u64_u64_div_u64(duty_ns, AIROHA_PWM_DUTY_FULL,
> + period_ns);
For readability this can be one line.
> +}
...
> + regmap_read(pc->regmap, AIROHA_PWM_REG_GPIO_FLASH_PRD_SET(offset),
> + &val);
Ditto.
Btw, no error checks for regmap_*() calls?
> +static int airoha_pwm_get_generator(struct airoha_pwm *pc, u64 duty_ns,
> + u64 period_ns)
> +{
> + int i, best = -ENOENT, unused = -ENOENT;
Why is 'i' signed?
> + u64 best_period_ns = 0;
> + u64 best_duty_ns = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(pc->buckets); i++) {
> + struct airoha_pwm_bucket *bucket = &pc->buckets[i];
> + u64 bucket_period_ns = bucket->period_ns;
> + u64 bucket_duty_ns = bucket->duty_ns;
> + u32 duty_ticks, duty_ticks_bucket;
> +
> + /* If found, save an unused bucket to return it later */
> + if (!bucket->used) {
> + unused = i;
> + continue;
> + }
> +
> + /* We found a matching bucket, exit early */
> + if (duty_ns == bucket_duty_ns &&
> + period_ns == bucket_period_ns)
> + 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
> + */
> + duty_ticks = airoha_pwm_get_duty_ticks_from_ns(period_ns, duty_ns);
> + duty_ticks_bucket = airoha_pwm_get_duty_ticks_from_ns(bucket_period_ns,
> + bucket_duty_ns);
> + if (duty_ticks == AIROHA_PWM_DUTY_FULL &&
> + duty_ticks_bucket == 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)
> + continue;
> +
> + /* Ignore bucket with invalid configs */
> + if (bucket_period_ns > period_ns ||
> + bucket_duty_ns > duty_ns)
> + 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.
> + */
> + if (bucket_period_ns > best_period_ns ||
> + (bucket_period_ns == best_period_ns &&
> + bucket_duty_ns > best_duty_ns)) {
> + best_period_ns = bucket_period_ns;
> + best_duty_ns = bucket_duty_ns;
> + best = i;
> + }
> + }
> +
> + /* With no unused bucket, return the best one found (if ever) */
> + return unused == -ENOENT ? best : unused;
> +}
This entire function reminds me of something from util_macros.h or
bsearch.h or similar. Can you double check that you really can't
utilise one of those?
...
> + /* Nothing to clear, PWM channel never used */
> + if (!(pc->initialized & BIT_ULL(hwpwm)))
> + return;
So, it's a bitmap, why not use bitmap types and APIs?
> + bucket = pc->channel_bucket[hwpwm];
> + pc->buckets[bucket].used &= ~BIT_ULL(hwpwm);
Oh, why do you need 'used' to be also 64-bit?
> +}
...
> + /*
> + * Search for a bucket that already satisfy duty and period
satisfies
> + * or an unused one.
> + * If not found, -ENOENT is returned.
> + */
...
> +static int airoha_pwm_sipo_init(struct airoha_pwm *pc)
> +{
> + u32 val;
> +
> + if (!(pc->initialized >> AIROHA_PWM_NUM_GPIO))
> + return 0;
It will be clearer if you use bitmap APIs here to show how many bits
are indeed being used in "initialized" for this check.
Basically it's something like find_first_set_from() or so (I don't
remember names by heart). It will show the starting point
and the limit.
...
> + regmap_clear_bits(pc->regmap, AIROHA_PWM_REG_SIPO_FLASH_MODE_CFG,
> + AIROHA_PWM_SERIAL_GPIO_MODE_74HC164);
This is interesting. Can the gpio-74x164 be used as a whole?
...
> + regmap_write(pc->regmap, AIROHA_PWM_REG_SGPIO_CLK_DLY, 0x0);
'0x' is not needed.
...
> + if (regmap_read_poll_timeout(pc->regmap, AIROHA_PWM_REG_SGPIO_LED_DATA, val,
> + !(val & AIROHA_PWM_SGPIO_LED_DATA_SHIFT_FLAG),
> + 10, 200 * USEC_PER_MSEC))
> + return -ETIMEDOUT;
Why is the error code shadowed?
ret = regmap...
if (ret)
return ret;
...
> + if (regmap_read_poll_timeout(pc->regmap, AIROHA_PWM_REG_SGPIO_LED_DATA, val,
> + !(val & AIROHA_PWM_SGPIO_LED_DATA_SHIFT_FLAG),
> + 10, 200 * USEC_PER_MSEC))
> + return -ETIMEDOUT;
Ditto.
...
> + /* index -1 means disable PWM channel */
Negative index means
> + if (index < 0) {
> + }
...
> +static int airoha_pwm_config(struct airoha_pwm *pc, struct pwm_device *pwm,
> + u64 duty_ns, u64 period_ns)
> +{
> + unsigned int hwpwm = pwm->hwpwm;
> + int bucket;
> +
> + bucket = airoha_pwm_consume_generator(pc, duty_ns, period_ns,
> + hwpwm);
> + if (bucket < 0)
> + return -EBUSY;
Why is the error code shadowed?
> +
> + airoha_pwm_config_flash_map(pc, hwpwm, bucket);
> +
> + pc->initialized |= BIT_ULL(hwpwm);
> + 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 we are configuring a SIPO, always reinit the shift register chip
> + * to make sure the correct flash map is applied.
> + * We skip reconfiguring the shift register if we related hwpwm
s/we/the/ ?
> + * is disabled (as it doesn't need to be mapped).
> + */
> + if (!(pc->initialized & BIT_ULL(hwpwm)) && hwpwm >= AIROHA_PWM_NUM_GPIO)
> + airoha_pwm_sipo_init(pc);
> +
> + return 0;
> +}
...
> + if (!(pc->initialized >> AIROHA_PWM_NUM_GPIO))
> + regmap_clear_bits(pc->regmap, AIROHA_PWM_REG_SIPO_FLASH_MODE_CFG,
> + AIROHA_PWM_SERIAL_GPIO_FLASH_MODE);
If you use regmap cache the "initialized" might be not needed at all.
It might be possible to read back (from the cache) the current state
of some registers. Have you checked if this is a feasible approach?
...
> + /*
> + * Duty goes at 255 step, normalize it to check if we can
"in steps of 255 ns" ?
The original comment is confusing as step in singular form may mislead.
> + * share a generator.
> + */
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists