Re: [PATCH 03/12] wifi: mt76: mt7996: add dsp firmware download

[Kalle Valo <kvalo@kernel.org>]

(I moved Lorenzo's comment to better see the macro)

Lorenzo Bianconi <lorenzo@kernel.org> writes:

>> From: Peter Chiu <chui-hao.chiu@mediatek.com>
>> 
>> Add DSP firmware for phy-related control. The firmware is transparent to
>> driver, but it's necessary for the firmware download process.
>> 
>> Reviewed-by: Shayne Chen <shayne.chen@mediatek.com>
>> Signed-off-by: Peter Chiu <chui-hao.chiu@mediatek.com>
>> Signed-off-by: Shayne Chen <shayne.chen@mediatek.com>

[...]

>> +#define LOAD_RAM(_type) \
>> +	do {									\
>> +		ret = request_firmware(&fw, MT7996_FIRMWARE_##_type, dev->mt76.dev);\
>> +		if (ret)							\
>> +			return ret;						\
>> +										\
>> +		if (!fw || !fw->data || fw->size < sizeof(*hdr)) {		\
>> +			dev_err(dev->mt76.dev, "Invalid firmware\n");		\
>> +			release_firmware(fw);					\
>> +			return -EINVAL;						\
>> +		}								\
>> +										\
>> +		hdr = (const struct mt7996_fw_trailer *)			\
>> +				(fw->data + fw->size - sizeof(*hdr));		\
>> +										\
>> +		dev_info(dev->mt76.dev,						\
>> +			 "%s Firmware Version: %.10s, Build Time: %.15s\n",	\
>> +			 #_type, hdr->fw_ver, hdr->build_date);			\
>> +										\
>> +		ret = mt7996_mcu_send_ram_firmware(dev, hdr, fw->data,		\
>> +						   MT7996_RAM_TYPE_##_type);    \
>> +		if (ret) {							\
>> +			dev_err(dev->mt76.dev, "Failed to start %s firmware\n", #_type);\
>> +			release_firmware(fw);					\
>> +			return ret;						\
>> +		}								\
>> +										\
>> +		release_firmware(fw);						\
>> +	} while (0)
>> +
>> +	LOAD_RAM(WM);
>> +	LOAD_RAM(DSP);
>> +	LOAD_RAM(WA);
>> +#undef LOAD_RAM
>
> I think it would be better to used a regular function instead of a macro, the
> code would be much easier to read.

Yeah, a function is preferred. I think Andrew Morton says: prefer C over
CPP

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches