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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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
>