[PATCH v3 1/2] ASoC: cs35l41: CS35L41 Boosted Smart Amplifier

[David Rhodes]

On 7/5/21 2:20 PM, Mark Brown wrote:

Thanks for the review. I believe I can fix all of these in the next 
revision.

> On Fri, Jul 02, 2021 at 03:51:26PM -0500, David Rhodes wrote:

>> +	struct regulator_bulk_data supplies[2];
>> +	int num_supplies;
> 
> Why might the number of supplies vary?
> 

Good point. This is set by the size of a const array so I will replace 
this with a #define.

> 
>> +	/* Check to see if unmasked bits are active */
>> +	if (!(status[0] & ~masks[0]) && !(status[1] & ~masks[1]) &&
>> +		!(status[2] & ~masks[2]) && !(status[3] & ~masks[3]))
>> +		return IRQ_NONE;
> 
> 
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> This seems to handle any asserted interrupt, is it clear on read?
> 

Theoretically every unmasked interrupt source should have a handler 
here, so the function should have exited early if there was nothing handled.
I think it's easier to read and more explicitly enforced if I change it 
so that the return code is only set to IRQ_HANDLED inside the conditions 
where the bits are actually cleared.

>> +	case SND_SOC_DAPM_POST_PMD:
>> +		regmap_read(cs35l41->regmap, CS35L41_PWR_CTRL1, &val);
>> +		if (val & CS35L41_GLOBAL_EN_MASK) {
>> +			regmap_update_bits(cs35l41->regmap, CS35L41_PWR_CTRL1,
>> +					CS35L41_GLOBAL_EN_MASK, 0);
> 
> I can't see any references to GLOBAL_EN outside this function, why might
> it not be set?
> 

This check prevents an incorrect 'PDN Failed' message if DAPM 
initializes and turns the widget off at boot (with no prior power-up).


>> +	ret = devm_request_threaded_irq(cs35l41->dev, cs35l41->irq, NULL,
>> +			cs35l41_irq, IRQF_ONESHOT | IRQF_SHARED | irq_pol,
>> +			"cs35l41", cs35l41);
>> +
>> +	/* CS35L41 needs INT for PDN_DONE */
>> +	if (ret != 0) {
>> +		dev_err(cs35l41->dev, "Failed to request IRQ: %d\n", ret);
>> +		ret = -ENODEV;
>> +		goto err;
>> +	}
>> +
>> +	/* Set interrupt masks for critical errors */
>> +	regmap_write(cs35l41->regmap, CS35L41_IRQ1_MASK1,
>> +			CS35L41_INT1_MASK_DEFAULT);
> 
> Shouldn't this be configured prior to requesting interrupts or might
> there be a race?
> 

Should not be any problems with unmasking before the request as long as 
the pin configuration is also done beforehand.


Thanks,
David