Re: [alsa-devel] [PATCH v2 4/5] soundwire: intel/cadence: add flag for interrupt enable

[Vinod Koul]

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 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.

-- 
~Vinod