| Message ID | 20230729091223.193466-1-wangweidong.a@awinic.com |
|---|---|
| Headers | show |
| Series | ASoC: codecs: Add awinic AW88261 audio amplifier driver | expand |
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 > + } > + > + 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 > + } > + > + 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 > + } > + > + /* 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 > + } > + > + aw88261 = devm_kzalloc(&i2c->dev, sizeof(struct aw88261), GFP_KERNEL); sizeof(*) > + 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. Best regards, Krzysztof
On 29/07/2023 11:12, wangweidong.a@awinic.com wrote: > From: Weidong Wang <wangweidong.a@awinic.com> > > Operate the aw88261 chip, including device initialization, > chip power-on and power-off, control volume, etc. > > Signed-off-by: Weidong Wang <wangweidong.a@awinic.com> > --- > sound/soc/codecs/aw88261/aw88261_device.c | 877 ++++++++++++++++++++++ > sound/soc/codecs/aw88261/aw88261_device.h | 79 ++ > 2 files changed, 956 insertions(+) > create mode 100644 sound/soc/codecs/aw88261/aw88261_device.c > create mode 100644 sound/soc/codecs/aw88261/aw88261_device.h > ... > + > +int aw88261_dev_stop(struct aw88261_device *aw_dev) > +{ > + if (aw_dev->aw88261_base->status == AW88261_DEV_PW_OFF) { > + dev_info(aw_dev->aw88261_base->dev, "already power off"); > + return 0; > + } > + > + aw_dev->aw88261_base->status = AW88261_DEV_PW_OFF; > + > + /* clear inturrupt */ > + aw_dev_clear_int_status(aw_dev); > + > + aw88261_dev_uls_hmute(aw_dev, true); > + /* set mute */ > + aw88261_dev_mute(aw_dev, true); > + > + /* close tx feedback */ > + aw_dev_i2s_tx_enable(aw_dev, false); > + usleep_range(AW88261_1000_US, AW88261_1000_US + 100); > + > + /* enable amppd */ > + aw_dev_amppd(aw_dev, true); > + > + /* set power down */ > + aw_dev_pwd(aw_dev, true); > + > + dev_dbg(aw_dev->dev, "pa stop success\n"); No for debug replacing tracing. We have tracing for this. > + > + return 0; > +} > + > +int aw88261_dev_init(struct aw88261_device *aw_dev, struct aw_container *aw_cfg) You already used this function in patch #3, so your order of patches is confusing. > +{ > + int ret; > + > + if ((!aw_dev) || (!aw_cfg)) { > + pr_err("aw_dev is NULL or aw_cfg is NULL"); Is this possible? If so, why? > + return -ENOMEM; > + } > + > + ret = aw88395_dev_cfg_load(aw_dev->aw88261_base, aw_cfg); > + if (ret) { > + dev_err(aw_dev->dev, "aw_dev acf parse failed"); > + return -EINVAL; > + } > + > + ret = regmap_write(aw_dev->aw88261_base->regmap, AW88261_ID_REG, AW88261_SOFT_RESET_VALUE); > + if (ret < 0) > + return ret; > + > + aw_dev->aw88261_base->fade_in_time = AW88261_1000_US / 10; > + aw_dev->aw88261_base->fade_out_time = AW88261_1000_US >> 1; > + aw_dev->aw88261_base->prof_cur = AW_INIT_PROFILE; > + aw_dev->aw88261_base->prof_index = AW_INIT_PROFILE; > + > + ret = aw_dev_fw_update(aw_dev); > + if (ret) { > + dev_err(aw_dev->dev, "fw update failed ret = %d\n", ret); > + return ret; > + } > + > + ret = aw_frcset_check(aw_dev); > + if (ret) { > + dev_err(aw_dev->dev, "aw_frcset_check failed ret = %d\n", ret); > + return ret; > + } > + > + aw_dev_clear_int_status(aw_dev); > + > + aw88261_dev_uls_hmute(aw_dev, true); > + > + aw88261_dev_mute(aw_dev, true); > + > + aw_dev_i2s_tx_enable(aw_dev, false); > + > + usleep_range(AW88261_1000_US, AW88261_1000_US + 100); > + > + aw_dev_amppd(aw_dev, true); > + > + aw_dev_pwd(aw_dev, true); > + > + return 0; > +} > + > +static void aw_parse_channel_dt(struct aw88261_device *aw_dev) > +{ > + struct device_node *np = aw_dev->aw88261_base->dev->of_node; > + u32 channel_value; > + u32 sync_enable; > + int ret; > + > + ret = of_property_read_u32(np, "sound-channel", &channel_value); > + if (ret) > + channel_value = AW88261_DEV_DEFAULT_CH; > + > + ret = of_property_read_u32(np, "sync-flag", &sync_enable); > + if (ret) > + sync_enable = false; > + > + dev_dbg(aw_dev->dev, "sync flag is %d", sync_enable); Fix style - only one space after , > + dev_dbg(aw_dev->dev, "read sound-channel value is: %d", channel_value); > + > + aw_dev->aw88261_base->channel = channel_value; > + aw_dev->phase_sync = sync_enable; > +} > + > +static int aw_dev_init(struct aw88261_device *aw_dev) > +{ > + aw_dev->aw88261_base->chip_id = AW88261_CHIP_ID; > + /* call aw device init func */ > + aw_dev->aw88261_base->acf = NULL; > + aw_dev->aw88261_base->prof_info.prof_desc = NULL; > + aw_dev->aw88261_base->prof_info.count = 0; > + aw_dev->aw88261_base->prof_info.prof_type = AW88395_DEV_NONE_TYPE_ID; > + aw_dev->aw88261_base->channel = 0; > + aw_dev->aw88261_base->fw_status = AW88261_DEV_FW_FAILED; > + > + aw_dev->aw88261_base->fade_step = AW88261_VOLUME_STEP_DB; > + aw_dev->aw88261_base->volume_desc.ctl_volume = AW88261_VOL_DEFAULT_VALUE; > + aw_dev->aw88261_base->volume_desc.mute_volume = AW88261_MUTE_VOL; > + aw_parse_channel_dt(aw_dev); > + > + return 0; > +} > + > +int aw88261_dev_set_profile_index(struct aw88261_device *aw_dev, int index) > +{ > + struct aw_device *aw88261_base = aw_dev->aw88261_base; > + > + /* check the index whether is valid */ > + if ((index >= aw88261_base->prof_info.count) || (index < 0)) > + return -EINVAL; > + /* check the index whether change */ > + if (aw88261_base->prof_index == index) > + return -EINVAL; > + > + aw88261_base->prof_index = index; > + > + return 0; > +} > + > +char *aw88261_dev_get_prof_name(struct aw88261_device *aw_dev, int index) > +{ > + struct aw_prof_info *prof_info = &aw_dev->aw88261_base->prof_info; > + struct aw_prof_desc *prof_desc; > + > + if ((index >= aw_dev->aw88261_base->prof_info.count) || (index < 0)) { > + dev_err(aw_dev->dev, "index[%d] overflow count[%d]", > + index, aw_dev->aw88261_base->prof_info.count); > + return NULL; > + } > + > + prof_desc = &aw_dev->aw88261_base->prof_info.prof_desc[index]; > + > + return prof_info->prof_name_list[prof_desc->id]; > +} > + > +int aw88261_dev_get_prof_data(struct aw88261_device *aw_dev, int index, > + struct aw_prof_desc **prof_desc) > +{ > + if ((index >= aw_dev->aw88261_base->prof_info.count) || (index < 0)) { > + dev_err(aw_dev->dev, "%s: index[%d] overflow count[%d]\n", > + __func__, index, aw_dev->aw88261_base->prof_info.count); > + return -EINVAL; > + } > + > + *prof_desc = &aw_dev->aw88261_base->prof_info.prof_desc[index]; > + > + return 0; > +} > + > +int aw88261_init(struct aw88261_device **aw_dev, struct i2c_client *i2c, struct regmap *regmap) You already used this function in patch #3, so your order of patches is confusing. > +{ > + unsigned int chip_id; > + int ret; > + > + if (*aw_dev) { > + dev_info(&i2c->dev, "it should be initialized here.\n"); How is this possible? > + } else { > + *aw_dev = devm_kzalloc(&i2c->dev, sizeof(struct aw88261_device), GFP_KERNEL); sizeof(**) > + if (!(*aw_dev)) > + return -ENOMEM; > + } > + > + (*aw_dev)->aw88261_base = > + devm_kzalloc(&i2c->dev, sizeof(struct aw_device), GFP_KERNEL); sizeof(*) > + if (!(*aw_dev)->aw88261_base) > + return -ENOMEM; > + > + (*aw_dev)->aw88261_base->i2c = i2c; I propose to use some local variable, to simplify all these assignments. > + (*aw_dev)->aw88261_base->dev = &i2c->dev; > + (*aw_dev)->aw88261_base->regmap = regmap; > + (*aw_dev)->dev = &i2c->dev; In how many places do you need to store &i2c->dev? > + > + /* read chip id */ > + ret = regmap_read((*aw_dev)->aw88261_base->regmap, AW88261_CHIP_ID_REG, &chip_id); > + if (ret) { > + dev_err((*aw_dev)->dev, "%s read chipid error. ret = %d", __func__, ret); > + return ret; > + } > + dev_info((*aw_dev)->dev, "chip id = %x\n", chip_id); "(*aw_dev)->dev" all over this function is not really readable. > + > + switch (chip_id) { > + case AW88261_CHIP_ID: > + ret = aw_dev_init((*aw_dev)); > + break; > + default: > + ret = -EINVAL; > + dev_err((*aw_dev)->dev, "unsupported device"); > + break; > + } > + > + return ret; > +} > + > +MODULE_DESCRIPTION("AW88261 device"); > +MODULE_LICENSE("GPL v2"); Wait, is this a module? Does not look complete. I already saw one module, so what is this for? For which module? Best regards, Krzysztof
On 29/07/2023 11:12, wangweidong.a@awinic.com wrote: > From: Weidong Wang <wangweidong.a@awinic.com> > > Mainly includes aw88261 register table, Makefile and Kconfig. > > Signed-off-by: Weidong Wang <wangweidong.a@awinic.com> > --- > sound/soc/codecs/Kconfig | 15 + > sound/soc/codecs/Makefile | 3 + > sound/soc/codecs/aw88261/aw88261_reg.h | 374 +++++++++++++++++++++++++ > 3 files changed, 392 insertions(+) > create mode 100644 sound/soc/codecs/aw88261/aw88261_reg.h > > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig > index c2de4ee72183..1e3526812cc8 100644 > --- a/sound/soc/codecs/Kconfig > +++ b/sound/soc/codecs/Kconfig > @@ -55,6 +55,7 @@ config SND_SOC_ALL_CODECS > imply SND_SOC_ALC5632 > imply SND_SOC_AW8738 > imply SND_SOC_AW88395 > + imply SND_SOC_AW88261 > imply SND_SOC_BT_SCO > imply SND_SOC_BD28623 > imply SND_SOC_CHV3_CODEC > @@ -640,6 +641,20 @@ config SND_SOC_AW88395 > digital Smart K audio amplifier with an integrated 10V > smart boost convert. > > +config SND_SOC_AW88261 > + tristate "Soc Audio for awinic aw88261" > + depends on I2C > + select CRC8 > + select REGMAP_I2C > + select GPIOLIB > + select SND_SOC_AW88395_LIB > + help > + This option enables support for aw88261 Smart PA. > + The awinic AW88261 is an I2S/TDM input, high efficiency > + digital Smart K audio amplifier. The output voltage of > + boost converter can be adjusted smartly according to > + the input amplitude. > + > config SND_SOC_BD28623 > tristate "ROHM BD28623 CODEC" > help > diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile > index b48a9a323b84..9df43de213f0 100644 > --- a/sound/soc/codecs/Makefile > +++ b/sound/soc/codecs/Makefile > @@ -49,6 +49,8 @@ snd-soc-aw8738-objs := aw8738.o > snd-soc-aw88395-lib-objs := aw88395/aw88395_lib.o > snd-soc-aw88395-objs := aw88395/aw88395.o \ > aw88395/aw88395_device.o > +snd-soc-aw88261-objs := aw88261/aw88261.o \ > + aw88261/aw88261_device.o > snd-soc-bd28623-objs := bd28623.o > snd-soc-bt-sco-objs := bt-sco.o > snd-soc-chv3-codec-objs := chv3-codec.o > @@ -431,6 +433,7 @@ obj-$(CONFIG_SND_SOC_ARIZONA) += snd-soc-arizona.o > obj-$(CONFIG_SND_SOC_AW8738) += snd-soc-aw8738.o > obj-$(CONFIG_SND_SOC_AW88395_LIB) += snd-soc-aw88395-lib.o > obj-$(CONFIG_SND_SOC_AW88395) +=snd-soc-aw88395.o > +obj-$(CONFIG_SND_SOC_AW88261) +=snd-soc-aw88261.o > obj-$(CONFIG_SND_SOC_BD28623) += snd-soc-bd28623.o > obj-$(CONFIG_SND_SOC_BT_SCO) += snd-soc-bt-sco.o > obj-$(CONFIG_SND_SOC_CHV3_CODEC) += snd-soc-chv3-codec.o > diff --git a/sound/soc/codecs/aw88261/aw88261_reg.h b/sound/soc/codecs/aw88261/aw88261_reg.h > new file mode 100644 > index 000000000000..7ef128a3e6ee > --- /dev/null > +++ b/sound/soc/codecs/aw88261/aw88261_reg.h > @@ -0,0 +1,374 @@ > +// SPDX-License-Identifier: GPL-2.0-only If you add the headers now, it means they are not used in any previous patches. Therefore drop the header - it is useless. Best regards, Krzysztof
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
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:29, krzysztof.kozlowski@linaro.org wrote: > On 29/07/2023 11:12, wangweidong.a@awinic.com wrote: >> From: Weidong Wang <wangweidong.a@awinic.com> >> >> Operate the aw88261 chip, including device initialization, >> chip power-on and power-off, control volume, etc. >> >> Signed-off-by: Weidong Wang <wangweidong.a@awinic.com> >> --- >> sound/soc/codecs/aw88261/aw88261_device.c | 877 ++++++++++++++++++++++ >> sound/soc/codecs/aw88261/aw88261_device.h | 79 ++ >> 2 files changed, 956 insertions(+) >> create mode 100644 sound/soc/codecs/aw88261/aw88261_device.c >> create mode 100644 sound/soc/codecs/aw88261/aw88261_device.h >> ... >> + >> +int aw88261_dev_stop(struct aw88261_device *aw_dev) >> +{ >> + if (aw_dev->aw88261_base->status == AW88261_DEV_PW_OFF) { >> + dev_info(aw_dev->aw88261_base->dev, "already power off"); >> + return 0; >> + } >> + >> + aw_dev->aw88261_base->status = AW88261_DEV_PW_OFF; >> + >> + /* clear inturrupt */ >> + aw_dev_clear_int_status(aw_dev); >> + >> + aw88261_dev_uls_hmute(aw_dev, true); >> + /* set mute */ >> + aw88261_dev_mute(aw_dev, true); >> + >> + /* close tx feedback */ >> + aw_dev_i2s_tx_enable(aw_dev, false); >> + usleep_range(AW88261_1000_US, AW88261_1000_US + 100); >> + >> + /* enable amppd */ >> + aw_dev_amppd(aw_dev, true); >> + >> + /* set power down */ >> + aw_dev_pwd(aw_dev, true); >> + >> + dev_dbg(aw_dev->dev, "pa stop success\n"); > No for debug replacing tracing. We have tracing for this. I will delete this print debug statement >> + >> + return 0; >> +} >> + >> +int aw88261_dev_init(struct aw88261_device *aw_dev, struct aw_container *aw_cfg) > You already used this function in patch #3, so your order of patches is > confusing. Do I need to change the order of patch? Do I neeed to put aw88261_device.c aw88261_device.h in patch #3 and put aw88261.c aw88261.h in patch #4? Is that how you change the order? >> +{ >> + int ret; >> + >> + if ((!aw_dev) || (!aw_cfg)) { >> + pr_err("aw_dev is NULL or aw_cfg is NULL"); > Is this possible? If so, why? Thank you very much, I will delete this judgment. >> + return -ENOMEM; >> + } >> + >> + ret = aw88395_dev_cfg_load(aw_dev->aw88261_base, aw_cfg); >> + if (ret) { >> + dev_err(aw_dev->dev, "aw_dev acf parse failed"); >> + return -EINVAL; >> + } >> + >> + ret = regmap_write(aw_dev->aw88261_base->regmap, AW88261_ID_REG, AW88261_SOFT_RESET_VALUE); >> + if (ret < 0) >> + return ret; >> + >> + aw_dev->aw88261_base->fade_in_time = AW88261_1000_US / 10; >> + aw_dev->aw88261_base->fade_out_time = AW88261_1000_US >> 1; >> + aw_dev->aw88261_base->prof_cur = AW_INIT_PROFILE; >> + aw_dev->aw88261_base->prof_index = AW_INIT_PROFILE; >> + >> + ret = aw_dev_fw_update(aw_dev); >> + if (ret) { >> + dev_err(aw_dev->dev, "fw update failed ret = %d\n", ret); >> + return ret; >> + } >> + >> + ret = aw_frcset_check(aw_dev); >> + if (ret) { >> + dev_err(aw_dev->dev, "aw_frcset_check failed ret = %d\n", ret); >> + return ret; >> + } >> + >> + aw_dev_clear_int_status(aw_dev); >> + >> + aw88261_dev_uls_hmute(aw_dev, true); >> + >> + aw88261_dev_mute(aw_dev, true); >> + >> + aw_dev_i2s_tx_enable(aw_dev, false); >> + >> + usleep_range(AW88261_1000_US, AW88261_1000_US + 100); >> + >> + aw_dev_amppd(aw_dev, true); >> + >> + aw_dev_pwd(aw_dev, true); >> + >> + return 0; >> +} >> + >> +static void aw_parse_channel_dt(struct aw88261_device *aw_dev) >> +{ >> + struct device_node *np = aw_dev->aw88261_base->dev->of_node; >> + u32 channel_value; >> + u32 sync_enable; >> + int ret; >> + >> + ret = of_property_read_u32(np, "sound-channel", &channel_value); >> + if (ret) >> + channel_value = AW88261_DEV_DEFAULT_CH; >> + >> + ret = of_property_read_u32(np, "sync-flag", &sync_enable); >> + if (ret) >> + sync_enable = false; >> + >> + dev_dbg(aw_dev->dev, "sync flag is %d", sync_enable); > Fix style - only one space after , Thank you very much. I will modify it according to your suggestion. >> + dev_dbg(aw_dev->dev, "read sound-channel value is: %d", channel_value); >> + >> + aw_dev->aw88261_base->channel = channel_value; >> + aw_dev->phase_sync = sync_enable; >> +} >> + >> +static int aw_dev_init(struct aw88261_device *aw_dev) >> +{ >> + aw_dev->aw88261_base->chip_id = AW88261_CHIP_ID; >> + /* call aw device init func */ >> + aw_dev->aw88261_base->acf = NULL; >> + aw_dev->aw88261_base->prof_info.prof_desc = NULL; >> + aw_dev->aw88261_base->prof_info.count = 0; >> + aw_dev->aw88261_base->prof_info.prof_type = AW88395_DEV_NONE_TYPE_ID; >> + aw_dev->aw88261_base->channel = 0; >> + aw_dev->aw88261_base->fw_status = AW88261_DEV_FW_FAILED; >> + >> + aw_dev->aw88261_base->fade_step = AW88261_VOLUME_STEP_DB; >> + aw_dev->aw88261_base->volume_desc.ctl_volume = AW88261_VOL_DEFAULT_VALUE; >> + aw_dev->aw88261_base->volume_desc.mute_volume = AW88261_MUTE_VOL; >> + aw_parse_channel_dt(aw_dev); >> + >> + return 0; >> +} >> + >> +int aw88261_dev_set_profile_index(struct aw88261_device *aw_dev, int index) >> +{ >> + struct aw_device *aw88261_base = aw_dev->aw88261_base; >> + >> + /* check the index whether is valid */ >> + if ((index >= aw88261_base->prof_info.count) || (index < 0)) >> + return -EINVAL; >> + /* check the index whether change */ >> + if (aw88261_base->prof_index == index) >> + return -EINVAL; >> + >> + aw88261_base->prof_index = index; >> + >> + return 0; >> +} >> + >> +char *aw88261_dev_get_prof_name(struct aw88261_device *aw_dev, int index) >> +{ >> + struct aw_prof_info *prof_info = &aw_dev->aw88261_base->prof_info; >> + struct aw_prof_desc *prof_desc; >> + >> + if ((index >= aw_dev->aw88261_base->prof_info.count) || (index < 0)) { >> + dev_err(aw_dev->dev, "index[%d] overflow count[%d]", >> + index, aw_dev->aw88261_base->prof_info.count); >> + return NULL; >> + } >> + >> + prof_desc = &aw_dev->aw88261_base->prof_info.prof_desc[index]; >> + >> + return prof_info->prof_name_list[prof_desc->id]; >> +} >> + >> +int aw88261_dev_get_prof_data(struct aw88261_device *aw_dev, int index, >> + struct aw_prof_desc **prof_desc) >> +{ >> + if ((index >= aw_dev->aw88261_base->prof_info.count) || (index < 0)) { >> + dev_err(aw_dev->dev, "%s: index[%d] overflow count[%d]\n", >> + __func__, index, aw_dev->aw88261_base->prof_info.count); >> + return -EINVAL; >> + } >> + >> + *prof_desc = &aw_dev->aw88261_base->prof_info.prof_desc[index]; >> + >> + return 0; >> +} >> + >> +int aw88261_init(struct aw88261_device **aw_dev, struct i2c_client *i2c, struct regmap *regmap) > You already used this function in patch #3, so your order of patches is > confusing. I will change the patch order as mentioned above >> +{ >> + unsigned int chip_id; >> + int ret; >> + >> + if (*aw_dev) { >> + dev_info(&i2c->dev, "it should be initialized here.\n"); > How is this possible? I will modify it according to your suggestion. >> + } else { >> + *aw_dev = devm_kzalloc(&i2c->dev, sizeof(struct aw88261_device), GFP_KERNEL); > sizeof(**) Thank you very much. I will modify it according to your suggestion. >> + if (!(*aw_dev)) >> + return -ENOMEM; >> + } >> + >> + (*aw_dev)->aw88261_base = >> + devm_kzalloc(&i2c->dev, sizeof(struct aw_device), GFP_KERNEL); > sizeof(*) Thank you very much. I will modify it according to your suggestion >> + if (!(*aw_dev)->aw88261_base) >> + return -ENOMEM; >> + >> + (*aw_dev)->aw88261_base->i2c = i2c; > I propose to use some local variable, to simplify all these assignments. Thank you very much. I will modify it according to your suggestion >> + (*aw_dev)->aw88261_base->dev = &i2c->dev; >> + (*aw_dev)->aw88261_base->regmap = regmap; >> + (*aw_dev)->dev = &i2c->dev; > In how many places do you need to store &i2c->dev? There are many places where I use dev_err(aw_dev->dev, xxx) for error printing. So I did this part of the storage. Does it make sense to change to dev_err(aw_dev->aw88261_base->dev, xx) or add a local variable and use "dev_err(aw88261_base->dev, xxx)"? >> + >> + /* read chip id */ >> + ret = regmap_read((*aw_dev)->aw88261_base->regmap, AW88261_CHIP_ID_REG, &chip_id); >> + if (ret) { >> + dev_err((*aw_dev)->dev, "%s read chipid error. ret = %d", __func__, ret); >> + return ret; >> + } >> + dev_info((*aw_dev)->dev, "chip id = %x\n", chip_id); > "(*aw_dev)->dev" all over this function is not really readable. Thank you very much, I will change it to "dev_info(&i2c->dev, xxx)"; >> + >> + switch (chip_id) { >> + case AW88261_CHIP_ID: >> + ret = aw_dev_init((*aw_dev)); >> + break; >> + default: >> + ret = -EINVAL; >> + dev_err((*aw_dev)->dev, "unsupported device"); >> + break; >> + } >> + >> + return ret; >> +} >> + >> +MODULE_DESCRIPTION("AW88261 device"); >> +MODULE_LICENSE("GPL v2"); > Wait, is this a module? Does not look complete. I already saw one > module, so what is this for? For which module? Can it be changed to MODULE_DESCRIPTION("AW88261 device lib")? The function in the aw88261_device.c file, which I used in the aw88261.c file. Best regards, Weidong Wang
Thank you very much for your advice. > On 29/07/2023 11:12, wangweidong.a@awinic.com wrote: >> From: Weidong Wang <wangweidong.a@awinic.com> >> >> Mainly includes aw88261 register table, Makefile and Kconfig. >> >> Signed-off-by: Weidong Wang <wangweidong.a@awinic.com> >> --- >> sound/soc/codecs/Kconfig | 15 + >> sound/soc/codecs/Makefile | 3 + >> sound/soc/codecs/aw88261/aw88261_reg.h | 374 +++++++++++++++++++++++++ >> 3 files changed, 392 insertions(+) >> create mode 100644 sound/soc/codecs/aw88261/aw88261_reg.h >> >> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig >> index c2de4ee72183..1e3526812cc8 100644 >> --- a/sound/soc/codecs/Kconfig >> +++ b/sound/soc/codecs/Kconfig >> @@ -55,6 +55,7 @@ config SND_SOC_ALL_CODECS >> imply SND_SOC_ALC5632 >> imply SND_SOC_AW8738 >> imply SND_SOC_AW88395 >> + imply SND_SOC_AW88261 >> imply SND_SOC_BT_SCO >> imply SND_SOC_BD28623 >> imply SND_SOC_CHV3_CODEC >> @@ -640,6 +641,20 @@ config SND_SOC_AW88395 >> digital Smart K audio amplifier with an integrated 10V >> smart boost convert. >> >> +config SND_SOC_AW88261 >> + tristate "Soc Audio for awinic aw88261" >> + depends on I2C >> + select CRC8 >> + select REGMAP_I2C >> + select GPIOLIB >> + select SND_SOC_AW88395_LIB >> + help >> + This option enables support for aw88261 Smart PA. >> + The awinic AW88261 is an I2S/TDM input, high efficiency >> + digital Smart K audio amplifier. The output voltage of >> + boost converter can be adjusted smartly according to >> + the input amplitude. >> + >> config SND_SOC_BD28623 >> tristate "ROHM BD28623 CODEC" >> help >> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile >> index b48a9a323b84..9df43de213f0 100644 >> --- a/sound/soc/codecs/Makefile >> +++ b/sound/soc/codecs/Makefile >> @@ -49,6 +49,8 @@ snd-soc-aw8738-objs := aw8738.o >> snd-soc-aw88395-lib-objs := aw88395/aw88395_lib.o >> snd-soc-aw88395-objs := aw88395/aw88395.o \ >> aw88395/aw88395_device.o >> +snd-soc-aw88261-objs := aw88261/aw88261.o \ >> + aw88261/aw88261_device.o >> snd-soc-bd28623-objs := bd28623.o >> snd-soc-bt-sco-objs := bt-sco.o >> snd-soc-chv3-codec-objs := chv3-codec.o >> @@ -431,6 +433,7 @@ obj-$(CONFIG_SND_SOC_ARIZONA) += snd-soc-arizona.o >> obj-$(CONFIG_SND_SOC_AW8738) += snd-soc-aw8738.o >> obj-$(CONFIG_SND_SOC_AW88395_LIB) += snd-soc-aw88395-lib.o >> obj-$(CONFIG_SND_SOC_AW88395) +=snd-soc-aw88395.o >> +obj-$(CONFIG_SND_SOC_AW88261) +=snd-soc-aw88261.o >> obj-$(CONFIG_SND_SOC_BD28623) += snd-soc-bd28623.o >> obj-$(CONFIG_SND_SOC_BT_SCO) += snd-soc-bt-sco.o >> obj-$(CONFIG_SND_SOC_CHV3_CODEC) += snd-soc-chv3-codec.o >> diff --git a/sound/soc/codecs/aw88261/aw88261_reg.h b/sound/soc/codecs/aw88261/aw88261_reg.h >> new file mode 100644 >> index 000000000000..7ef128a3e6ee >> --- /dev/null >> +++ b/sound/soc/codecs/aw88261/aw88261_reg.h >> @@ -0,0 +1,374 @@ >> +// SPDX-License-Identifier: GPL-2.0-only > If you add the headers now, it means they are not used in any previous > patches. Therefore drop the header - it is useless. I will combine this file with aw88261.c aw88261.h to form a patch #4. But I have a question as to whether the Makefile and Kconfig files need to be separate to make up patch #5? Best regards, Weidong Wang
On 31/07/2023 08:41, wangweidong.a@awinic.com wrote: > >>> + >>> + return 0; >>> +} >>> + >>> +int aw88261_dev_init(struct aw88261_device *aw_dev, struct aw_container *aw_cfg) > >> You already used this function in patch #3, so your order of patches is >> confusing. > > Do I need to change the order of patch? > Do I neeed to put aw88261_device.c aw88261_device.h in patch #3 and > put aw88261.c aw88261.h in patch #4? > Is that how you change the order? Your patchset should be logically ordered, so first you add bindings (because it must be before their users), then you one piece, then other etc. I understand that only the last patch will make everything buildable, but still code should be added before its user/caller. ... > >>> + >>> + switch (chip_id) { >>> + case AW88261_CHIP_ID: >>> + ret = aw_dev_init((*aw_dev)); >>> + break; >>> + default: >>> + ret = -EINVAL; >>> + dev_err((*aw_dev)->dev, "unsupported device"); >>> + break; >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +MODULE_DESCRIPTION("AW88261 device"); >>> +MODULE_LICENSE("GPL v2"); > >> Wait, is this a module? Does not look complete. I already saw one >> module, so what is this for? For which module? > > Can it be changed to MODULE_DESCRIPTION("AW88261 device lib")? If this is a module, then it can. If this is not a module, then why would you ever like to do it? > The function in the aw88261_device.c file, which I used in the aw88261.c file. Functions are not modules. Best regards, Krzysztof
Thank you very much for your advice On 31/07/2023 08:51, krzysztof.kozlowski@linaro.org wrote: > On 31/07/2023 08:41, wangweidong.a@awinic.com wrote: >> >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +int aw88261_dev_init(struct aw88261_device *aw_dev, struct aw_container *aw_cfg) >> >>> You already used this function in patch #3, so your order of patches is >>> confusing. >> >> Do I need to change the order of patch? >> Do I neeed to put aw88261_device.c aw88261_device.h in patch #3 and >> put aw88261.c aw88261.h in patch #4? >> Is that how you change the order? > Your patchset should be logically ordered, so first you add bindings > (because it must be before their users), then you one piece, then other > etc. I understand that only the last patch will make everything > buildable, but still code should be added before its user/caller. Thank you very much for your suggestion. Do I need to keep the Makefile and kconfig files in a separate patch? ... >> >>>> + >>>> + switch (chip_id) { >>>> + case AW88261_CHIP_ID: >>>> + ret = aw_dev_init((*aw_dev)); >>>> + break; >>>> + default: >>>> + ret = -EINVAL; >>>> + dev_err((*aw_dev)->dev, "unsupported device"); >>>> + break; >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +MODULE_DESCRIPTION("AW88261 device"); >>>> +MODULE_LICENSE("GPL v2"); >> >>> Wait, is this a module? Does not look complete. I already saw one >>> module, so what is this for? For which module? >> >> Can it be changed to MODULE_DESCRIPTION("AW88261 device lib")? > If this is a module, then it can. If this is not a module, then why > would you ever like to do it? >> The function in the aw88261_device.c file, which I used in the aw88261.c file. > Functions are not modules. Thank you very much for your suggestion. I will delete MODULE_DESCRIPTION and MODULE_LICENSE Best regards, Weidong Wang
From: Weidong Wang <wangweidong.a@awinic.com> The awinic AW88261 is an I2S/TDM input, high efficiency digital Smart K audio amplifier v1 -> v2: Submit the yaml file as a separate patch file Modify word capitalization in the Kconfig file Delete unused macro definitions Weidong Wang (5): ASoC: dt-bindings: Add schema for "awinic,aw88261" ASoC: codecs: Add code for bin parsing compatible with aw88261 ASoC: codecs: Add aw88261 amplifier driver ASoC: codecs: aw88261 device related operation functions ASoC: codecs: aw88261 chip register file, Kconfig and Makefile .../bindings/sound/awinic,aw88395.yaml | 4 +- sound/soc/codecs/Kconfig | 15 + sound/soc/codecs/Makefile | 3 + sound/soc/codecs/aw88261/aw88261.c | 517 +++++++++++ sound/soc/codecs/aw88261/aw88261.h | 52 ++ sound/soc/codecs/aw88261/aw88261_device.c | 877 ++++++++++++++++++ sound/soc/codecs/aw88261/aw88261_device.h | 79 ++ sound/soc/codecs/aw88261/aw88261_reg.h | 374 ++++++++ sound/soc/codecs/aw88395/aw88395_lib.c | 193 +++- sound/soc/codecs/aw88395/aw88395_reg.h | 1 + 10 files changed, 2097 insertions(+), 18 deletions(-) create mode 100644 sound/soc/codecs/aw88261/aw88261.c create mode 100644 sound/soc/codecs/aw88261/aw88261.h create mode 100644 sound/soc/codecs/aw88261/aw88261_device.c create mode 100644 sound/soc/codecs/aw88261/aw88261_device.h create mode 100644 sound/soc/codecs/aw88261/aw88261_reg.h base-commit: ffabf7c731765da3dbfaffa4ed58b51ae9c2e650