On Wed, Mar 16, 2022 at 5:05 PM Paul Cercueil <paul@xxxxxxxxxxxxxxx> wrote:
> Le mer., mars 16 2022 at 16:47:24 +0200, Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> a écrit :
> > On Wed, Mar 16, 2022 at 4:11 AM Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > wrote:
...
> >> +static const u32 enabled_socs =
> >
> > If you make it unsigned long, it would be easier to switch to bitmap
> > APIs if needed in the future.
>
> Sure, I can make it a unsigned long. But we'd need a lot more entries
> to justify a switch to bitmap APIs...
It was given below, i.e. using test_bit().
> >> + IS_ENABLED(CONFIG_MACH_JZ4730) << ID_JZ4730 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4740) << ID_JZ4740 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4725B) << ID_JZ4725B |
> >> + IS_ENABLED(CONFIG_MACH_JZ4750) << ID_JZ4750 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4755) << ID_JZ4755 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4760) << ID_JZ4760 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4770) << ID_JZ4770 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4775) << ID_JZ4775 |
> >> + IS_ENABLED(CONFIG_MACH_JZ4780) << ID_JZ4780 |
> >> + IS_ENABLED(CONFIG_MACH_X1000) << ID_X1000 |
> >> + IS_ENABLED(CONFIG_MACH_X1500) << ID_X1500 |
> >> + IS_ENABLED(CONFIG_MACH_X1830) << ID_X1830 |
> >> + IS_ENABLED(CONFIG_MACH_X2000) << ID_X2000 |
> >> + IS_ENABLED(CONFIG_MACH_X2100) << ID_X2100;
...
> >> +is_soc_or_above(const struct ingenic_pinctrl *jzpc, enum
> >> jz_version version)
> >> +{
> >> + return (enabled_socs >> version) &&
> >> + (!(enabled_socs & GENMASK((unsigned int)version -
> >> 1, 0))
> >
> > Why casting? Why not use BIT()?
>
> Casting just to be explicit about what we're doing here - I don't like
> doing arithmetic on enums. But I can certainly remove it.
>
> > But these two lines seem like a very interesting way to reinvent the
> > test_bit().
>
> I don't use BIT() or test_bit() because I am not checking a bit, but a
> mask:
> - (enabled_socs >> version) checks that the config supports either
> (version) or anything above;
> - !(enabled_socs & GENMASK(version - 1, 0)) checks that the config does
> not support anything below. If true, the actual version check can be
> skipped and the operation is a compile-time constant, and GCC will trim
> the dead branches accordingly.
You can still simplify the code, no? Calling ffs() (or similar, I
don't remember by heart all of the details) will give you the result
in one op. And it may also be optimized away by the compiler.
> > If I'm mistaken, this all version code needs a good comment.
> >
> >> + || jzpc->info->version >= version);
> >> +}
--
With Best Regards,
Andy Shevchenko
