On Tue, 25 Feb 2025, Andi Shyti <andi.shyti@xxxxxxxxxx> wrote: > Hi Jani, > > On Tue, Feb 25, 2025 at 09:52:41AM +0200, Jani Nikula wrote: >> On Mon, 24 Feb 2025, Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx> wrote: >> > Hi Ville, >> > >> > On Wed, Feb 12, 2025 at 01:19:37AM +0200, Ville Syrjala wrote: >> >> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> >> >> >> Convert the BDW+ EU/slice fuse bits to the modern REG_BIT()/etc. >> >> style. >> > >> > using REG_BIT() and co. doesn't alway make it more readable. In >> > some of the cases below I would have preferred not to use it. >> >> Interesting. I read through the patch and I thought all of it was >> good. Care to elaborate? > > yes you're right, I should have given an example, but I had > already edited the e-mail and I was lazy to get it back. > > In any case, this is an example: > > - s_en = intel_uncore_read(uncore, GEN11_GT_SLICE_ENABLE) & > - GEN11_GT_S_ENA_MASK; > + s_en = REG_FIELD_GET(GEN11_GT_S_ENA_MASK, > + intel_uncore_read(uncore, GEN11_GT_SLICE_ENABLE)); > > > The removed line to me is clearer than the added line. I suppose clarity could be improved with: val = intel_uncore_read(uncore, GEN11_GT_SLICE_ENABLE); s_en = REG_FIELD_GET(GEN11_GT_S_ENA_MASK, val); but in general I think REG_FIELD_GET() is better than just val & mask, because the former handles shifting. (For the record I'm not suggesting separating the read in this patch.) > I'm not saying that it's not good (otherwise I wouldn't have > r-b'ed it), I'm just saying that not always using the REG_* > macros makes the code clearer. > > For consistency with the rest of the patch is anyway fine. Agreed. BR, Jani. > > Thanks, > Andi -- Jani Nikula, Intel
