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