Le mer., mars 16 2022 at 17:37:37 +0200, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> a écrit :
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().
Again, I am not checking a bit, but a mask.
>> + 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.
ffs() gives me the first bit set, this is absolutely not what I want.
Let's say that my kernel supports the JZ4750, JZ4755, JZ4760 SoCs.
That means that I have enabled_socs == 0x38.
Calling is_soc_or_above(jzpc, ID_JZ4740) would resolve to:
(0x38 >> 1) && (!(0x38 & GENMASK(0, 0)) || jzpc->info->version >=
version);
== 1 && (1 || jzpc->info->version >= version)
Which is a compile-time constant equal to 1.
Calling is_soc_or_above(jzpc, ID_JZ4755) would resolve to:
(0x38 >> 4) && (!(0x38 & GENMASK(3, 0)) || jzpc->info->version >=
version);
== 1 && (0 || jzpc->info->version >= version)
which is not a compile-time constant, so the jzpc->info->version must
be checked.
Calling is_soc_or_above(jzpc, ID_JZ4770) would resolve to:
(0x38 >> 6) && (!(0x38 & GENMASK(5, 0)) || jzpc->info->version >=
version);
== 0 && ...
which is a compile-time constant equal to 0.
Cheers,
-Paul
> If I'm mistaken, this all version code needs a good comment.
>
>> + || jzpc->info->version >= version);
>> +}
--
With Best Regards,
Andy Shevchenko
