| 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
| ||
|
Message-ID: <7cdd4825-c0da-f60e-bbef-970bea48dc95@linaro.org>
Date: Sat, 29 Jul 2023 13:28:31 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: wangweidong.a@...nic.com, lgirdwood@...il.com, broonie@...nel.org,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, perex@...ex.cz, tiwai@...e.com,
rf@...nsource.cirrus.com, shumingf@...ltek.com,
ryans.lee@...log.com, 13916275206@....com,
herve.codina@...tlin.com, ckeepax@...nsource.cirrus.com,
doug@...morgal.com, fido_max@...ox.ru, povik+lin@...ebit.org,
liweilei@...nic.com, yijiangtao@...nic.com, colin.i.king@...il.com,
trix@...hat.com, alsa-devel@...a-project.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: zhangjianming@...nic.com
Subject: Re: [PATCH V3 4/5] ASoC: codecs: aw88261 device related operation
functions
On 29/07/2023 11:12, wangweidong.a@...nic.com wrote:
> From: Weidong Wang <wangweidong.a@...nic.com>
>
> Operate the aw88261 chip, including device initialization,
> chip power-on and power-off, control volume, etc.
>
> Signed-off-by: Weidong Wang <wangweidong.a@...nic.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
Powered by blists - more mailing lists