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

[Cezary Rojewski <cezary.rojewski@intel.com>]

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@intel.com>
>>> ---
>>>    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?

Kind regards,
Czarek