Hi Andy Answer inline
-----Original Message----- From: Andy Shevchenko andriy.shevchenko@linux.intel.com Sent: Friday, August 9, 2024 11:18 PM To: Ding, Shenghao shenghao-ding@ti.com Cc: broonie@kernel.org; lgirdwood@gmail.com; perex@perex.cz; pierre- louis.bossart@linux.intel.com; 13916275206@139.com; zhourui@huaqin.com; alsa-devel@alsa-project.org; Salazar, Ivan i-salazar@ti.com; linux- kernel@vger.kernel.org; Chadha, Jasjot Singh j-chadha@ti.com; liam.r.girdwood@intel.com; Yue, Jaden jaden-yue@ti.com; yung- chuan.liao@linux.intel.com; Rao, Dipa dipa@ti.com; yuhsuan@google.com; Lo, Henry henry.lo@ti.com; tiwai@suse.de; Xu, Baojun baojun.xu@ti.com; soyer@irl.hu; Baojun.Xu@fpt.com; judyhsiao@google.com; Navada Kanyana, Mukund navada@ti.com; cujomalainey@google.com; Kutty, Aanya aanya@ti.com; Mahmud, Nayeem nayeem.mahmud@ti.com; savyasanchi.shukla@netradyne.com; flaviopr@microsoft.com; Ji, Jesse <jesse- ji@ti.com>; darren.ye@mediatek.com Subject: [EXTERNAL] Re: [PATCH v1] ASoc: tas2781: Add new Kontrol to set tas2563 digital gain
On Fri, Jun 28, 2024 at 12: 18: 43PM +0800, Shenghao Ding wrote: > Requriment from customer to add new kcontrol to set tas2563 digital gain > and set "Speaker Force Firmware Load" as the common kcontrol for both > tas27871 and tas2563. ZjQcmQRYFpfptBannerStart This message was sent from outside of Texas Instruments. Do not click links or open attachments unless you recognize the source of this email and know the content is safe. https://us-phishalarm- ewt.proofpoint.com/EWT/v1/G3vK!uBdnVVqmOIH3BkzsUFgO9QgWJ- u9qFEVKhhfo2Ubp9J6d6r4srvhwGiHZoYkKiQ5od83XAJWq9sCybuN7u4$ Report Suspicious
ZjQcmQRYFpfptBannerEnd On Fri, Jun 28, 2024 at 12:18:43PM +0800, Shenghao Ding wrote:
Requriment from customer to add new kcontrol to set tas2563 digital gain and set "Speaker Force Firmware Load" as the common kcontrol for both tas27871 and tas2563.
...
#include <sound/tas2781.h> #include <sound/tlv.h> #include <sound/tas2781-tlv.h>
+#include <asm/unaligned.h>
Before sound would be better, but I'm not insisting.
[ding]Apply to the new patch.
...
- ret = tasdevice_dev_bulk_read(tas_dev, 0, reg, data, 4);
Too many spaces.
...
- /* find out the member same as or closer to the current volume */
- ucontrol->value.integer.value[0] =
abs(target - ar_l) <= abs(target - ar_r) ? l : r;Why do you need to have target to be applied here? IIUC arithmetics correctly it makes no value to use target in this equation.
[ding] target save the vol register value, and the code will calculate the closest value in the tas2563_dvc_table. Sometimes, the target value is not same as the value in the table. It is wise to find the closest one. /* pow(10, db/20) * pow(2,30) */ static const unsigned char tas2563_dvc_table[][4] = { { 0X00, 0X00, 0X00, 0X00 }, /* -121.5db */ { 0X00, 0X00, 0X03, 0XBC }, /* -121.0db */ { 0X00, 0X00, 0X03, 0XF5 }, /* -120.5db */ ...
...
+out:
- mutex_unlock(&tas_dev->codec_lock);
Why not using cleanup.h?
[ding] Accept.
- return 0;
...
This all reminds me that I already gave same/similar comments in the past...
-- With Best Regards, Andy Shevchenko