Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7cdd4825-c0da-f60e-bbef-970bea48dc95@linaro.org>
Date:   Sat, 29 Jul 2023 13:28:31 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH V3 4/5] ASoC: codecs: aw88261 device related operation
 functions
Content-Language: en-US
To:     wangweidong.a@awinic.com, lgirdwood@gmail.com, broonie@kernel.org,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        conor+dt@kernel.org, perex@perex.cz, tiwai@suse.com,
        rf@opensource.cirrus.com, shumingf@realtek.com,
        ryans.lee@analog.com, 13916275206@139.com,
        herve.codina@bootlin.com, ckeepax@opensource.cirrus.com,
        doug@schmorgal.com, fido_max@inbox.ru, povik+lin@cutebit.org,
        liweilei@awinic.com, yijiangtao@awinic.com, colin.i.king@gmail.com,
        trix@redhat.com, alsa-devel@alsa-project.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     zhangjianming@awinic.com
References: <20230729091223.193466-1-wangweidong.a@awinic.com>
 <20230729091223.193466-5-wangweidong.a@awinic.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230729091223.193466-5-wangweidong.a@awinic.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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