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

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

On Tue, Jun 24, 2025 at 10:41:54AM +0200, Christian Marangi wrote:
> 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:

...

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

Yes, I understand that. Maybe you should have dropped versioning of the series
:-)

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

Understood.

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

Just above :-)

> Do you have a tool to identify the missing header?

Unfortunately no tool available, this is just by experience of reviewing code
and doing some cleanups in include/ area.

The rule of thumb, for anythin you use, check which header provides that type
or API. But OTOH don't be too pedantic about it, types.h, for example, covers
a lot of basic kernel types and many compiler attributes and so on, no need
to take care of those separately.

TL;DR: you should go through your code and check it.

...

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

Why? It has a shortcuts for unsigned long, so it should give you no difference
on 64-bit compilation. 32-bit might suffer a bit, but if you curious you may
check it. The ask here is only based on the variable naming and unclearness of
how many bits are in use. Also bitmap will help to understand the code better.

For instance, here

	DEFINE_BITMAP(initialized, 33); // or rather a definition for 33

will give immediate understanding how this is used and what are the limits.
With u64 it;s unclear what you will do with a potential garbage in the upper
bits, for example.

So, let's say I prefer to see bitmap types and APIs here based on the above
examples.

...

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

Perhaps adding a comment on top to summarize this, or did I miss it?

...

> > > +       pc->buckets[bucket].used &= ~BIT_ULL(hwpwm);
> > 
> > Oh, why do you need 'used' to be also 64-bit?

Right, but I mean why does each of the buckets require a 64-bit value? Can it
be handled in a single bitmap or so?

Or is it used like a cluster of the PWMs in one bucket?
It feels like bitmap APIs can also improve the implementation here.

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

-- 
With Best Regards,
Andy Shevchenko