[v2,4/5] soundwire: intel/cadence: add flag for interrupt enable
diff mbox series

Message ID 20190916190952.32388-5-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series
  • soundwire: intel/cadence: better initialization
Related show

Commit Message

Pierre-Louis Bossart Sept. 16, 2019, 7:09 p.m. UTC
Prepare for future PM support and fix error handling by disabling
interrupts as needed.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 18 ++++++++++++------
 drivers/soundwire/cadence_master.h |  2 +-
 drivers/soundwire/intel.c          | 12 +++++++-----
 3 files changed, 20 insertions(+), 12 deletions(-)

Comments

Vinod Koul Oct. 21, 2019, 4:14 a.m. UTC | #1
On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
> Prepare for future PM support and fix error handling by disabling
> interrupts as needed.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/cadence_master.c | 18 ++++++++++++------
>  drivers/soundwire/cadence_master.h |  2 +-
>  drivers/soundwire/intel.c          | 12 +++++++-----
>  3 files changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 5f900cf2acb9..a71df99ca18f 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -819,14 +819,17 @@ EXPORT_SYMBOL(sdw_cdns_exit_reset);
>   * sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
>   * @cdns: Cadence instance
>   */
> -int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
>  {
> -	u32 mask;
> +	u32 slave_intmask0 = 0;
> +	u32 slave_intmask1 = 0;
> +	u32 mask = 0;
> +
> +	if (!state)
> +		goto update_masks;
>  
> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
> -		    CDNS_MCP_SLAVE_INTMASK0_MASK);
> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
> -		    CDNS_MCP_SLAVE_INTMASK1_MASK);
> +	slave_intmask0 = CDNS_MCP_SLAVE_INTMASK0_MASK;
> +	slave_intmask1 = CDNS_MCP_SLAVE_INTMASK1_MASK;
>  
>  	/* enable detection of all slave state changes */
>  	mask = CDNS_MCP_INT_SLAVE_MASK;
> @@ -849,6 +852,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
>  	if (interrupt_mask) /* parameter override */
>  		mask = interrupt_mask;
>  
> +update_masks:
> +	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0, slave_intmask0);
> +	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, slave_intmask1);
>  	cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
>  
>  	/* commit changes */
> diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
> index 1a67728c5000..302351808098 100644
> --- a/drivers/soundwire/cadence_master.h
> +++ b/drivers/soundwire/cadence_master.h
> @@ -162,7 +162,7 @@ int sdw_cdns_init(struct sdw_cdns *cdns);
>  int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>  		      struct sdw_cdns_stream_config config);
>  int sdw_cdns_exit_reset(struct sdw_cdns *cdns);
> -int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
>  
>  #ifdef CONFIG_DEBUG_FS
>  void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index cdb3243e8823..08530c136c5f 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1036,7 +1036,7 @@ static int intel_probe(struct platform_device *pdev)
>  	ret = sdw_add_bus_master(&sdw->cdns.bus);
>  	if (ret) {
>  		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
> -		goto err_master_reg;
> +		return ret;

I am not sure I like this line change, before this IIRC the function and
single place of return, so changing this doesn't seem to improve
anything here..?

>  	}
>  
>  	if (sdw->cdns.bus.prop.hw_disabled) {
> @@ -1067,7 +1067,7 @@ static int intel_probe(struct platform_device *pdev)
>  		goto err_init;
>  	}
>  
> -	ret = sdw_cdns_enable_interrupt(&sdw->cdns);
> +	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
>  	if (ret < 0) {
>  		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
>  		goto err_init;
> @@ -1076,7 +1076,7 @@ static int intel_probe(struct platform_device *pdev)
>  	ret = sdw_cdns_exit_reset(&sdw->cdns);
>  	if (ret < 0) {
>  		dev_err(sdw->cdns.dev, "unable to exit bus reset sequence\n");
> -		goto err_init;
> +		goto err_interrupt;
>  	}
>  
>  	/* Register DAIs */
> @@ -1084,18 +1084,19 @@ static int intel_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err(sdw->cdns.dev, "DAI registration failed: %d\n", ret);
>  		snd_soc_unregister_component(sdw->cdns.dev);
> -		goto err_dai;
> +		goto err_interrupt;
>  	}
>  
>  	intel_debugfs_init(sdw);
>  
>  	return 0;
>  
> +err_interrupt:
> +	sdw_cdns_enable_interrupt(&sdw->cdns, false);
>  err_dai:

Isn't this unused now?

>  	free_irq(sdw->res->irq, sdw);
>  err_init:
>  	sdw_delete_bus_master(&sdw->cdns.bus);
> -err_master_reg:
>  	return ret;
>  }
>  
> @@ -1107,6 +1108,7 @@ static int intel_remove(struct platform_device *pdev)
>  
>  	if (!sdw->cdns.bus.prop.hw_disabled) {
>  		intel_debugfs_exit(sdw);
> +		sdw_cdns_enable_interrupt(&sdw->cdns, false);
>  		free_irq(sdw->res->irq, sdw);
>  		snd_soc_unregister_component(sdw->cdns.dev);
>  	}
> -- 
> 2.20.1
Pierre-Louis Bossart Oct. 21, 2019, 10:26 a.m. UTC | #2
On 10/20/19 11:14 PM, Vinod Koul wrote:
> On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
>> Prepare for future PM support and fix error handling by disabling
>> interrupts as needed.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/cadence_master.c | 18 ++++++++++++------
>>   drivers/soundwire/cadence_master.h |  2 +-
>>   drivers/soundwire/intel.c          | 12 +++++++-----
>>   3 files changed, 20 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
>> index 5f900cf2acb9..a71df99ca18f 100644
>> --- a/drivers/soundwire/cadence_master.c
>> +++ b/drivers/soundwire/cadence_master.c
>> @@ -819,14 +819,17 @@ EXPORT_SYMBOL(sdw_cdns_exit_reset);
>>    * sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
>>    * @cdns: Cadence instance
>>    */
>> -int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
>> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
>>   {
>> -	u32 mask;
>> +	u32 slave_intmask0 = 0;
>> +	u32 slave_intmask1 = 0;
>> +	u32 mask = 0;
>> +
>> +	if (!state)
>> +		goto update_masks;
>>   
>> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
>> -		    CDNS_MCP_SLAVE_INTMASK0_MASK);
>> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
>> -		    CDNS_MCP_SLAVE_INTMASK1_MASK);
>> +	slave_intmask0 = CDNS_MCP_SLAVE_INTMASK0_MASK;
>> +	slave_intmask1 = CDNS_MCP_SLAVE_INTMASK1_MASK;
>>   
>>   	/* enable detection of all slave state changes */
>>   	mask = CDNS_MCP_INT_SLAVE_MASK;
>> @@ -849,6 +852,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
>>   	if (interrupt_mask) /* parameter override */
>>   		mask = interrupt_mask;
>>   
>> +update_masks:
>> +	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0, slave_intmask0);
>> +	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, slave_intmask1);
>>   	cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
>>   
>>   	/* commit changes */
>> diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
>> index 1a67728c5000..302351808098 100644
>> --- a/drivers/soundwire/cadence_master.h
>> +++ b/drivers/soundwire/cadence_master.h
>> @@ -162,7 +162,7 @@ int sdw_cdns_init(struct sdw_cdns *cdns);
>>   int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>>   		      struct sdw_cdns_stream_config config);
>>   int sdw_cdns_exit_reset(struct sdw_cdns *cdns);
>> -int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
>> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
>>   
>>   #ifdef CONFIG_DEBUG_FS
>>   void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>> index cdb3243e8823..08530c136c5f 100644
>> --- a/drivers/soundwire/intel.c
>> +++ b/drivers/soundwire/intel.c
>> @@ -1036,7 +1036,7 @@ static int intel_probe(struct platform_device *pdev)
>>   	ret = sdw_add_bus_master(&sdw->cdns.bus);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
>> -		goto err_master_reg;
>> +		return ret;
> 
> I am not sure I like this line change, before this IIRC the function and
> single place of return, so changing this doesn't seem to improve
> anything here..?

Doing a goto to do a return is not very nice either.

I can change this, but it doesn't really matter: this entire code will 
be removed anyways to get rid of platform_devices and the probe itself 
will be split in two.

> 
>>   	}
>>   
>>   	if (sdw->cdns.bus.prop.hw_disabled) {
>> @@ -1067,7 +1067,7 @@ static int intel_probe(struct platform_device *pdev)
>>   		goto err_init;
>>   	}
>>   
>> -	ret = sdw_cdns_enable_interrupt(&sdw->cdns);
>> +	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
>>   	if (ret < 0) {
>>   		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
>>   		goto err_init;
>> @@ -1076,7 +1076,7 @@ static int intel_probe(struct platform_device *pdev)
>>   	ret = sdw_cdns_exit_reset(&sdw->cdns);
>>   	if (ret < 0) {
>>   		dev_err(sdw->cdns.dev, "unable to exit bus reset sequence\n");
>> -		goto err_init;
>> +		goto err_interrupt;
>>   	}
>>   
>>   	/* Register DAIs */
>> @@ -1084,18 +1084,19 @@ static int intel_probe(struct platform_device *pdev)
>>   	if (ret) {
>>   		dev_err(sdw->cdns.dev, "DAI registration failed: %d\n", ret);
>>   		snd_soc_unregister_component(sdw->cdns.dev);
>> -		goto err_dai;
>> +		goto err_interrupt;
>>   	}
>>   
>>   	intel_debugfs_init(sdw);
>>   
>>   	return 0;
>>   
>> +err_interrupt:
>> +	sdw_cdns_enable_interrupt(&sdw->cdns, false);
>>   err_dai:
> 
> Isn't this unused now?
> 
>>   	free_irq(sdw->res->irq, sdw);
>>   err_init:
>>   	sdw_delete_bus_master(&sdw->cdns.bus);
>> -err_master_reg:
>>   	return ret;
>>   }
>>   
>> @@ -1107,6 +1108,7 @@ static int intel_remove(struct platform_device *pdev)
>>   
>>   	if (!sdw->cdns.bus.prop.hw_disabled) {
>>   		intel_debugfs_exit(sdw);
>> +		sdw_cdns_enable_interrupt(&sdw->cdns, false);
>>   		free_irq(sdw->res->irq, sdw);
>>   		snd_soc_unregister_component(sdw->cdns.dev);
>>   	}
>> -- 
>> 2.20.1
>
Vinod Koul Oct. 22, 2019, 4:55 a.m. UTC | #3
On 21-10-19, 05:26, Pierre-Louis Bossart wrote:
> On 10/20/19 11:14 PM, Vinod Koul wrote:
> > On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
> > > Prepare for future PM support and fix error handling by disabling
> > > interrupts as needed.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > ---
> > >   drivers/soundwire/cadence_master.c | 18 ++++++++++++------
> > >   drivers/soundwire/cadence_master.h |  2 +-
> > >   drivers/soundwire/intel.c          | 12 +++++++-----
> > >   3 files changed, 20 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> > > index 5f900cf2acb9..a71df99ca18f 100644
> > > --- a/drivers/soundwire/cadence_master.c
> > > +++ b/drivers/soundwire/cadence_master.c
> > > @@ -819,14 +819,17 @@ EXPORT_SYMBOL(sdw_cdns_exit_reset);
> > >    * sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
> > >    * @cdns: Cadence instance
> > >    */
> > > -int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
> > > +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
> > >   {
> > > -	u32 mask;
> > > +	u32 slave_intmask0 = 0;
> > > +	u32 slave_intmask1 = 0;
> > > +	u32 mask = 0;
> > > +
> > > +	if (!state)
> > > +		goto update_masks;
> > > -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
> > > -		    CDNS_MCP_SLAVE_INTMASK0_MASK);
> > > -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
> > > -		    CDNS_MCP_SLAVE_INTMASK1_MASK);
> > > +	slave_intmask0 = CDNS_MCP_SLAVE_INTMASK0_MASK;
> > > +	slave_intmask1 = CDNS_MCP_SLAVE_INTMASK1_MASK;
> > >   	/* enable detection of all slave state changes */
> > >   	mask = CDNS_MCP_INT_SLAVE_MASK;
> > > @@ -849,6 +852,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
> > >   	if (interrupt_mask) /* parameter override */
> > >   		mask = interrupt_mask;
> > > +update_masks:
> > > +	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0, slave_intmask0);
> > > +	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, slave_intmask1);
> > >   	cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
> > >   	/* commit changes */
> > > diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
> > > index 1a67728c5000..302351808098 100644
> > > --- a/drivers/soundwire/cadence_master.h
> > > +++ b/drivers/soundwire/cadence_master.h
> > > @@ -162,7 +162,7 @@ int sdw_cdns_init(struct sdw_cdns *cdns);
> > >   int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
> > >   		      struct sdw_cdns_stream_config config);
> > >   int sdw_cdns_exit_reset(struct sdw_cdns *cdns);
> > > -int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
> > > +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
> > >   #ifdef CONFIG_DEBUG_FS
> > >   void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
> > > diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> > > index cdb3243e8823..08530c136c5f 100644
> > > --- a/drivers/soundwire/intel.c
> > > +++ b/drivers/soundwire/intel.c
> > > @@ -1036,7 +1036,7 @@ static int intel_probe(struct platform_device *pdev)
> > >   	ret = sdw_add_bus_master(&sdw->cdns.bus);
> > >   	if (ret) {
> > >   		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
> > > -		goto err_master_reg;
> > > +		return ret;
> > 
> > I am not sure I like this line change, before this IIRC the function and
> > single place of return, so changing this doesn't seem to improve
> > anything here..?
> 
> Doing a goto to do a return is not very nice either.

Hrmm, isn't that what you are doing few lines below. The point here is
that this line of change here doesnt change anything, doesnt improve
anything so why change :)

> I can change this, but it doesn't really matter: this entire code will be
> removed anyways to get rid of platform_devices and the probe itself will be
> split in two.
> 
> > 
> > >   	}
> > >   	if (sdw->cdns.bus.prop.hw_disabled) {
> > > @@ -1067,7 +1067,7 @@ static int intel_probe(struct platform_device *pdev)
> > >   		goto err_init;
> > >   	}
> > > -	ret = sdw_cdns_enable_interrupt(&sdw->cdns);
> > > +	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
> > >   	if (ret < 0) {
> > >   		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
> > >   		goto err_init;
> > > @@ -1076,7 +1076,7 @@ static int intel_probe(struct platform_device *pdev)
> > >   	ret = sdw_cdns_exit_reset(&sdw->cdns);
> > >   	if (ret < 0) {
> > >   		dev_err(sdw->cdns.dev, "unable to exit bus reset sequence\n");
> > > -		goto err_init;
> > > +		goto err_interrupt;
> > >   	}
> > >   	/* Register DAIs */
> > > @@ -1084,18 +1084,19 @@ static int intel_probe(struct platform_device *pdev)
> > >   	if (ret) {
> > >   		dev_err(sdw->cdns.dev, "DAI registration failed: %d\n", ret);
> > >   		snd_soc_unregister_component(sdw->cdns.dev);
> > > -		goto err_dai;
> > > +		goto err_interrupt;
> > >   	}
> > >   	intel_debugfs_init(sdw);
> > >   	return 0;
> > > +err_interrupt:
> > > +	sdw_cdns_enable_interrupt(&sdw->cdns, false);
> > >   err_dai:
> > 
> > Isn't this unused now?

??? you missed this.
Pierre-Louis Bossart Oct. 22, 2019, 8:41 p.m. UTC | #4
On 10/21/19 11:55 PM, Vinod Koul wrote:
> On 21-10-19, 05:26, Pierre-Louis Bossart wrote:
>> On 10/20/19 11:14 PM, Vinod Koul wrote:
>>> On 16-09-19, 14:09, Pierre-Louis Bossart wrote:
>>>> Prepare for future PM support and fix error handling by disabling
>>>> interrupts as needed.
>>>>
>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>> ---
>>>>    drivers/soundwire/cadence_master.c | 18 ++++++++++++------
>>>>    drivers/soundwire/cadence_master.h |  2 +-
>>>>    drivers/soundwire/intel.c          | 12 +++++++-----
>>>>    3 files changed, 20 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
>>>> index 5f900cf2acb9..a71df99ca18f 100644
>>>> --- a/drivers/soundwire/cadence_master.c
>>>> +++ b/drivers/soundwire/cadence_master.c
>>>> @@ -819,14 +819,17 @@ EXPORT_SYMBOL(sdw_cdns_exit_reset);
>>>>     * sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
>>>>     * @cdns: Cadence instance
>>>>     */
>>>> -int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
>>>> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
>>>>    {
>>>> -	u32 mask;
>>>> +	u32 slave_intmask0 = 0;
>>>> +	u32 slave_intmask1 = 0;
>>>> +	u32 mask = 0;
>>>> +
>>>> +	if (!state)
>>>> +		goto update_masks;
>>>> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
>>>> -		    CDNS_MCP_SLAVE_INTMASK0_MASK);
>>>> -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
>>>> -		    CDNS_MCP_SLAVE_INTMASK1_MASK);
>>>> +	slave_intmask0 = CDNS_MCP_SLAVE_INTMASK0_MASK;
>>>> +	slave_intmask1 = CDNS_MCP_SLAVE_INTMASK1_MASK;
>>>>    	/* enable detection of all slave state changes */
>>>>    	mask = CDNS_MCP_INT_SLAVE_MASK;
>>>> @@ -849,6 +852,9 @@ int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
>>>>    	if (interrupt_mask) /* parameter override */
>>>>    		mask = interrupt_mask;
>>>> +update_masks:
>>>> +	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0, slave_intmask0);
>>>> +	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, slave_intmask1);
>>>>    	cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
>>>>    	/* commit changes */
>>>> diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
>>>> index 1a67728c5000..302351808098 100644
>>>> --- a/drivers/soundwire/cadence_master.h
>>>> +++ b/drivers/soundwire/cadence_master.h
>>>> @@ -162,7 +162,7 @@ int sdw_cdns_init(struct sdw_cdns *cdns);
>>>>    int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>>>>    		      struct sdw_cdns_stream_config config);
>>>>    int sdw_cdns_exit_reset(struct sdw_cdns *cdns);
>>>> -int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
>>>> +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
>>>>    #ifdef CONFIG_DEBUG_FS
>>>>    void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
>>>> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
>>>> index cdb3243e8823..08530c136c5f 100644
>>>> --- a/drivers/soundwire/intel.c
>>>> +++ b/drivers/soundwire/intel.c
>>>> @@ -1036,7 +1036,7 @@ static int intel_probe(struct platform_device *pdev)
>>>>    	ret = sdw_add_bus_master(&sdw->cdns.bus);
>>>>    	if (ret) {
>>>>    		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
>>>> -		goto err_master_reg;
>>>> +		return ret;
>>>
>>> I am not sure I like this line change, before this IIRC the function and
>>> single place of return, so changing this doesn't seem to improve
>>> anything here..?
>>
>> Doing a goto to do a return is not very nice either.
> 
> Hrmm, isn't that what you are doing few lines below. The point here is
> that this line of change here doesnt change anything, doesnt improve
> anything so why change :)

I knew there was a reason.. the existing code on your soundwire/next 
branch mixes goto and return, so I chose to use a return for the first 
case as well.

	ret = sdw_add_bus_master(&sdw->cdns.bus);
	if (ret) {
		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
		goto err_master_reg; >>> changed to return ret;
	}

	if (sdw->cdns.bus.prop.hw_disabled) {
		dev_info(&pdev->dev, "SoundWire master %d is disabled, ignoring\n",
			 sdw->cdns.bus.link_id);
		return 0;
	}



> 
>> I can change this, but it doesn't really matter: this entire code will be
>> removed anyways to get rid of platform_devices and the probe itself will be
>> split in two.
>>
>>>
>>>>    	}
>>>>    	if (sdw->cdns.bus.prop.hw_disabled) {
>>>> @@ -1067,7 +1067,7 @@ static int intel_probe(struct platform_device *pdev)
>>>>    		goto err_init;
>>>>    	}
>>>> -	ret = sdw_cdns_enable_interrupt(&sdw->cdns);
>>>> +	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
>>>>    	if (ret < 0) {
>>>>    		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
>>>>    		goto err_init;
>>>> @@ -1076,7 +1076,7 @@ static int intel_probe(struct platform_device *pdev)
>>>>    	ret = sdw_cdns_exit_reset(&sdw->cdns);
>>>>    	if (ret < 0) {
>>>>    		dev_err(sdw->cdns.dev, "unable to exit bus reset sequence\n");
>>>> -		goto err_init;
>>>> +		goto err_interrupt;
>>>>    	}
>>>>    	/* Register DAIs */
>>>> @@ -1084,18 +1084,19 @@ static int intel_probe(struct platform_device *pdev)
>>>>    	if (ret) {
>>>>    		dev_err(sdw->cdns.dev, "DAI registration failed: %d\n", ret);
>>>>    		snd_soc_unregister_component(sdw->cdns.dev);
>>>> -		goto err_dai;
>>>> +		goto err_interrupt;
>>>>    	}
>>>>    	intel_debugfs_init(sdw);
>>>>    	return 0;
>>>> +err_interrupt:
>>>> +	sdw_cdns_enable_interrupt(&sdw->cdns, false);
>>>>    err_dai:
>>>
>>> Isn't this unused now?
> 
> ??? you missed this.

fixed.

Patch
diff mbox series

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 5f900cf2acb9..a71df99ca18f 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -819,14 +819,17 @@  EXPORT_SYMBOL(sdw_cdns_exit_reset);
  * sdw_cdns_enable_interrupt() - Enable SDW interrupts and update config
  * @cdns: Cadence instance
  */
-int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
+int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
 {
-	u32 mask;
+	u32 slave_intmask0 = 0;
+	u32 slave_intmask1 = 0;
+	u32 mask = 0;
+
+	if (!state)
+		goto update_masks;
 
-	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
-		    CDNS_MCP_SLAVE_INTMASK0_MASK);
-	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
-		    CDNS_MCP_SLAVE_INTMASK1_MASK);
+	slave_intmask0 = CDNS_MCP_SLAVE_INTMASK0_MASK;
+	slave_intmask1 = CDNS_MCP_SLAVE_INTMASK1_MASK;
 
 	/* enable detection of all slave state changes */
 	mask = CDNS_MCP_INT_SLAVE_MASK;
@@ -849,6 +852,9 @@  int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
 	if (interrupt_mask) /* parameter override */
 		mask = interrupt_mask;
 
+update_masks:
+	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0, slave_intmask0);
+	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, slave_intmask1);
 	cdns_writel(cdns, CDNS_MCP_INTMASK, mask);
 
 	/* commit changes */
diff --git a/drivers/soundwire/cadence_master.h b/drivers/soundwire/cadence_master.h
index 1a67728c5000..302351808098 100644
--- a/drivers/soundwire/cadence_master.h
+++ b/drivers/soundwire/cadence_master.h
@@ -162,7 +162,7 @@  int sdw_cdns_init(struct sdw_cdns *cdns);
 int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
 		      struct sdw_cdns_stream_config config);
 int sdw_cdns_exit_reset(struct sdw_cdns *cdns);
-int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns);
+int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state);
 
 #ifdef CONFIG_DEBUG_FS
 void sdw_cdns_debugfs_init(struct sdw_cdns *cdns, struct dentry *root);
diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index cdb3243e8823..08530c136c5f 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -1036,7 +1036,7 @@  static int intel_probe(struct platform_device *pdev)
 	ret = sdw_add_bus_master(&sdw->cdns.bus);
 	if (ret) {
 		dev_err(&pdev->dev, "sdw_add_bus_master fail: %d\n", ret);
-		goto err_master_reg;
+		return ret;
 	}
 
 	if (sdw->cdns.bus.prop.hw_disabled) {
@@ -1067,7 +1067,7 @@  static int intel_probe(struct platform_device *pdev)
 		goto err_init;
 	}
 
-	ret = sdw_cdns_enable_interrupt(&sdw->cdns);
+	ret = sdw_cdns_enable_interrupt(&sdw->cdns, true);
 	if (ret < 0) {
 		dev_err(sdw->cdns.dev, "cannot enable interrupts\n");
 		goto err_init;
@@ -1076,7 +1076,7 @@  static int intel_probe(struct platform_device *pdev)
 	ret = sdw_cdns_exit_reset(&sdw->cdns);
 	if (ret < 0) {
 		dev_err(sdw->cdns.dev, "unable to exit bus reset sequence\n");
-		goto err_init;
+		goto err_interrupt;
 	}
 
 	/* Register DAIs */
@@ -1084,18 +1084,19 @@  static int intel_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(sdw->cdns.dev, "DAI registration failed: %d\n", ret);
 		snd_soc_unregister_component(sdw->cdns.dev);
-		goto err_dai;
+		goto err_interrupt;
 	}
 
 	intel_debugfs_init(sdw);
 
 	return 0;
 
+err_interrupt:
+	sdw_cdns_enable_interrupt(&sdw->cdns, false);
 err_dai:
 	free_irq(sdw->res->irq, sdw);
 err_init:
 	sdw_delete_bus_master(&sdw->cdns.bus);
-err_master_reg:
 	return ret;
 }
 
@@ -1107,6 +1108,7 @@  static int intel_remove(struct platform_device *pdev)
 
 	if (!sdw->cdns.bus.prop.hw_disabled) {
 		intel_debugfs_exit(sdw);
+		sdw_cdns_enable_interrupt(&sdw->cdns, false);
 		free_irq(sdw->res->irq, sdw);
 		snd_soc_unregister_component(sdw->cdns.dev);
 	}