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]
Message-ID: <20230731063907.4097-1-wangweidong.a@awinic.com>
Date:   Mon, 31 Jul 2023 14:39:07 +0800
From:   wangweidong.a@...nic.com
To:     krzysztof.kozlowski@...aro.org
Cc:     13916275206@....com, 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,
        herve.codina@...tlin.com, krzysztof.kozlowski+dt@...aro.org,
        lgirdwood@...il.com, linux-kernel@...r.kernel.org,
        liweilei@...nic.com, perex@...ex.cz, povik+lin@...ebit.org,
        rf@...nsource.cirrus.com, robh+dt@...nel.org, ryans.lee@...log.com,
        shumingf@...ltek.com, tiwai@...e.com, trix@...hat.com,
        wangweidong.a@...nic.com, yijiangtao@...nic.com,
        zhangjianming@...nic.com
Subject: Re: [PATCH V3 3/5] ASoC: codecs: Add aw88261 amplifier driver

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

On 29/07/2023 19:22, krzysztof.kozlowski@...aro.org wrote:
> On 29/07/2023 11:12, wangweidong.a@...nic.com wrote:
>> From: Weidong Wang <wangweidong.a@...nic.com>
>> 
>> Add i2c and amplifier registration for
>> aw88261 and their associated operation functions.
>> 
>> Signed-off-by: Weidong Wang <wangweidong.a@...nic.com>
>> ---
>>  sound/soc/codecs/aw88261/aw88261.c | 517 +++++++++++++++++++++++++++++
>>  sound/soc/codecs/aw88261/aw88261.h |  52 +++
>>  2 files changed, 569 insertions(+)
>>  create mode 100644 sound/soc/codecs/aw88261/aw88261.c
>>  create mode 100644 sound/soc/codecs/aw88261/aw88261.h
>> 


