| Message ID | 20231120141734.76679-1-mstrozek@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] ASoC: cs43130: Allow driver to work without IRQ connection | expand |
On Mon, Nov 20, 2023 at 02:17:34PM +0000, Maciej Strozek wrote: > + if (cs43130->has_irq_line) { > + ret = wait_for_completion_timeout(to_poll, msecs_to_jiffies(time)); > + } else { If you just put a return here then you don't need the else and can reduce the intentation level of the rest of the function, making it more legible. > + if (to_poll == &cs43130->xtal_rdy) { > + offset = 0; > + flag = CS43130_XTAL_RDY_INT; > + } else if (to_poll == &cs43130->pll_rdy) { > + offset = 0; > + flag = CS43130_PLL_RDY_INT; > + } else if (to_poll == &cs43130->hpload_evt) { > + offset = 3; > + flag = CS43130_HPLOAD_NO_DC_INT | CS43130_HPLOAD_UNPLUG_INT | > + CS43130_HPLOAD_OOR_INT | CS43130_HPLOAD_AC_INT | > + CS43130_HPLOAD_DC_INT | CS43130_HPLOAD_ON_INT | > + CS43130_HPLOAD_OFF_INT; > + } else { > + return 0; > + } Is it a bug to call this function without to_poll set to something known? This will just silently ignore it which seems wrong and is inconsitent with the handling in the interrupt case which will wait for the the completion to be signalled and report a timeout on error. > @@ -2545,8 +2579,10 @@ static int cs43130_i2c_probe(struct i2c_client *client) > IRQF_ONESHOT | IRQF_TRIGGER_LOW, > "cs43130", cs43130); > if (ret != 0) { > - dev_err(cs43130->dev, "Failed to request IRQ: %d\n", ret); > - goto err; > + dev_dbg(cs43130->dev, "Failed to request IRQ: %d, will poll instead\n", ret); > + cs43130->has_irq_line = 0; > + } else { > + cs43130->has_irq_line = 1; This will treat probe deferral as the interrupt not being supplied, and will squash even real errors silently - it should probably check for both the specific error the clock API returns when no clock is provided by the firmware and probe deferral and handle both specifically.
W dniu 20/11/2023 o 14:40, Mark Brown pisze: > On Mon, Nov 20, 2023 at 02:17:34PM +0000, Maciej Strozek wrote: >> + if (to_poll == &cs43130->xtal_rdy) { >> + offset = 0; >> + flag = CS43130_XTAL_RDY_INT; >> + } else if (to_poll == &cs43130->pll_rdy) { >> + offset = 0; >> + flag = CS43130_PLL_RDY_INT; >> + } else if (to_poll == &cs43130->hpload_evt) { >> + offset = 3; >> + flag = CS43130_HPLOAD_NO_DC_INT | CS43130_HPLOAD_UNPLUG_INT | >> + CS43130_HPLOAD_OOR_INT | CS43130_HPLOAD_AC_INT | >> + CS43130_HPLOAD_DC_INT | CS43130_HPLOAD_ON_INT | >> + CS43130_HPLOAD_OFF_INT; >> + } else { >> + return 0; >> + } > > Is it a bug to call this function without to_poll set to something > known? This will just silently ignore it which seems wrong and is > inconsitent with the handling in the interrupt case which will wait for > the the completion to be signalled and report a timeout on error. > In interrupt case 0 means timeout (and calling function should expect 0 as error/timeout), so the only inconsistency I see is in not waiting before returning a timeout, but that would be needlessly wasting time? Do you think adding a debug print or a comment would help here?
On Mon, Nov 20, 2023 at 03:46:26PM +0000, Maciej Strozek wrote: > W dniu 20/11/2023 o 14:40, Mark Brown pisze: > > > + } else { > > > + return 0; > > > + } > > Is it a bug to call this function without to_poll set to something > > known? This will just silently ignore it which seems wrong and is > > inconsitent with the handling in the interrupt case which will wait for > > the the completion to be signalled and report a timeout on error. > In interrupt case 0 means timeout (and calling function should expect 0 as > error/timeout), so the only inconsistency I see is in not waiting before > returning a timeout, but that would be needlessly wasting time? > Do you think adding a debug print or a comment would help here? It seems like a clear code bug if this is ever called with an unknown completion, I'd expect a WARN_ON_ONCE() there. The lack of a delay is potentially going to affect how any error handling works which doesn't feel ideal though the users look fine right now.
On Mon, Nov 20, 2023 at 03:54:14PM +0000, Mark Brown wrote: > On Mon, Nov 20, 2023 at 03:46:26PM +0000, Maciej Strozek wrote: > > W dniu 20/11/2023 o 14:40, Mark Brown pisze: > > > > > + } else { > > > > + return 0; > > > > + } > > > > Is it a bug to call this function without to_poll set to something > > > known? This will just silently ignore it which seems wrong and is > > > inconsitent with the handling in the interrupt case which will wait for > > > the the completion to be signalled and report a timeout on error. > > > In interrupt case 0 means timeout (and calling function should expect 0 as > > error/timeout), so the only inconsistency I see is in not waiting before > > returning a timeout, but that would be needlessly wasting time? > > Do you think adding a debug print or a comment would help here? > > It seems like a clear code bug if this is ever called with an unknown > completion, I'd expect a WARN_ON_ONCE() there. The lack of a delay is > potentially going to affect how any error handling works which doesn't > feel ideal though the users look fine right now. I guess perhaps another option might be to not stick so strictly to the wait_for_completion_timeout API. This function could return an -EINVAL here and a -ETIMEDOUT for a timeout then the callers could be updated accordingly. Thanks, Charles
On Mon, Nov 20, 2023 at 04:16:38PM +0000, Charles Keepax wrote: > On Mon, Nov 20, 2023 at 03:54:14PM +0000, Mark Brown wrote: > > It seems like a clear code bug if this is ever called with an unknown > > completion, I'd expect a WARN_ON_ONCE() there. The lack of a delay is > > potentially going to affect how any error handling works which doesn't > > feel ideal though the users look fine right now. > I guess perhaps another option might be to not stick so strictly > to the wait_for_completion_timeout API. This function could > return an -EINVAL here and a -ETIMEDOUT for a timeout then the > callers could be updated accordingly. Yes, that'd help with clarity in terms of the interface - the completion API is a bit non-standard here.
On Mon, 20 Nov 2023 14:17:34 +0000, Maciej Strozek wrote: > Add a polling mechanism that will keep the driver operational even in > absence of physical IRQ connection. If IRQ line is detected, the driver > will continue working as usual, in case of missing IRQ line it will > fallback to the polling mechanism introduced in this change. > This will support users which choose not to connect an IRQ line as it > is not critical to part's operation. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: cs43130: Allow driver to work without IRQ connection commit: 009eab8baa4d46c2b20d0c2c1cbdba61c81829e4 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Mon, Nov 20, 2023 at 07:18:51PM +0000, Mark Brown wrote: > On Mon, 20 Nov 2023 14:17:34 +0000, Maciej Strozek wrote: > > Add a polling mechanism that will keep the driver operational even in > > absence of physical IRQ connection. If IRQ line is detected, the driver > > will continue working as usual, in case of missing IRQ line it will > > fallback to the polling mechanism introduced in this change. > > This will support users which choose not to connect an IRQ line as it > > is not critical to part's operation. > > > > [...] > > Applied to > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next > > Thanks! Sorry, this was done in error - dropped it. Sorry for the noise.
diff --git a/sound/soc/codecs/cs43130.c b/sound/soc/codecs/cs43130.c index fd39328579fb..1e7c32eedc7b 100644 --- a/sound/soc/codecs/cs43130.c +++ b/sound/soc/codecs/cs43130.c @@ -326,6 +326,43 @@ static int cs43130_set_pll(struct snd_soc_component *component, int pll_id, int return ret; } +static int cs43130_wait_for_completion(struct cs43130_private *cs43130, struct completion *to_poll, + int time) +{ + int stickies, offset, flag; + int ret = 0; + + if (cs43130->has_irq_line) { + ret = wait_for_completion_timeout(to_poll, msecs_to_jiffies(time)); + } else { + if (to_poll == &cs43130->xtal_rdy) { + offset = 0; + flag = CS43130_XTAL_RDY_INT; + } else if (to_poll == &cs43130->pll_rdy) { + offset = 0; + flag = CS43130_PLL_RDY_INT; + } else if (to_poll == &cs43130->hpload_evt) { + offset = 3; + flag = CS43130_HPLOAD_NO_DC_INT | CS43130_HPLOAD_UNPLUG_INT | + CS43130_HPLOAD_OOR_INT | CS43130_HPLOAD_AC_INT | + CS43130_HPLOAD_DC_INT | CS43130_HPLOAD_ON_INT | + CS43130_HPLOAD_OFF_INT; + } else { + return 0; + } + + ret = regmap_read_poll_timeout(cs43130->regmap, CS43130_INT_STATUS_1 + offset, + stickies, (stickies & flag), + 1000, time * 1000); + + /* + * Return 0 for an timeout/error to be consistent with wait_for_completion_timeout + */ + ret = !ret; + } + return ret; +} + static int cs43130_change_clksrc(struct snd_soc_component *component, enum cs43130_mclk_src_sel src) { @@ -364,8 +401,7 @@ static int cs43130_change_clksrc(struct snd_soc_component *component, CS43130_XTAL_RDY_INT_MASK, 0); regmap_update_bits(cs43130->regmap, CS43130_PWDN_CTL, CS43130_PDN_XTAL_MASK, 0); - ret = wait_for_completion_timeout(&cs43130->xtal_rdy, - msecs_to_jiffies(100)); + ret = cs43130_wait_for_completion(cs43130, &cs43130->xtal_rdy, 100); regmap_update_bits(cs43130->regmap, CS43130_INT_MASK_1, CS43130_XTAL_RDY_INT_MASK, 1 << CS43130_XTAL_RDY_INT_SHIFT); @@ -400,8 +436,7 @@ static int cs43130_change_clksrc(struct snd_soc_component *component, CS43130_XTAL_RDY_INT_MASK, 0); regmap_update_bits(cs43130->regmap, CS43130_PWDN_CTL, CS43130_PDN_XTAL_MASK, 0); - ret = wait_for_completion_timeout(&cs43130->xtal_rdy, - msecs_to_jiffies(100)); + ret = cs43130_wait_for_completion(cs43130, &cs43130->xtal_rdy, 100); regmap_update_bits(cs43130->regmap, CS43130_INT_MASK_1, CS43130_XTAL_RDY_INT_MASK, 1 << CS43130_XTAL_RDY_INT_SHIFT); @@ -416,8 +451,7 @@ static int cs43130_change_clksrc(struct snd_soc_component *component, CS43130_PLL_RDY_INT_MASK, 0); regmap_update_bits(cs43130->regmap, CS43130_PWDN_CTL, CS43130_PDN_PLL_MASK, 0); - ret = wait_for_completion_timeout(&cs43130->pll_rdy, - msecs_to_jiffies(100)); + ret = cs43130_wait_for_completion(cs43130, &cs43130->pll_rdy, 100); regmap_update_bits(cs43130->regmap, CS43130_INT_MASK_1, CS43130_PLL_RDY_INT_MASK, 1 << CS43130_PLL_RDY_INT_SHIFT); @@ -2040,8 +2074,8 @@ static int cs43130_hpload_proc(struct cs43130_private *cs43130, regmap_multi_reg_write(cs43130->regmap, seq, seq_size); - ret = wait_for_completion_timeout(&cs43130->hpload_evt, - msecs_to_jiffies(1000)); + ret = cs43130_wait_for_completion(cs43130, &cs43130->hpload_evt, 1000); + regmap_read(cs43130->regmap, CS43130_INT_MASK_4, &msk); if (!ret) { dev_err(cs43130->dev, "Timeout waiting for HPLOAD interrupt\n"); @@ -2545,8 +2579,10 @@ static int cs43130_i2c_probe(struct i2c_client *client) IRQF_ONESHOT | IRQF_TRIGGER_LOW, "cs43130", cs43130); if (ret != 0) { - dev_err(cs43130->dev, "Failed to request IRQ: %d\n", ret); - goto err; + dev_dbg(cs43130->dev, "Failed to request IRQ: %d, will poll instead\n", ret); + cs43130->has_irq_line = 0; + } else { + cs43130->has_irq_line = 1; } cs43130->mclk_int_src = CS43130_MCLK_SRC_RCO; diff --git a/sound/soc/codecs/cs43130.h b/sound/soc/codecs/cs43130.h index d3f595bbd3ba..dbdb5b262f1b 100644 --- a/sound/soc/codecs/cs43130.h +++ b/sound/soc/codecs/cs43130.h @@ -508,6 +508,7 @@ struct cs43130_private { struct gpio_desc *reset_gpio; unsigned int dev_id; /* codec device ID */ int xtal_ibias; + bool has_irq_line; /* shared by both DAIs */ struct mutex clk_mutex;
Add a polling mechanism that will keep the driver operational even in absence of physical IRQ connection. If IRQ line is detected, the driver will continue working as usual, in case of missing IRQ line it will fallback to the polling mechanism introduced in this change. This will support users which choose not to connect an IRQ line as it is not critical to part's operation. Signed-off-by: Maciej Strozek <mstrozek@opensource.cirrus.com> --- V2 -> V3: Amended changelog message and subject line V1 -> V2: Add changelog message sound/soc/codecs/cs43130.c | 56 +++++++++++++++++++++++++++++++------- sound/soc/codecs/cs43130.h | 1 + 2 files changed, 47 insertions(+), 10 deletions(-) -- 2.34.1