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: <4ef0d83e-b0c9-1f6e-c1a4-24d650630fe4@linaro.org>
Date:   Mon, 17 Jul 2023 22:39:07 +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 V1 2/5] ASoC: codecs: aw88261 function for ALSA Audio
 Driver
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.co, shumingf@realtek.com,
        herve.codina@bootlin.com, flatmax@flatmax.com,
        ckeepax@opensource.cirrus.com, doug@schmorgal.com,
        fido_max@inbox.ru, pierre-louis.bossart@linux.intel.com,
        kiseok.jo@irondevice.com, liweilei@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:     yijiangtao@awinic.com, zhangjianming@awinic.com
References: <20230717115845.297991-1-wangweidong.a@awinic.com>
 <20230717115845.297991-3-wangweidong.a@awinic.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230717115845.297991-3-wangweidong.a@awinic.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 17/07/2023 13:58, wangweidong.a@awinic.com wrote:
> From: Weidong Wang <wangweidong.a@awinic.com>
> 
> The AW88261 is an I2S/TDM input, high efficiency
> digital Smart K audio amplifier with an integrated 10.25V
> smart boost convert

It's the same as in patch before. This does not help and does not
justify having one driver split into two.

> 

...

> +
> +static void aw_dev_i2s_tx_enable(struct aw_device *aw_dev, bool flag)
> +{
> +	int ret;
> +
> +	if (flag) {
> +		ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCFG1_REG,
> +			~AW88261_I2STXEN_MASK, AW88261_I2STXEN_ENABLE_VALUE);
> +	} else {
> +		ret = regmap_update_bits(aw_dev->regmap, AW88261_I2SCFG1_REG,
> +			~AW88261_I2STXEN_MASK, AW88261_I2STXEN_DISABLE_VALUE);
> +	}

You should not need {} here and in multiple other places.

> +	if (ret)
> +		dev_dbg(aw_dev->dev, "%s failed", __func__);

Why you are not handling the errors properly?

> +}
> +
> +static void aw_dev_pwd(struct aw_device *aw_dev, bool pwd)
> +{
> +	int ret;
> +
> +	if (pwd) {
> +		ret = regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL_REG,
> +				~AW88261_PWDN_MASK,	AW88261_PWDN_POWER_DOWN_VALUE);
> +	} else {
> +		ret = regmap_update_bits(aw_dev->regmap, AW88261_SYSCTRL_REG,
> +				~AW88261_PWDN_MASK,	AW88261_PWDN_WORKING_VALUE);
> +	}
> +	if (ret)
> +		dev_dbg(aw_dev->dev, "%s failed", __func__);
> +}
> +

...

> +
> +int aw88261_dev_fw_update(struct aw_device *aw_dev)
> +{
> +	struct aw_prof_desc *prof_index_desc;
> +	struct aw_sec_data_desc *sec_desc;
> +	char *prof_name;
> +	int ret;
> +
> +	prof_name = aw88261_dev_get_prof_name(aw_dev, aw_dev->prof_index);
> +	if (!prof_name) {
> +		dev_err(aw_dev->dev, "get prof name failed");
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(aw_dev->dev, "start update %s", prof_name);
> +
> +	ret = aw88261_dev_get_prof_data(aw_dev, aw_dev->prof_index, &prof_index_desc);
> +	if (ret)
> +		return ret;
> +
> +	/* update reg */
> +	sec_desc = prof_index_desc->sec_desc;
> +	ret = aw_dev_reg_update(aw_dev, sec_desc[AW88261_DATA_TYPE_REG].data,
> +					sec_desc[AW88261_DATA_TYPE_REG].len);
> +	if (ret) {
> +		dev_err(aw_dev->dev, "update reg failed");
> +		return ret;
> +	}
> +
> +	aw_dev->prof_cur = aw_dev->prof_index;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(aw88261_dev_fw_update);

Why do you need to export this? Where is the user?

> +
> +int aw88261_dev_reg_update(struct aw_device *aw_dev, bool force)
> +{
> +	int ret;
> +
> +	if (force) {
> +		aw88261_dev_soft_reset(aw_dev);
> +		ret = aw88261_dev_fw_update(aw_dev);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		if (aw_dev->prof_cur != aw_dev->prof_index) {
> +			ret = aw88261_dev_fw_update(aw_dev);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +
> +	aw_dev->prof_cur = aw_dev->prof_index;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(aw88261_dev_reg_update);

Same question. And in all other places as well.


Best regards,
Krzysztof