>> +
>> +static int aw88261_request_firmware_file(struct aw88261 *aw88261)
>> +{
>> +	const struct firmware *cont = NULL;
>> +	int ret;
>> +
>> +	aw88261->aw_pa->aw88261_base->fw_status = AW88261_DEV_FW_FAILED;
>> +
>> +	ret = request_firmware(&cont, AW88261_ACF_FILE, aw88261->aw_pa->dev);
>> +	if (ret < 0) {
>> +		dev_err_probe(aw88261->aw_pa->dev, ret, "load [%s] failed!", AW88261_ACF_FILE);
>> +		return ret;

> That's not how you use dev_err_probe. Instead: return dev_err_probe

Thank you very much. I will changed it to
"return dev_err_probe(aw88261->aw_pa->dev, ret, "load [%s] failed!", AW88261_ACF_FILE)".

>> +	}
>> +
>> +	dev_info(aw88261->aw_pa->dev, "loaded %s - size: %zu\n",
>> +			AW88261_ACF_FILE, cont ? cont->size : 0);
>> +
>> +	aw88261->aw_cfg = devm_kzalloc(aw88261->aw_pa->dev, cont->size + sizeof(int), GFP_KERNEL);
>> +	if (!aw88261->aw_cfg) {
>> +		release_firmware(cont);
>> +		return -ENOMEM;
>> +	}
>> +	aw88261->aw_cfg->len = (int)cont->size;
>> +	memcpy(aw88261->aw_cfg->data, cont->data, cont->size);
>> +	release_firmware(cont);
>> +
>> +	ret = aw88395_dev_load_acf_check(aw88261->aw_pa->aw88261_base, aw88261->aw_cfg);
>> +	if (ret < 0) {
>> +		dev_err_probe(aw88261->aw_pa->dev, ret, "load [%s] failed !", AW88261_ACF_FILE);
>> +		return ret;

> return dev_err_probe

I want to use "dev_err" here, Because the aw88395_dev_load_acf_check
function only checks the bin file, it does not involve the application of resources

>> +	}
>> +
>> +	mutex_lock(&aw88261->lock);
>> +	/* aw device init */
>> +	ret = aw88261_dev_init(aw88261->aw_pa, aw88261->aw_cfg);
>> +	if (ret < 0)
>> +		dev_err(aw88261->aw_pa->dev, "dev init failed");
>> +	mutex_unlock(&aw88261->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int aw88261_codec_probe(struct snd_soc_component *component)
>> +{
>> +	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
>> +	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(component);
>> +	int ret;
>> +
>> +	INIT_DELAYED_WORK(&aw88261->start_work, aw88261_startup_work);
>> +
>> +	ret = aw88261_request_firmware_file(aw88261);
>> +	if (ret < 0) {
>> +		dev_err_probe(aw88261->aw_pa->dev, ret, "aw88261_request_firmware_file failed\n");
>> +		return ret;

> return dev_err_probe

Thank you very much. I'll change it to "return dev_err_probe"

>> +	}
>> +
>> +	/* add widgets */
>> +	ret = snd_soc_dapm_new_controls(dapm, aw88261_dapm_widgets,
>> +							ARRAY_SIZE(aw88261_dapm_widgets));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* add route */
>> +	ret = snd_soc_dapm_add_routes(dapm, aw88261_audio_map,
>> +							ARRAY_SIZE(aw88261_audio_map));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = snd_soc_add_component_controls(component, aw88261_controls,
>> +							ARRAY_SIZE(aw88261_controls));
>> +
>> +	return ret;
>> +}
>> +
>> +static void aw88261_codec_remove(struct snd_soc_component *aw_codec)
>> +{
>> +	struct aw88261 *aw88261 = snd_soc_component_get_drvdata(aw_codec);
>> +
>> +	cancel_delayed_work_sync(&aw88261->start_work);
>> +}
>> +
>> +static const struct snd_soc_component_driver soc_codec_dev_aw88261 = {
>> +	.probe = aw88261_codec_probe,
>> +	.remove = aw88261_codec_remove,
>> +};
>> +
>> +static void aw88261_hw_reset(struct aw88261 *aw88261)
>> +{
>> +	gpiod_set_value_cansleep(aw88261->reset_gpio, 0);
>> +	usleep_range(AW88261_1000_US, AW88261_1000_US + 10);
>> +	gpiod_set_value_cansleep(aw88261->reset_gpio, 1);
>> +	usleep_range(AW88261_1000_US, AW88261_1000_US + 10);
>> +}
>> +
>> +static int aw88261_i2c_probe(struct i2c_client *i2c)
>> +{
>> +	struct aw88261 *aw88261;
>> +	int ret;
>> +
>> +	ret = i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C);
>> +	if (!ret) {
>> +		dev_err_probe(&i2c->dev, ret, "check_functionality failed");
>> +		return -ENXIO;

> return dev_err_probe

Thank you very much. I'll change it to "return dev_err_probe"

>> +	}
>> +
>> +	aw88261 = devm_kzalloc(&i2c->dev, sizeof(struct aw88261), GFP_KERNEL);

> sizeof(*)

Thank you very much. I will change it to 
"devm_kzalloc(&i2c->dev, sizeof(*aw88261), GFP_KERNEL)".

>> +	if (!aw88261)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&aw88261->lock);
>> +
>> +	i2c_set_clientdata(i2c, aw88261);
>> +
>> +	aw88261->reset_gpio = devm_gpiod_get_optional(&i2c->dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(aw88261->reset_gpio))
>> +		dev_info(&i2c->dev, "reset gpio not defined\n");
>> +	else
>> +		aw88261_hw_reset(aw88261);
>> +
>> +	aw88261->regmap = devm_regmap_init_i2c(i2c, &aw88261_remap_config);
>> +	if (IS_ERR(aw88261->regmap)) {
>> +		ret = PTR_ERR(aw88261->regmap);
>> +		dev_err_probe(&i2c->dev, ret, "failed to init regmap: %d\n", ret);
>> +		return ret;

> return dev_err_probe

> I asked you about this in your first version. I explicitly wrote "return
> dev_err_probe", not some other syntax.

Thank you very much. I'll change it to "return dev_err_probe"

Best regards,
Weidong Wang

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