Re: [RFC PATCH 36/40] soundwire: intel: disable interrupts on suspend

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

> > -int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns)
> > +int sdw_cdns_enable_interrupt(struct sdw_cdns *cdns, bool state)
> >   {
> >   	u32 mask;
> >   
> > -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
> > -		    CDNS_MCP_SLAVE_INTMASK0_MASK);
> > -	cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
> > -		    CDNS_MCP_SLAVE_INTMASK1_MASK);
> > +	if (state) {
> > +		cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0,
> > +			    CDNS_MCP_SLAVE_INTMASK0_MASK);
> > +		cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1,
> > +			    CDNS_MCP_SLAVE_INTMASK1_MASK);
> >   
> > -	/* enable detection of slave state changes */
> > -	mask = CDNS_MCP_INT_SLAVE_RSVD | CDNS_MCP_INT_SLAVE_ALERT |
> > -		CDNS_MCP_INT_SLAVE_ATTACH | CDNS_MCP_INT_SLAVE_NATTACH;
> > +		/* enable detection of slave state changes */
> > +		mask = CDNS_MCP_INT_SLAVE_RSVD | CDNS_MCP_INT_SLAVE_ALERT |
> > +			CDNS_MCP_INT_SLAVE_ATTACH | CDNS_MCP_INT_SLAVE_NATTACH;
> >   
> > -	/* enable detection of bus issues */
> > -	mask |= CDNS_MCP_INT_CTRL_CLASH | CDNS_MCP_INT_DATA_CLASH |
> > -		CDNS_MCP_INT_PARITY;
> > +		/* enable detection of bus issues */
> > +		mask |= CDNS_MCP_INT_CTRL_CLASH | CDNS_MCP_INT_DATA_CLASH |
> > +			CDNS_MCP_INT_PARITY;
> >   
> > -	/* no detection of port interrupts for now */
> > +		/* no detection of port interrupts for now */
> >   
> > -	/* enable detection of RX fifo level */
> > -	mask |= CDNS_MCP_INT_RX_WL;
> > +		/* enable detection of RX fifo level */
> > +		mask |= CDNS_MCP_INT_RX_WL;
> >   
> > -	/* now enable all of the above */
> > -	mask |= CDNS_MCP_INT_IRQ;
> > +		/* now enable all of the above */
> > +		mask |= CDNS_MCP_INT_IRQ;
> >   
> > -	if (interrupt_mask) /* parameter override */
> > -		mask = interrupt_mask;
> > +		if (interrupt_mask) /* parameter override */
> > +			mask = interrupt_mask;
> > +	} else {
> > +		cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK0, 0);
> > +		cdns_writel(cdns, CDNS_MCP_SLAVE_INTMASK1, 0);
> > +		mask = 0;
> > +	}
>
> Looks like this should be two functions? Especially since "state" is always a constant
> when it is called. If there is still a lot of common code below, maybe make it a helper
> function.

Yes, the code is a bit ugly. I could initialize all the masks to zero, 
have the if(state) block and write the masks.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel