diff mbox series

soundwire: qcom: Fix enumeration of second device on the bus

Message ID 20230405142926.842173-1-krzysztof.kozlowski@linaro.org (mailing list archive)
State New, archived
Headers show
Series soundwire: qcom: Fix enumeration of second device on the bus | expand

Commit Message

Krzysztof Kozlowski April 5, 2023, 2:29 p.m. UTC
Some Soundwire buses (like &swr0 on Qualcomm HDK8450) have two devices,
which can be brought from powerdown state one after another.  We need to
keep enumerating them on each slave attached interrupt, otherwise only
first will appear.

Cc: <stable@vger.kernel.org>
Fixes: a6e6581942ca ("soundwire: qcom: add auto enumeration support")
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Cc: Patrick Lai <quic_plai@quicinc.com>
---
 drivers/soundwire/qcom.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Pierre-Louis Bossart April 5, 2023, 3:01 p.m. UTC | #1
On 4/5/23 09:29, Krzysztof Kozlowski wrote:
> Some Soundwire buses (like &swr0 on Qualcomm HDK8450) have two devices,
> which can be brought from powerdown state one after another.  We need to
> keep enumerating them on each slave attached interrupt, otherwise only
> first will appear.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: a6e6581942ca ("soundwire: qcom: add auto enumeration support")
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
> 
> Cc: Patrick Lai <quic_plai@quicinc.com>
> ---
>  drivers/soundwire/qcom.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
> index c296e0bf897b..1e5077d91f59 100644
> --- a/drivers/soundwire/qcom.c
> +++ b/drivers/soundwire/qcom.c
> @@ -587,14 +587,9 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>  			case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS:
>  				dev_dbg_ratelimited(swrm->dev, "SWR new slave attached\n");
>  				swrm->reg_read(swrm, SWRM_MCP_SLV_STATUS, &slave_status);
> -				if (swrm->slave_status == slave_status) {
> -					dev_dbg(swrm->dev, "Slave status not changed %x\n",
> -						slave_status);

it's not clear to me how removing this test helps with the two-device
configuration?

Or is this a case where the status for both devices changes at the same
time but the interrupt status remains set, so the next iteration of the
loop is ignored?

> -				} else {
> -					qcom_swrm_get_device_status(swrm);
> -					qcom_swrm_enumerate(&swrm->bus);
> -					sdw_handle_slave_status(&swrm->bus, swrm->status);
> -				}
> +				qcom_swrm_get_device_status(swrm);
> +				qcom_swrm_enumerate(&swrm->bus);
> +				sdw_handle_slave_status(&swrm->bus, swrm->status);
>  				break;
>  			case SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET:
>  				dev_err_ratelimited(swrm->dev,
Krzysztof Kozlowski April 6, 2023, 7:03 a.m. UTC | #2
On 05/04/2023 17:01, Pierre-Louis Bossart wrote:
> 
> 
> On 4/5/23 09:29, Krzysztof Kozlowski wrote:
>> Some Soundwire buses (like &swr0 on Qualcomm HDK8450) have two devices,
>> which can be brought from powerdown state one after another.  We need to
>> keep enumerating them on each slave attached interrupt, otherwise only
>> first will appear.
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: a6e6581942ca ("soundwire: qcom: add auto enumeration support")
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Cc: Patrick Lai <quic_plai@quicinc.com>
>> ---
>>  drivers/soundwire/qcom.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
>> index c296e0bf897b..1e5077d91f59 100644
>> --- a/drivers/soundwire/qcom.c
>> +++ b/drivers/soundwire/qcom.c
>> @@ -587,14 +587,9 @@ static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
>>  			case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS:
>>  				dev_dbg_ratelimited(swrm->dev, "SWR new slave attached\n");
>>  				swrm->reg_read(swrm, SWRM_MCP_SLV_STATUS, &slave_status);
>> -				if (swrm->slave_status == slave_status) {
>> -					dev_dbg(swrm->dev, "Slave status not changed %x\n",
>> -						slave_status);
> 
> it's not clear to me how removing this test helps with the two-device
> configuration?
> 
> Or is this a case where the status for both devices changes at the same
> time but the interrupt status remains set, so the next iteration of the
> loop is ignored?

I think the patch is not correct. I misinterpreted the slave status
field and after double checking I see two speakers bound. Please ignore
for now.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c
index c296e0bf897b..1e5077d91f59 100644
--- a/drivers/soundwire/qcom.c
+++ b/drivers/soundwire/qcom.c
@@ -587,14 +587,9 @@  static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
 			case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS:
 				dev_dbg_ratelimited(swrm->dev, "SWR new slave attached\n");
 				swrm->reg_read(swrm, SWRM_MCP_SLV_STATUS, &slave_status);
-				if (swrm->slave_status == slave_status) {
-					dev_dbg(swrm->dev, "Slave status not changed %x\n",
-						slave_status);
-				} else {
-					qcom_swrm_get_device_status(swrm);
-					qcom_swrm_enumerate(&swrm->bus);
-					sdw_handle_slave_status(&swrm->bus, swrm->status);
-				}
+				qcom_swrm_get_device_status(swrm);
+				qcom_swrm_enumerate(&swrm->bus);
+				sdw_handle_slave_status(&swrm->bus, swrm->status);
 				break;
 			case SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET:
 				dev_err_ratelimited(swrm->dev,