On 19/07/2023 05:09, wangweidong.a@xxxxxxxxxx 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@xxxxxxxxxx wrote:
>> On 17/07/2023 13:58, wangweidong.a@xxxxxxxxxx wrote:
>>> From: Weidong Wang <wangweidong.a@xxxxxxxxxx>
>>>
>>> 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
