On 28/03/2025 at 19:42, Geert Uytterhoeven wrote: > Hi Marc, > > On Fri, 28 Mar 2025 at 11:20, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote: >> On 28.03.2025 09:51:43, Biju Das wrote: >>>> Yes, it starts at bit 16, but at which bit does it end? >>>> >>>> The GENMASK() helps to document the register names. Of course is works if you replace the FIELD_PREP >>>> with a left shift, but you are replacing some meaningful information about the register name, register >>>> start bit and end bit by just a start bit value. See? You just lost the register name and end bit >>>> info. >>>> >>>> FIELD_PREP() is not only about doing the correct shift but also about documenting that you are putting >>>> the value into a specific register. >>>> >>>> When reading: >>>> >>>> FIELD_PREP(RCANFD_GERFL_EEF0_7, gpriv->channels_mask) >>>> >>>> I immediately understand that you are putting the gpriv->channels_mask value into the GERFL_EEF0_7 >>>> register and I can look at the datasheet for details about that GERFL_EEF0_7 if I want to. >>> >>> Gen4 has 8 channels (GENMASK(16, 23) >>> G3E has 6 channels (GENMASK(16, 21) >>> V4M has 4 channels (GENMASK(16, 19) >>> V3H_2 has 3 channels (GENMASK(16,18) >>> All other SoCs has 2 channels (GENMASK(16,17) >>> >>> So you mean, I should replace RCANFD_GERFL_EEF0_7 with a >>> >>> Generic one RCANFD_GERFL_EEF(x) GENMASK(16, 16 + (x) - 1) to handle >>> this differences >>> >>> Where x is the number of supported channels(info->max_channels) >>> >>> and then use >>> >>> FIELD_PREP(RCANFD_GERFL_EEF(x), gpriv->channels_mask) >> >> The mask for FIELD_PREP must be compile time constant. >> >> Geert recently posted a series to add non constant helpers: >> >> | https://lore.kernel.org/all/1824412519cb8791ab428065116927ee7b77cf35.1738329459.git.geert+renesas@xxxxxxxxx/ >> >> It seems it wasn't applied yet... > > Yeah, people keep on asking for more... > #perfectistheenemyofgoodenough... Modifying the most popular headers really takes a lot of effort. I experienced it first hand when I introduced the GEMASK_U*() fixed width macros: https://lore.kernel.org/all/20250326-fixed-type-genmasks-v8-0-24afed16ca00@xxxxxxxxxx/ Sorry to have been one of those asking for more. My intent was not to block the effort. Your answers were satisfactory to me. In my opinion, just adding a rationale to the patch description of: - why FIELD_PREP() can not be generalized to non-const mask - why the u32_get_bits() & Cie do not work as well would help to have it move forward. If you resend, I would give you my support. Yours sincerely, Vincent Mailhol
