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: <4096c968-aa77-5638-7f67-2a478d41ded3@linaro.org>
Date:   Wed, 19 Jul 2023 08:24:39 +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
Cc:     alsa-devel@alsa-project.org, broonie@kernel.org,
        ckeepax@opensource.cirrus.com, colin.i.king@gmail.com,
        conor+dt@kernel.org, devicetree@vger.kernel.org,
        doug@schmorgal.com, fido_max@inbox.ru, flatmax@flatmax.com,
        herve.codina@bootlin.com, kiseok.jo@irondevice.com,
        krzysztof.kozlowski+dt@linaro.org, lgirdwood@gmail.com,
        linux-kernel@vger.kernel.org, liweilei@awinic.com, perex@perex.cz,
        pierre-louis.bossart@linux.intel.com, rf@opensource.cirrus.com,
        robh+dt@kernel.org, shumingf@realtek.com, tiwai@suse.com,
        trix@redhat.com, yijiangtao@awinic.com, zhangjianming@awinic.com
References: <4ef0d83e-b0c9-1f6e-c1a4-24d650630fe4@linaro.org>
 <20230719030955.41489-1-wangweidong.a@awinic.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230719030955.41489-1-wangweidong.a@awinic.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 19/07/2023 05:09, wangweidong.a@awinic.com wrote:
> Hi, Krzysztof,
> Thank you very much for your advice, 
>  but I have a few questions I'd like to discuss with you
> 
> On 18/07/2023 16:58, krzysztof.kozlowski@linaro.org wrote:
>> 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.
> 
> I will modify the commit information and differentiate the commit 
>  information for each file
> 
>>>
> 
> ...
> 
>>> +
>>> +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.
> 
> I will delete {} as suggested
> 
>>> +	if (ret)
>>> +		dev_dbg(aw_dev->dev, "%s failed", __func__);
> 
>> Why you are not handling the errors properly?
> 
> Do you need to use dev_err instead?

No, return err and handle it somehow (if it is reasonable).

..

>>> +	} 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.
> 
> This function will be called in aw88261.c, can I keep it?

Not really. Being used in other unit does not mean you need export.

Best regards,
Krzysztof