From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A5FCFC4167B for ; Tue, 5 Dec 2023 01:11:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343749AbjLEBLC convert rfc822-to-8bit (ORCPT ); Mon, 4 Dec 2023 20:11:02 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56682 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231618AbjLEBLA (ORCPT ); Mon, 4 Dec 2023 20:11:00 -0500 Received: from irl.hu (irl.hu [95.85.9.111]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0DDC69B for ; Mon, 4 Dec 2023 17:11:05 -0800 (PST) Received: from [192.168.2.4] (51b68398.dsl.pool.telekom.hu [::ffff:81.182.131.152]) (AUTH: CRAM-MD5 soyer@irl.hu, ) by irl.hu with ESMTPSA id 0000000000071433.00000000656E78A6.00115099; Tue, 05 Dec 2023 02:11:02 +0100 Message-ID: Subject: Re: [PATCH 1/2] ASoc: tas2563: DSP Firmware loading support From: Gergo Koteles To: Pierre-Louis Bossart , Shenghao Ding , Kevin Lu , Baojun Xu , Jaroslav Kysela , Takashi Iwai , Liam Girdwood , Mark Brown Cc: linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org Date: Tue, 05 Dec 2023 02:11:01 +0100 In-Reply-To: <7a919eef-d9b4-4bcb-bc19-9a6868d1cc54@linux.intel.com> References: <7a919eef-d9b4-4bcb-bc19-9a6868d1cc54@linux.intel.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT User-Agent: Evolution 3.50.1 (3.50.1-1.fc39) MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Pierre-Louis, Thank you for your review. On Mon, 2023-12-04 at 18:05 -0600, Pierre-Louis Bossart wrote: > > > > sound/soc/codecs/tas2562.c | 2 +- > > are tas2562 and tas2563 (from commit subject) the same? > No, tas2563 is register compatible with tas2562. > > > > +#include > > +#include > > nit-pick: alphabetical order? > Good idea, I'll fix it and the others in the next version. > > + > > +#include > > +#include > > + > > + > > +static void tas25xx_process_fw_delay(struct tas25xx_cmd *cmd) > > +{ > > + mdelay(cpu_to_be16(cmd->hdr.length)); > > is this the length of the header, or the duration of the delay? > > Someone will get it wrong with this naming... > I think this is because of the format. If cmd_type is TAS25XX_CMD_DELAY, then hdr.length is the length of the delay. At least I think so, based on https://lore.kernel.org/lkml/6d6aaed3-dac8-e1ec-436c-9b04273df2b3@ti.com/T/ https://github.com/torvalds/linux/blob/bee0e7762ad2c6025b9f5245c040fcc36ef2bde8/sound/soc/codecs/tas2781-fmwlib.c#L769 But I don't see any TAS25XX_CMD_DELAY command in the firmware I'm using. > > > > + prog_num = cpu_to_be32(fw_data->hdr->num_programs); > > + config_num = cpu_to_be32(fw_data->hdr->num_configs); > > + dev_info(dev, "Firmware loaded: programs %d, configs %d\n", > > + prog_num, config_num); > > + For me: release_firmware(fw) is missing from here > > + return fw_data; > > + > > +err_prog: > > + devm_kfree(dev, fw_data->prog_data); > > +err_hdr: > > + devm_kfree(dev, fw_data->hdr); > > +err_data: > > + devm_kfree(dev, fw_data); > > +err_fw: > > + release_firmware(fw); > > + > > + return NULL; > > +} > > Regards, Gergo