On Tue, 2026-05-19 at 14:49 +0300, Jani Nikula wrote: > On Tue, 19 May 2026, Luca Coelho <[email protected]> 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 > > 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. > > > > 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..25df7af4f67f 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 > > + * value (TRANSCODER_EDP, TRANSCODER_DSI_*, INVALID_TRANSCODER) is not > > + * valid here. Mask the index to the low 2 bits so the shift is > > + * 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_MASK_TRANS(trans) (((unsigned int)(trans) & 0x3) > > * 4) > > +#define AUDIO_INACTIVE(trans) ((u32)BIT(3) << > > __AUDIO_MASK_TRANS(trans)) > > +#define AUDIO_OUTPUT_ENABLE(trans) ((u32)BIT(2) << > > __AUDIO_MASK_TRANS(trans)) > > +#define AUDIO_CP_READY(trans) ((u32)BIT(1) << > > __AUDIO_MASK_TRANS(trans)) > > +#define AUDIO_ELD_VALID(trans) ((u32)BIT(0) << > > __AUDIO_MASK_TRANS(trans)) > > Please use REG_BIT() instead of (u32)BIT().
Yeah, looks much nicer, thanks. I converted it to use REG_BIT(). > All of this would be cleaner with the REG_GENMASK() stuff too. I don't see how REG_GENMASK() could be used here. We're handling single-bits only, with a shift to get it in the proper place, so it's not really a mask. -- Cheers, Luca.
