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@linaro.org wrote:
On 29/07/2023 11:12, wangweidong.a@awinic.com wrote:
From: Weidong Wang wangweidong.a@awinic.com
Add i2c and amplifier registration for aw88261 and their associated operation functions.
Signed-off-by: Weidong Wang wangweidong.a@awinic.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