Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kalle Valo <kvalo@kernel.org>
To:     Lorenzo Bianconi <lorenzo@kernel.org>
Cc:     Shayne Chen <shayne.chen@mediatek.com>,
        Felix Fietkau <nbd@nbd.name>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Ryder Lee <ryder.lee@mediatek.com>,
        Evelyn Tsai <evelyn.tsai@mediatek.com>,
        Bo Jiao <Bo.Jiao@mediatek.com>,
        linux-mediatek <linux-mediatek@lists.infradead.org>,
        Peter Chiu <chui-hao.chiu@mediatek.com>
Subject: Re: [PATCH 03/12] wifi: mt76: mt7996: add dsp firmware download
References: <20230602152108.26860-1-shayne.chen@mediatek.com>
        <20230602152108.26860-3-shayne.chen@mediatek.com>
        <ZHotQTUHbWOt/ibt@lore-desk>
Date:   Wed, 07 Jun 2023 17:16:02 +0300
In-Reply-To: <ZHotQTUHbWOt/ibt@lore-desk> (Lorenzo Bianconi's message of "Fri,
        2 Jun 2023 19:56:17 +0200")
Message-ID: <878rcvgxy5.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

(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