diff mbox series

[4/4] soundwire: stream: Move remaining register accesses over to no_pm

Message ID 20221114102956.914468-5-ckeepax@opensource.cirrus.com (mailing list archive)
State Superseded
Headers show
Series Minor SoundWire clean ups | expand

Commit Message

Charles Keepax Nov. 14, 2022, 10:29 a.m. UTC 
There is no need to play with the runtime reference everytime a register
is accessed. All the remaining "pm" style register accesses trace back
to 4 functions:

sdw_prepare_stream
sdw_deprepare_stream
sdw_enable_stream
sdw_disable_stream

Any sensible implementation will need to hold a runtime reference
across all those functions, it makes no sense to be allowing the
device/bus to suspend whilst streams are being prepared/enabled. And
certainly in the case of the all existing users, they all call these
functions from hw_params/prepare/trigger/hw_free callbacks in ALSA,
which will have already runtime resumed all the audio devices
associated during the open callback.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/bus.c    |  2 +-
 drivers/soundwire/stream.c | 30 +++++++++++++++---------------
 2 files changed, 16 insertions(+), 16 deletions(-)

Comments

Pierre-Louis Bossart Nov. 14, 2022, 4:04 p.m. UTC | #1 
On 11/14/22 04:29, Charles Keepax wrote:
> There is no need to play with the runtime reference everytime a register
> is accessed. All the remaining "pm" style register accesses trace back
> to 4 functions:
> 
> sdw_prepare_stream
> sdw_deprepare_stream
> sdw_enable_stream
> sdw_disable_stream
> 
> Any sensible implementation will need to hold a runtime reference
> across all those functions, it makes no sense to be allowing the
> device/bus to suspend whilst streams are being prepared/enabled. And
> certainly in the case of the all existing users, they all call these
> functions from hw_params/prepare/trigger/hw_free callbacks in ALSA,
> which will have already runtime resumed all the audio devices
> associated during the open callback.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

I tend to agree with this one, and if this ever fails that would point
to a miss at a higher-level we'd need to address.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

