Re: [PATCH V1 2/5] ASoC: codecs: aw88261 function for ALSA Audio Driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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