Re: [PATCH v15 2/2] pwm: airoha: Add support for EN7581 SoC

[Christian Marangi <ansuelsmth@gmail.com>]

On Tue, Jun 24, 2025 at 09:37:26AM +0300, Andy Shevchenko wrote:
> On Tue, Jun 24, 2025 at 12:11 AM Christian Marangi <ansuelsmth@gmail.com> wrote:
> >
> > Introduce driver for PWM module available on EN7581 SoC.
> 
> ...
> 
> > Signed-off-by: Benjamin Larsson <benjamin.larsson@genexis.eu>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > Co-developed-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com>
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.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.
>

As you can see by the changelog this is very old code so I wasn't
aware.

> > +#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.
> 

Aside from types do you have hint of other missing header? Do you have a
tool to identify the missing header?

> ...
> 
> > +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.
> 

There could be 33 PWM channel so it doesn't fit a u32. This is why u64.
I feel bitmap might be overkill for the task but if requested, I will
change it.

> > +       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.
> 

Mhhh I try to limit code to 80 column where possible.

> > +}
> 
> ...
> 
> > +       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?
> 

I checked and bsearch can't be used and and for util_macros the closest
can't be used. As explained in previous revision, it's not simply a
matter of finding the closest value but it's about finding a value that
is closest to the period_ns and only with that condition satisfied one
closest to the duty. We can't mix them as search for the closest of
both.

> ...
> 
> > +       /* 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?
> 

In the extreme case, a bucket can be used by all 33 PWM channel.

> > +}
> 
> ...
> 
> > +       /*
> > +        * 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?
> 

It's sad but the shift register chip is entirely handled by the SoC. We
can't access to it's registers so the dedicated gpio driver can't be
used.

> ...
> 
> > +       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?
> 

The initialized is still needed to understand if a PWM channel has been
provisioned or it's still "dirty" and assigned to a bucket externally to
the kernel (for example by a bootloader)

Also the documentation is not very clear on what is really considered a
volatile register or not so maybe skipping some write might introduce
some unintended regression.

> ...
> 
> > +       /*
> > +        * 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.
>

I think you are confused duty is divided in 255 segment so I chencged
this to Duty is divided in 255 segment, normalize it t...

> > +        * share a generator.
> > +        */
> 
> -- 
> With Best Regards,
> Andy Shevchenko

-- 
	Ansuel