> ---
>  drivers/soundwire/bus.c    |  2 +-
>  drivers/soundwire/stream.c | 30 +++++++++++++++---------------
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index ef4878258afad..d87a188fcce1e 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -1214,7 +1214,7 @@ int sdw_configure_dpn_intr(struct sdw_slave *slave,
>  		val &= ~SDW_DPN_INT_PORT_READY;
>  	}
>  
> -	ret = sdw_update(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val);
> +	ret = sdw_update_no_pm(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val);
>  	if (ret < 0)
>  		dev_err(&slave->dev,
>  			"SDW_DPN_INTMASK write failed:%d\n", val);
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index bd502368339e5..df3b36670df4c 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -81,14 +81,14 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus,
>  	}
>  
>  	/* Program DPN_OffsetCtrl2 registers */
> -	ret = sdw_write(slave, addr1, t_params->offset2);
> +	ret = sdw_write_no_pm(slave, addr1, t_params->offset2);
>  	if (ret < 0) {
>  		dev_err(bus->dev, "DPN_OffsetCtrl2 register write failed\n");
>  		return ret;
>  	}
>  
>  	/* Program DPN_BlockCtrl3 register */
> -	ret = sdw_write(slave, addr2, t_params->blk_pkg_mode);
> +	ret = sdw_write_no_pm(slave, addr2, t_params->blk_pkg_mode);
>  	if (ret < 0) {
>  		dev_err(bus->dev, "DPN_BlockCtrl3 register write failed\n");
>  		return ret;
> @@ -105,7 +105,7 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus,
>  	/* Program DPN_SampleCtrl2 register */
>  	wbuf = FIELD_GET(SDW_DPN_SAMPLECTRL_HIGH, t_params->sample_interval - 1);
>  
> -	ret = sdw_write(slave, addr3, wbuf);
> +	ret = sdw_write_no_pm(slave, addr3, wbuf);
>  	if (ret < 0) {
>  		dev_err(bus->dev, "DPN_SampleCtrl2 register write failed\n");
>  		return ret;
> @@ -115,7 +115,7 @@ static int _sdw_program_slave_port_params(struct sdw_bus *bus,
>  	wbuf = FIELD_PREP(SDW_DPN_HCTRL_HSTART, t_params->hstart);
>  	wbuf |= FIELD_PREP(SDW_DPN_HCTRL_HSTOP, t_params->hstop);
>  
> -	ret = sdw_write(slave, addr4, wbuf);
> +	ret = sdw_write_no_pm(slave, addr4, wbuf);
>  	if (ret < 0)
>  		dev_err(bus->dev, "DPN_HCtrl register write failed\n");
>  
> @@ -163,7 +163,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>  	wbuf = FIELD_PREP(SDW_DPN_PORTCTRL_DATAMODE, p_params->data_mode);
>  	wbuf |= FIELD_PREP(SDW_DPN_PORTCTRL_FLOWMODE, p_params->flow_mode);
>  
> -	ret = sdw_update(s_rt->slave, addr1, 0xF, wbuf);
> +	ret = sdw_update_no_pm(s_rt->slave, addr1, 0xF, wbuf);
>  	if (ret < 0) {
>  		dev_err(&s_rt->slave->dev,
>  			"DPN_PortCtrl register write failed for port %d\n",
> @@ -173,7 +173,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>  
>  	if (!dpn_prop->read_only_wordlength) {
>  		/* Program DPN_BlockCtrl1 register */
> -		ret = sdw_write(s_rt->slave, addr2, (p_params->bps - 1));
> +		ret = sdw_write_no_pm(s_rt->slave, addr2, (p_params->bps - 1));
>  		if (ret < 0) {
>  			dev_err(&s_rt->slave->dev,
>  				"DPN_BlockCtrl1 register write failed for port %d\n",
> @@ -184,7 +184,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>  
>  	/* Program DPN_SampleCtrl1 register */
>  	wbuf = (t_params->sample_interval - 1) & SDW_DPN_SAMPLECTRL_LOW;
> -	ret = sdw_write(s_rt->slave, addr3, wbuf);
> +	ret = sdw_write_no_pm(s_rt->slave, addr3, wbuf);
>  	if (ret < 0) {
>  		dev_err(&s_rt->slave->dev,
>  			"DPN_SampleCtrl1 register write failed for port %d\n",
> @@ -193,7 +193,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>  	}
>  
>  	/* Program DPN_OffsetCtrl1 registers */
> -	ret = sdw_write(s_rt->slave, addr4, t_params->offset1);
> +	ret = sdw_write_no_pm(s_rt->slave, addr4, t_params->offset1);
>  	if (ret < 0) {
>  		dev_err(&s_rt->slave->dev,
>  			"DPN_OffsetCtrl1 register write failed for port %d\n",
> @@ -203,7 +203,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>  
>  	/* Program DPN_BlockCtrl2 register*/
>  	if (t_params->blk_grp_ctrl_valid) {
> -		ret = sdw_write(s_rt->slave, addr5, t_params->blk_grp_ctrl);
> +		ret = sdw_write_no_pm(s_rt->slave, addr5, t_params->blk_grp_ctrl);
>  		if (ret < 0) {
>  			dev_err(&s_rt->slave->dev,
>  				"DPN_BlockCtrl2 reg write failed for port %d\n",
> @@ -214,7 +214,7 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>  
>  	/* program DPN_LaneCtrl register */
>  	if (slave_prop->lane_control_support) {
> -		ret = sdw_write(s_rt->slave, addr6, t_params->lane_ctrl);
> +		ret = sdw_write_no_pm(s_rt->slave, addr6, t_params->lane_ctrl);
>  		if (ret < 0) {
>  			dev_err(&s_rt->slave->dev,
>  				"DPN_LaneCtrl register write failed for port %d\n",
> @@ -319,9 +319,9 @@ static int sdw_enable_disable_slave_ports(struct sdw_bus *bus,
>  	 * it is safe to reset this register
>  	 */
>  	if (en)
> -		ret = sdw_write(s_rt->slave, addr, p_rt->ch_mask);
> +		ret = sdw_write_no_pm(s_rt->slave, addr, p_rt->ch_mask);
>  	else
> -		ret = sdw_write(s_rt->slave, addr, 0x0);
> +		ret = sdw_write_no_pm(s_rt->slave, addr, 0x0);
>  
>  	if (ret < 0)
>  		dev_err(&s_rt->slave->dev,
> @@ -476,9 +476,9 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
>  		addr = SDW_DPN_PREPARECTRL(p_rt->num);
>  
>  		if (prep)
> -			ret = sdw_write(s_rt->slave, addr, p_rt->ch_mask);
> +			ret = sdw_write_no_pm(s_rt->slave, addr, p_rt->ch_mask);
>  		else
> -			ret = sdw_write(s_rt->slave, addr, 0x0);
> +			ret = sdw_write_no_pm(s_rt->slave, addr, 0x0);
>  
>  		if (ret < 0) {
>  			dev_err(&s_rt->slave->dev,
> @@ -491,7 +491,7 @@ static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
>  		wait_for_completion_timeout(port_ready,
>  			msecs_to_jiffies(dpn_prop->ch_prep_timeout));
>  
> -		val = sdw_read(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num));
> +		val = sdw_read_no_pm(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num));
>  		if ((val < 0) || (val & p_rt->ch_mask)) {
>  			ret = (val < 0) ? val : -ETIMEDOUT;
>  			dev_err(&s_rt->slave->dev,
Charles Keepax Nov. 15, 2022, 11:05 a.m. UTC | #2 
On Mon, Nov 14, 2022 at 10:04:55AM -0600, Pierre-Louis Bossart wrote:
> 
> 
> On 11/14/22 04:29, Charles Keepax wrote:
> > There is no need to play with the runtime reference everytime a register
> > is accessed. All the remaining "pm" style register accesses trace back
> > to 4 functions:
> > 
> > sdw_prepare_stream
> > sdw_deprepare_stream
> > sdw_enable_stream
> > sdw_disable_stream
> > 
> > Any sensible implementation will need to hold a runtime reference
> > across all those functions, it makes no sense to be allowing the
> > device/bus to suspend whilst streams are being prepared/enabled. And
> > certainly in the case of the all existing users, they all call these
> > functions from hw_params/prepare/trigger/hw_free callbacks in ALSA,
> > which will have already runtime resumed all the audio devices
> > associated during the open callback.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> 
> I tend to agree with this one, and if this ever fails that would point
> to a miss at a higher-level we'd need to address.

Exactly my concern here is the core is trying to be helpful, but
really it is just going to be hiding bugs in the callers.

Thanks,
Charles
diff mbox series

Patch

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index ef4878258afad..d87a188fcce1e 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1214,7 +1214,7 @@  int sdw_configure_dpn_intr(struct sdw_slave *slave,
 		val &= ~SDW_DPN_INT_PORT_READY;
 	}
 
-	ret = sdw_update(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val);
+	ret = sdw_update_no_pm(slave, addr, (mask | SDW_DPN_INT_PORT_READY), val);
 	if (ret < 0)
 		dev_err(&slave->dev,
 			"SDW_DPN_INTMASK write failed:%d\n", val);
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index bd502368339e5..df3b36670df4c 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -81,14 +81,14 @@  static int _sdw_program_slave_port_params(struct sdw_bus *bus,
 	}
 
 	/* Program DPN_OffsetCtrl2 registers */
-	ret = sdw_write(slave, addr1, t_params->offset2);
+	ret = sdw_write_no_pm(slave, addr1, t_params->offset2);
 	if (ret < 0) {
 		dev_err(bus->dev, "DPN_OffsetCtrl2 register write failed\n");
 		return ret;
 	}
 
 	/* Program DPN_BlockCtrl3 register */
-	ret = sdw_write(slave, addr2, t_params->blk_pkg_mode);
+	ret = sdw_write_no_pm(slave, addr2, t_params->blk_pkg_mode);
 	if (ret < 0) {
 		dev_err(bus->dev, "DPN_BlockCtrl3 register write failed\n");
 		return ret;
@@ -105,7 +105,7 @@  static int _sdw_program_slave_port_params(struct sdw_bus *bus,
 	/* Program DPN_SampleCtrl2 register */
 	wbuf = FIELD_GET(SDW_DPN_SAMPLECTRL_HIGH, t_params->sample_interval - 1);
 
-	ret = sdw_write(slave, addr3, wbuf);
+	ret = sdw_write_no_pm(slave, addr3, wbuf);
 	if (ret < 0) {
 		dev_err(bus->dev, "DPN_SampleCtrl2 register write failed\n");
 		return ret;
@@ -115,7 +115,7 @@  static int _sdw_program_slave_port_params(struct sdw_bus *bus,
 	wbuf = FIELD_PREP(SDW_DPN_HCTRL_HSTART, t_params->hstart);
 	wbuf |= FIELD_PREP(SDW_DPN_HCTRL_HSTOP, t_params->hstop);
 
-	ret = sdw_write(slave, addr4, wbuf);
+	ret = sdw_write_no_pm(slave, addr4, wbuf);
 	if (ret < 0)
 		dev_err(bus->dev, "DPN_HCtrl register write failed\n");
 
@@ -163,7 +163,7 @@  static int sdw_program_slave_port_params(struct sdw_bus *bus,
 	wbuf = FIELD_PREP(SDW_DPN_PORTCTRL_DATAMODE, p_params->data_mode);
 	wbuf |= FIELD_PREP(SDW_DPN_PORTCTRL_FLOWMODE, p_params->flow_mode);
 
-	ret = sdw_update(s_rt->slave, addr1, 0xF, wbuf);
+	ret = sdw_update_no_pm(s_rt->slave, addr1, 0xF, wbuf);
 	if (ret < 0) {
 		dev_err(&s_rt->slave->dev,
 			"DPN_PortCtrl register write failed for port %d\n",
@@ -173,7 +173,7 @@  static int sdw_program_slave_port_params(struct sdw_bus *bus,
 
 	if (!dpn_prop->read_only_wordlength) {
 		/* Program DPN_BlockCtrl1 register */
-		ret = sdw_write(s_rt->slave, addr2, (p_params->bps - 1));
+		ret = sdw_write_no_pm(s_rt->slave, addr2, (p_params->bps - 1));
 		if (ret < 0) {
 			dev_err(&s_rt->slave->dev,
 				"DPN_BlockCtrl1 register write failed for port %d\n",
@@ -184,7 +184,7 @@  static int sdw_program_slave_port_params(struct sdw_bus *bus,
 
 	/* Program DPN_SampleCtrl1 register */
 	wbuf = (t_params->sample_interval - 1) & SDW_DPN_SAMPLECTRL_LOW;
-	ret = sdw_write(s_rt->slave, addr3, wbuf);
+	ret = sdw_write_no_pm(s_rt->slave, addr3, wbuf);
 	if (ret < 0) {
 		dev_err(&s_rt->slave->dev,
 			"DPN_SampleCtrl1 register write failed for port %d\n",
@@ -193,7 +193,7 @@  static int sdw_program_slave_port_params(struct sdw_bus *bus,
 	}
 
 	/* Program DPN_OffsetCtrl1 registers */
-	ret = sdw_write(s_rt->slave, addr4, t_params->offset1);
+	ret = sdw_write_no_pm(s_rt->slave, addr4, t_params->offset1);
 	if (ret < 0) {
 		dev_err(&s_rt->slave->dev,
 			"DPN_OffsetCtrl1 register write failed for port %d\n",
@@ -203,7 +203,7 @@  static int sdw_program_slave_port_params(struct sdw_bus *bus,
 
 	/* Program DPN_BlockCtrl2 register*/
 	if (t_params->blk_grp_ctrl_valid) {
-		ret = sdw_write(s_rt->slave, addr5, t_params->blk_grp_ctrl);
+		ret = sdw_write_no_pm(s_rt->slave, addr5, t_params->blk_grp_ctrl);
 		if (ret < 0) {
 			dev_err(&s_rt->slave->dev,
 				"DPN_BlockCtrl2 reg write failed for port %d\n",
@@ -214,7 +214,7 @@  static int sdw_program_slave_port_params(struct sdw_bus *bus,
 
 	/* program DPN_LaneCtrl register */
 	if (slave_prop->lane_control_support) {
-		ret = sdw_write(s_rt->slave, addr6, t_params->lane_ctrl);
+		ret = sdw_write_no_pm(s_rt->slave, addr6, t_params->lane_ctrl);
 		if (ret < 0) {
 			dev_err(&s_rt->slave->dev,
 				"DPN_LaneCtrl register write failed for port %d\n",
@@ -319,9 +319,9 @@  static int sdw_enable_disable_slave_ports(struct sdw_bus *bus,
 	 * it is safe to reset this register
 	 */
 	if (en)
-		ret = sdw_write(s_rt->slave, addr, p_rt->ch_mask);
+		ret = sdw_write_no_pm(s_rt->slave, addr, p_rt->ch_mask);
 	else
-		ret = sdw_write(s_rt->slave, addr, 0x0);
+		ret = sdw_write_no_pm(s_rt->slave, addr, 0x0);
 
 	if (ret < 0)
 		dev_err(&s_rt->slave->dev,
@@ -476,9 +476,9 @@  static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
 		addr = SDW_DPN_PREPARECTRL(p_rt->num);
 
 		if (prep)
-			ret = sdw_write(s_rt->slave, addr, p_rt->ch_mask);
+			ret = sdw_write_no_pm(s_rt->slave, addr, p_rt->ch_mask);
 		else
-			ret = sdw_write(s_rt->slave, addr, 0x0);
+			ret = sdw_write_no_pm(s_rt->slave, addr, 0x0);
 
 		if (ret < 0) {
 			dev_err(&s_rt->slave->dev,
@@ -491,7 +491,7 @@  static int sdw_prep_deprep_slave_ports(struct sdw_bus *bus,
 		wait_for_completion_timeout(port_ready,
 			msecs_to_jiffies(dpn_prop->ch_prep_timeout));
 
-		val = sdw_read(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num));
+		val = sdw_read_no_pm(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num));
 		if ((val < 0) || (val & p_rt->ch_mask)) {
 			ret = (val < 0) ? val : -ETIMEDOUT;
 			dev_err(&s_rt->slave->dev,