Skip to contentSkip to conversation

spi: s3c64xx: simplify suspend/resume handlers

PATCH In review Heiner Kallweit 2 participants 3 messages Aug 18, 2015 – Aug 19, 2015 1 message not in the archive

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 3/3 spi: s3c64xx: simplify suspend/resume handlers

The runtime PM handlers take care of enabling / disabling the clock. Therefore use them to simplify the suspend / resume handlers.

pm_runtime_suspend/resume internally check whether suspending/resuming is possible and return 1 in case PM state is suspended/active already.

Currently there also seems to be a bug in the resume handler as suspend as well as resume check for !pm_runtime_disabled(dev).

---
 drivers/spi/spi-s3c64xx.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
drivers/spi/spi-s3c64xx.c +7 -8
index 15a6de1..78a6f04 100644
@@ -1242,10 +1242,9 @@ static int s3c64xx_spi_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (!pm_runtime_suspended(dev)) {
-		clk_disable_unprepare(sdd->clk);
-		clk_disable_unprepare(sdd->src_clk);
-	}
+	ret = pm_runtime_suspend(dev);
+	if (ret < 0)
+		return ret;
 
 	sdd->cur_speed = 0; /* Output Clock is stopped */
 
@@ -1257,14 +1256,14 @@ static int s3c64xx_spi_resume(struct device *dev)
 	struct spi_master *master = dev_get_drvdata(dev);
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
+	int ret;
 
 	if (sci->cfg_gpio)
 		sci->cfg_gpio();
 
-	if (!pm_runtime_suspended(dev)) {
-		clk_prepare_enable(sdd->src_clk);
-		clk_prepare_enable(sdd->clk);
-	}
+	ret = pm_runtime_resume(dev);
+	if (ret < 0)
+		return ret;
 
 	s3c64xx_spi_hwinit(sdd, sdd->port_id);
 
Signature · 1 line
2.5.0

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw
Quoted 8 lines · Heiner Kallweit

The runtime PM handlers take care of enabling / disabling the clock. Therefore use them to simplify the suspend / resume handlers.

pm_runtime_suspend/resume internally check whether suspending/resuming is possible and return 1 in case PM state is suspended/active already.

Currently there also seems to be a bug in the resume handler as suspend as well as resume check for !pm_runtime_disabled(dev).

Actually currently there is a bug because PM runtime is not disabled and concurrent runtime suspends/resumes can happen.

Quoted 11 lines
---
 drivers/spi/spi-s3c64xx.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
drivers/spi/spi-s3c64xx.c +0 -0
index 15a6de1..78a6f04 100644

drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_suspend(struct device *dev) · line 1245

Quoted 8 lines
drivers/spi/spi-s3c64xx.c +1 -4
@@ -1242,10 +1242,9 @@ static int s3c64xx_spi_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (!pm_runtime_suspended(dev)) {
-		clk_disable_unprepare(sdd->clk);
-		clk_disable_unprepare(sdd->src_clk);
-	}
+	ret = pm_runtime_suspend(dev);

I think you need pm_runtime_force_suspend() instead. Same for resume.

Best regards,
Krzysztof

Quoted 15 lines
drivers/spi/spi-s3c64xx.c +3 -0
+	if (ret < 0)
+		return ret;
 
 	sdd->cur_speed = 0; /* Output Clock is stopped */
 
@@ -1257,14 +1256,14 @@ static int s3c64xx_spi_resume(struct device *dev)
 	struct spi_master *master = dev_get_drvdata(dev);
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
+	int ret;
 
 	if (sci->cfg_gpio)
 		sci->cfg_gpio();
 

drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_resume(struct device *dev) · line 1269

Quoted 10 lines
drivers/spi/spi-s3c64xx.c +3 -4
-	if (!pm_runtime_suspended(dev)) {
-		clk_prepare_enable(sdd->src_clk);
-		clk_prepare_enable(sdd->clk);
-	}
+	ret = pm_runtime_resume(dev);
+	if (ret < 0)
+		return ret;
 
 	s3c64xx_spi_hwinit(sdd, sdd->port_id);
 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw

Am 19.08.2015 um 03:11 schrieb Krzysztof Kozlowski:

Quoted 27 lines · Heiner Kallweit
Quoted 8 lines

The runtime PM handlers take care of enabling / disabling the clock. Therefore use them to simplify the suspend / resume handlers.

pm_runtime_suspend/resume internally check whether suspending/resuming is possible and return 1 in case PM state is suspended/active already.

Currently there also seems to be a bug in the resume handler as suspend as well as resume check for !pm_runtime_disabled(dev).

Actually currently there is a bug because PM runtime is not disabled and concurrent runtime suspends/resumes can happen.

Quoted 14 lines
---
 drivers/spi/spi-s3c64xx.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
drivers/spi/spi-s3c64xx.c +0 -0
index 15a6de1..78a6f04 100644
@@ -1242,10 +1242,9 @@ static int s3c64xx_spi_suspend(struct device *dev)
 	if (ret)
 		return ret;
 
Quoted 7 lines

drivers/spi/spi-s3c64xx.c static int s3c64xx_spi_suspend(struct device *dev) · line 1245

Quoted 5 lines
drivers/spi/spi-s3c64xx.c +1 -4
-	if (!pm_runtime_suspended(dev)) {
-		clk_disable_unprepare(sdd->clk);
-		clk_disable_unprepare(sdd->src_clk);
-	}
+	ret = pm_runtime_suspend(dev);

I think you need pm_runtime_force_suspend() instead. Same for resume.

You're right. Will update the patch.

Regards, Heiner

Quoted 31 lines

Best regards,
Krzysztof

Quoted 25 lines
drivers/spi/spi-s3c64xx.c +6 -4
+	if (ret < 0)
+		return ret;
 
 	sdd->cur_speed = 0; /* Output Clock is stopped */
 
@@ -1257,14 +1256,14 @@ static int s3c64xx_spi_resume(struct device *dev)
 	struct spi_master *master = dev_get_drvdata(dev);
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 	struct s3c64xx_spi_info *sci = sdd->cntrlr_info;
+	int ret;
 
 	if (sci->cfg_gpio)
 		sci->cfg_gpio();
 
-	if (!pm_runtime_suspended(dev)) {
-		clk_prepare_enable(sdd->src_clk);
-		clk_prepare_enable(sdd->clk);
-	}
+	ret = pm_runtime_resume(dev);
+	if (ret < 0)
+		return ret;
 
 	s3c64xx_spi_hwinit(sdd, sdd->port_id);