[PATCH v3] ASoC: cs43130: Allow driver to work without IRQ connection

[PATCH v3] ASoC: cs43130: Allow driver to work without IRQ connection

[Maciej Strozek]

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(-)

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

Re: [PATCH v3] ASoC: cs43130: Allow driver to work without IRQ connection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3] ASoC: cs43130: Allow driver to work without IRQ connection

[Maciej Strozek]

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?

Re: [PATCH v3] ASoC: cs43130: Allow driver to work without IRQ connection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3] ASoC: cs43130: Allow driver to work without IRQ connection

[Charles Keepax]

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

Re: [PATCH v3] ASoC: cs43130: Allow driver to work without IRQ connection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3] ASoC: cs43130: Allow driver to work without IRQ connection

[Mark Brown]

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

Re: [PATCH v3] ASoC: cs43130: Allow driver to work without IRQ connection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]