linux-kernel - Re: [PATCH v5 2/8] ASoC: wcd9335: add support to wcd9335 codec

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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 23 Jan 2019 10:11:42 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     robh+dt@...nel.org, lgirdwood@...il.com, bgoswami@...eaurora.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        vkoul@...nel.org, alsa-devel@...a-project.org
Subject: Re: [PATCH v5 2/8] ASoC: wcd9335: add support to wcd9335 codec

Thanks for the review,

On 22/01/2019 18:41, Mark Brown wrote:
> On Thu, Jan 10, 2019 at 03:06:10PM +0000, Srinivas Kandagatla wrote:
> 
> This all looks good apart from a couple of small things that should be
> easy to fix:
> 
>> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
>> index 62bdb7e333b8..1c4904940621 100644
>> --- a/sound/soc/codecs/Kconfig
>> +++ b/sound/soc/codecs/Kconfig
>> @@ -1100,6 +1100,12 @@ config SND_SOC_UDA1380
>>           tristate
>>   	depends on I2C
>>   
>> +config SND_SOC_WCD9335
>> +	tristate "WCD9335 Codec"
>> +	depends on SLIMBUS
>> +	select REGMAP_SLIMBUS
>> +        tristate
>> +
>>   config SND_SOC_WL1273
>>   	tristate
>>   
> 
> You should add this to SND_SOC_ALL_CODECS.
Sure, I will add this in next version.
> 
>> +static irqreturn_t wcd9335_slimbus_irq(int irq, void *data)
>> +{
>> +	struct wcd9335_codec *wcd = data;
>> +	unsigned long status = 0;
>> +	int i, j, port_id;
>> +	unsigned int val, int_val = 0;
>> +	bool tx;
>> +	unsigned short reg = 0;
>> +
>> +	for (i = WCD9335_SLIM_PGD_PORT_INT_STATUS_RX_0, j = 0;
>> +	     i <= WCD9335_SLIM_PGD_PORT_INT_STATUS_TX_1; i++, j++) {
>> +		regmap_read(wcd->if_regmap, i, &val);
>> +		status |= ((u32)val << (8 * j));
>> +	}
>> +
>> +	for_each_set_bit(j, &status, 32) {
>> +		tx = (j >= 16 ? true : false);
>> +		port_id = (tx ? j - 16 : j);
> 
> ...
> 
>> +	return IRQ_HANDLED;
>> +}
> 
> This will report that it handled the interrupt unconditionally, this
> means that if the interrupt fires in error or there's some bug the
> interrupt will never be acknowledged (it looks like the chip requires
> acks?) but the interrupt core won't be able to notice and handle the
> problem as effectively as it could've done.  It's better to report
> IRQ_NONE if nothing as handled so that the core can do error handling.
I agree, Will fix this in next version!

thanks,
srini
>