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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 19 Jul 2023 08:24:39 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     wangweidong.a@...nic.com
Cc:     alsa-devel@...a-project.org, broonie@...nel.org,
        ckeepax@...nsource.cirrus.com, colin.i.king@...il.com,
        conor+dt@...nel.org, devicetree@...r.kernel.org,
        doug@...morgal.com, fido_max@...ox.ru, flatmax@...tmax.com,
        herve.codina@...tlin.com, kiseok.jo@...ndevice.com,
        krzysztof.kozlowski+dt@...aro.org, lgirdwood@...il.com,
        linux-kernel@...r.kernel.org, liweilei@...nic.com, perex@...ex.cz,
        pierre-louis.bossart@...ux.intel.com, rf@...nsource.cirrus.com,
        robh+dt@...nel.org, shumingf@...ltek.com, tiwai@...e.com,
        trix@...hat.com, yijiangtao@...nic.com, zhangjianming@...nic.com
Subject: Re: [PATCH V1 2/5] ASoC: codecs: aw88261 function for ALSA Audio
 Driver

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