Re: [PATCH 1/3] ALSA: hda: Fix compilation of snd_hdac_adsp_xxx() helpers

[Takashi Iwai <tiwai@xxxxxxx>]

On Thu, 09 Jan 2025 15:41:59 +0100,
Cezary Rojewski wrote:
> 
> On 2025-01-09 3:20 PM, Takashi Iwai wrote:
> > On Thu, 09 Jan 2025 13:42:18 +0100,
> > Jaroslav Kysela wrote:
> >> 
> >> On 09. 01. 25 13:52, Cezary Rojewski wrote:
> >>> The snd_hdac_adsp_xxx() wrap snd_hdac_reg_xxx() helpers to simplify
> >>> register access for AudioDSP drivers e.g.: the avs-driver. Byte- and
> >>> word-variants of said helps do not expand to bare readx/writex()
> >>> operations but functions instead and, due to pointer type
> >>> incompatibility, cause compilation to fail.
> >>> 
> >>> As AudioDSP drivers e.g.: the avs-driver utilize struct hda_bus (and
> >>> thus struct hdac_bus) as the base structure, add casts to address the
> >>> problem.
> >>> 
> >>> Fixes: c19bd02e9029 ("ALSA: hda: Add helper macros for DSP capable devices")
> >>> Signed-off-by: Cezary Rojewski <cezary.rojewski@xxxxxxxxx>
> >>> ---
> >>>    include/sound/hdaudio_ext.h | 16 ++++++++--------
> >>>    1 file changed, 8 insertions(+), 8 deletions(-)
> >>> 
> >>> diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
> >>> index 957295364a5e..79a010dd0062 100644
> >>> --- a/include/sound/hdaudio_ext.h
> >>> +++ b/include/sound/hdaudio_ext.h
> >>> @@ -120,21 +120,21 @@ int snd_hdac_ext_bus_link_put(struct hdac_bus *bus, struct hdac_ext_link *hlink)
> >>>    void snd_hdac_ext_bus_link_power(struct hdac_device *codec, bool enable);
> >>>      #define snd_hdac_adsp_writeb(chip, reg, value) \
> >>> -	snd_hdac_reg_writeb(chip, (chip)->dsp_ba + (reg), value)
> >>> +	snd_hdac_reg_writeb((struct hdac_bus *)(chip), (chip)->dsp_ba + (reg), value)
> >>>    #define snd_hdac_adsp_readb(chip, reg) \
> >>> -	snd_hdac_reg_readb(chip, (chip)->dsp_ba + (reg))
> >>> +	snd_hdac_reg_readb((struct hdac_bus *)(chip), (chip)->dsp_ba + (reg))
> >>>    #define snd_hdac_adsp_writew(chip, reg, value) \
> >>> -	snd_hdac_reg_writew(chip, (chip)->dsp_ba + (reg), value)
> >>> +	snd_hdac_reg_writew((struct hdac_bus *)(chip), (chip)->dsp_ba + (reg), value)
> >>>    #define snd_hdac_adsp_readw(chip, reg) \
> >>> -	snd_hdac_reg_readw(chip, (chip)->dsp_ba + (reg))
> >>> +	snd_hdac_reg_readw((struct hdac_bus *)(chip), (chip)->dsp_ba + (reg))
> >>>    #define snd_hdac_adsp_writel(chip, reg, value) \
> >>> -	snd_hdac_reg_writel(chip, (chip)->dsp_ba + (reg), value)
> >>> +	snd_hdac_reg_writel((struct hdac_bus *)(chip), (chip)->dsp_ba + (reg), value)
> >>>    #define snd_hdac_adsp_readl(chip, reg) \
> >>> -	snd_hdac_reg_readl(chip, (chip)->dsp_ba + (reg))
> >>> +	snd_hdac_reg_readl((struct hdac_bus *)(chip), (chip)->dsp_ba + (reg))
> >>>    #define snd_hdac_adsp_writeq(chip, reg, value) \
> >>> -	snd_hdac_reg_writeq(chip, (chip)->dsp_ba + (reg), value)
> >>> +	snd_hdac_reg_writeq((struct hdac_bus *)(chip), (chip)->dsp_ba + (reg), value)
> >>>    #define snd_hdac_adsp_readq(chip, reg) \
> >>> -	snd_hdac_reg_readq(chip, (chip)->dsp_ba + (reg))
> >>> +	snd_hdac_reg_readq((struct hdac_bus *)(chip), (chip)->dsp_ba + (reg))
> >>>      #define snd_hdac_adsp_updateb(chip, reg, mask, val) \
> >>>    	snd_hdac_adsp_writeb(chip, reg, \
> >> 
> >> 
> >> I'm not sure, if this change is wanted. The passed pointer validation
> >> from the compiler side is lost with retyping.
> >> 
> >> Perhaps, it would be better to create another set of defines for other
> >> structure pointers.
> > 
> > Agreed.  The cast there secretly assumes the type punning and it hides
> > potential bugs.
> > 
> > As those macros are used only by AVS for now, you can move to AVS
> > locally and redefine for the more appropriate types, too.
> 
> Hi,
> 
> Thank you both for the constructive feedback. From what I understand,
> to fix the compilation issue, it is best to move the macros into
> local, sound/soc/intel/avs/ location, perhaps into the existing
> registers.h file?

Yes, it sounds reasonable.


thanks,

Takashi