Re: [alsa-devel] [PATCH v2 4/5] soundwire: intel/cadence: add flag for interrupt enable
From: Pierre-Louis Bossart
Date: Wed Oct 23 2019 - 13:57:57 EST
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@xxxxxxxxxxxxxxx>
---
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.