Re: [PATCH V3 4/5] ASoC: codecs: aw88261 device related operation functions

[wangweidong . a]

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@xxxxxxxxxx wrote:
> On 29/07/2023 11:12, wangweidong.a@xxxxxxxxxx wrote:
>> From: Weidong Wang <wangweidong.a@xxxxxxxxxx>
>> 
>> Operate the aw88261 chip, including device initialization,
>> chip power-on and power-off, control volume, etc.
>> 
>> Signed-off-by: Weidong Wang <wangweidong.a@xxxxxxxxxx>
>> ---
>>  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