Re: [PATCH 09/12] drm/i915: Use REG_BIT() & co. for BDW+ EU/slice fuse bits

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 25 Feb 2025, Andi Shyti <andi.shyti@kernel.org> 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@linux.intel.com> wrote:
>> > Hi Ville,
>> >
>> > On Wed, Feb 12, 2025 at 01:19:37AM +0200, Ville Syrjala wrote:
>> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >> 
>> >> 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