Re: [v6, 1/4] drm/i915/display: harden masks in HSW_AUD_PIN_ELD_CP_VLD macros

Hi Michał,

Thanks for the review and sorry for the delay in responding.

On Wed, 2026-07-01 at 20:31 +0200, Michał Grzelak wrote:
> On Tue, 9 Jun 2026, Luca Coelho wrote:
> > HSW_AUD_PIN_ELD_CP_VLD has a 4-bit field per transcoder for
> > TRANSCODER_A..TRANSCODER_D only (bits 0..15).  Any other transcoder
> 
> nasty nit: double space sign: s/  / /

As we discussed offline, this is intentional, it's just a different
type-setting style that some of us old farts still use.  I'll keep
them. ;)


> > value (TRANSCODER_EDP, TRANSCODER_DSI_*, INVALID_TRANSCODER) is not
> > valid here.
> > 
> > This is not a problem with the current implementation, because trans
> > is always valid when these macros are called, but it's more robust to
> > mask the index to the low 2 bits so the shift is always well-defined.
> > 
> > Reviewed-by: Jani Nikula <[email protected]>
> > Signed-off-by: Luca Coelho <[email protected]>
> > ---
> > drivers/gpu/drm/i915/display/intel_audio_regs.h | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_audio_regs.h b/drivers/gpu/drm/i915/display/intel_audio_regs.h
> > index 4c31844d21df..f3d2a99c03d0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_audio_regs.h
> > +++ b/drivers/gpu/drm/i915/display/intel_audio_regs.h
> > @@ -109,12 +109,20 @@
> > #define _HSW_AUD_EDID_DATA_B		0x65150
> > #define HSW_AUD_EDID_DATA(trans)	_MMIO_TRANS(trans, _HSW_AUD_EDID_DATA_A, _HSW_AUD_EDID_DATA_B)
> > 
> > +/*
> > + * HSW_AUD_PIN_ELD_CP_VLD has a 4-bit field per transcoder for
> > + * TRANSCODER_A..TRANSCODER_D only (bits 0..15).  Any other transcoder
> 
> double space: s/  / /
> 
> > + * value (TRANSCODER_EDP, TRANSCODER_DSI_*, INVALID_TRANSCODER) is not
> > + * valid here.  Mask the index to the low 2 bits so the shift is
> 
> double space: s/  / /
> 
> > + * always well-defined.
> > + */
> > #define HSW_AUD_PIPE_CONV_CFG		_MMIO(0x6507c)
> > #define HSW_AUD_PIN_ELD_CP_VLD		_MMIO(0x650c0)
> > -#define   AUDIO_INACTIVE(trans)		((1 << 3) << ((trans) * 4))
> > -#define   AUDIO_OUTPUT_ENABLE(trans)	((1 << 2) << ((trans) * 4))
> > -#define   AUDIO_CP_READY(trans)		((1 << 1) << ((trans) * 4))
> > -#define   AUDIO_ELD_VALID(trans)	((1 << 0) << ((trans) * 4))
> > +#define   __AUDIO_TRANS_SHIFT(trans)	(((trans) & 0x3) * 4)
> > +#define   AUDIO_INACTIVE(trans)		REG_BIT(__AUDIO_TRANS_SHIFT(trans) + 3)
> > +#define   AUDIO_OUTPUT_ENABLE(trans)	REG_BIT(__AUDIO_TRANS_SHIFT(trans) + 2)
> > +#define   AUDIO_CP_READY(trans)		REG_BIT(__AUDIO_TRANS_SHIFT(trans) + 1)
> > +#define   AUDIO_ELD_VALID(trans)	REG_BIT(__AUDIO_TRANS_SHIFT(trans))
> 
> I'm thinking about something like:
> 
> #define AUDIO_TRANS_MASK 		REG_GENMASK(1, 0)
> #define __AUDIO_TRANS(trans, bit) 	(REG_FIELD_GET(AUDIO_TRANS_MASK, trans) * 4 + (bit))
> #define   AUDIO_INACTIVE(trans) 	REG_BIT(__AUDIO_TRANS(trans, 3))
> #define   AUDIO_OUTPUT_ENABLE(trans) 	REG_BIT(__AUDIO_TRANS(trans, 2))
> #define   AUDIO_CP_READY(trans) 	REG_BIT(__AUDIO_TRANS(trans, 1))
> #define   AUDIO_ELD_VALID(trans) 	REG_BIT(__AUDIO_TRANS(trans, 0))
> 
> or, on a second thought, maybe even:
> 
> #define AUDIO_TRANS_MASK 		REG_GENMASK(3, 2)
> #define __AUDIO_TRANS_SHIFT(trans)	(trans << 2)
> #define __AUDIO_TRANS(trans, bit) 	(REG_FIELD_GET(AUDIO_TRANS_MASK, __AUDIO_TRANS_SHIFT(trans)) + (bit))
> ...
> 
> since multiplying by 4 should be effectively left-shifting by 2, 
> and it shouldn't matter whether we first do the left-shift or bitwise
> AND.
> 
> But no idea if it makes any sense. Anyways:

Neither of these are actually equivalent to the ones I implemented.  I
went through a few different implementations, but then settled on the
one I sent.  Especially the REG_GENMASK() you proposed doesn't work as
intuitively as it seems.


> Reviewed-by: Michał Grzelak <[email protected]>

Thanks!

--
Cheers,
Luca.