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 <[email protected]>
---
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(-)
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;
--
2.34.1
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.