linux-kernel - Re: [PATCH V1 2/5] ASoC: codecs: aw88261 function for ALSA Audio Driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Jul 2023 11:09:55 +0800
From:   wangweidong.a@...nic.com
To:     krzysztof.kozlowski@...aro.org
Cc:     alsa-devel@...a-project.org, broonie@...nel.org,
        ckeepax@...nsource.cirrus.com, colin.i.king@...il.com,
        conor+dt@...nel.org, devicetree@...r.kernel.org,
        doug@...morgal.com, fido_max@...ox.ru, flatmax@...tmax.com,
        herve.codina@...tlin.com, kiseok.jo@...ndevice.com,
        krzysztof.kozlowski+dt@...aro.org, lgirdwood@...il.com,
        linux-kernel@...r.kernel.org, liweilei@...nic.com, perex@...ex.cz,
        pierre-louis.bossart@...ux.intel.com, rf@...nsource.cirrus.com,
        robh+dt@...nel.org, shumingf@...ltek.com, tiwai@...e.com,
        trix@...hat.com, wangweidong.a@...nic.com, yijiangtao@...nic.com,
        zhangjianming@...nic.com
Subject: Re: [PATCH V1 2/5] ASoC: codecs: aw88261 function for ALSA Audio Driver

Hi, Krzysztof,
Thank you very much for your advice, 
 but I have a few questions I'd like to discuss with you

On 18/07/2023 16:58, krzysztof.kozlowski@...aro.org wrote:
> On 17/07/2023 13:58, wangweidong.a@...nic.com wrote:
>> From: Weidong Wang <wangweidong.a@...nic.com>
>> 
>> The AW88261 is an I2S/TDM input, high efficiency
>> digital Smart K audio amplifier with an integrated 10.25V
>> smart boost convert

> It's the same as in patch before. This does not help and does not
> justify having one driver split into two.

I will modify the commit information and differentiate the commit 
 information for each file

>> 

...

>> +
>> +static void aw_dev_i2s_tx_enable(struct aw_device *aw_dev, bool flag)
>> +{
>> +	int ret;
>> +
>> +	if (flag) {
>> +		ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCFG1_REG,
>> +			~AW88261_I2STXEN_MASK, AW88261_I2STXEN_ENABLE_VALUE);
>> +	} else {
>> +		ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCFG1_REG,
>> +			~AW88261_I2STXEN_MASK, AW88261_I2STXEN_DISABLE_VALUE);
>> +	}

> You should not need {} here and in multiple other places.

I will delete {} as suggested

>> +	if (ret)
>> +		dev_dbg(aw_dev->dev, "%s failed", __func__);

> Why you are not handling the errors properly?

Do you need to use dev_err instead?

>> +}
>> +
>> +static void aw_dev_pwd(struct aw_device *aw_dev, bool pwd)
>> +{
>> +	int ret;
>> +
>> +	if (pwd) {
>> +		ret = regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL_REG,
>> +				~AW88261_PWDN_MASK,	AW88261_PWDN_POWER_DOWN_VALUE);
>> +	} else {
>> +		ret = regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL_REG,
>> +				~AW88261_PWDN_MASK,	AW88261_PWDN_WORKING_VALUE);
>> +	}
>> +	if (ret)
>> +		dev_dbg(aw_dev->dev, "%s failed", __func__);
>> +}
>> +

...

>> +
>> +int aw88261_dev_fw_update(struct aw_device *aw_dev)
>> +{
>> +	struct aw_prof_desc *prof_index_desc;
>> +	struct aw_sec_data_desc *sec_desc;
>> +	char *prof_name;
>> +	int ret;
>> +
>> +	prof_name = aw88261_dev_get_prof_name(aw_dev, aw_dev->prof_index);
>> +	if (!prof_name) {
>> +		dev_err(aw_dev->dev, "get prof name failed");
>> +		return -EINVAL;
>> +	}
>> +
>> +	dev_dbg(aw_dev->dev, "start update %s", prof_name);
>> +
>> +	ret = aw88261_dev_get_prof_data(aw_dev, aw_dev->prof_index, &prof_index_desc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* update reg */
>> +	sec_desc = prof_index_desc->sec_desc;
>> +	ret = aw_dev_reg_update(aw_dev, sec_desc[AW88261_DATA_TYPE_REG].data,
>> +					sec_desc[AW88261_DATA_TYPE_REG].len);
>> +	if (ret) {
>> +		dev_err(aw_dev->dev, "update reg failed");
>> +		return ret;
>> +	}
>> +
>> +	aw_dev->prof_cur = aw_dev->prof_index;
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(aw88261_dev_fw_update);

> Why do you need to export this? Where is the user?

I will delete this export

>> +
>> +int aw88261_dev_reg_update(struct aw_device *aw_dev, bool force)
>> +{
>> +	int ret;
>> +
>> +	if (force) {
>> +		aw88261_dev_soft_reset(aw_dev);
>> +		ret = aw88261_dev_fw_update(aw_dev);
>> +		if (ret < 0)
>> +			return ret;
>> +	} else {
>> +		if (aw_dev->prof_cur != aw_dev->prof_index) {
>> +			ret = aw88261_dev_fw_update(aw_dev);
>> +			if (ret < 0)
>> +				return ret;
>> +		}
>> +	}
>> +
>> +	aw_dev->prof_cur = aw_dev->prof_index;
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(aw88261_dev_reg_update);

> Same question. And in all other places as well.

This function will be called in aw88261.c, can I keep it?

Best regards,
Weidong Wang