RE: [EXTERNAL] Re: [PATCH v1] ASoc: tas2781: Add new Kontrol to set tas2563 digital gain

["Ding, Shenghao" <shenghao-ding@ti.com>]

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
>