[PATCH 0/4] spi s3c64xx ioclk handling

[PATCH 0/4] spi s3c64xx ioclk handling

[Andi Shyti]

Hi,

this patchset improves the compatibility between the s3c64xx
driver and the exynos5433. One point of incompatibility, indeed,
was that the SPI controller in exynos5433 is fed by three clocks
which need to be handled by the driver. The third clock is the
ioclk which support is the main purpose of this patchset.

The last patch is a bit unrelated, it restores a comment that has
been removed earlier by mistake.

Thanks,
Andi

Andi Shyti (4):
  spi: s3c64xx: rename goto labels to meaningful names
  bindings: spi-samsung: add exynos5433 spi compatible
  spi: s3c64xx: add nexynos5433 compatible for ioclk handling
  spi: s3c64xx: restore removed comments

 .../devicetree/bindings/spi/spi-samsung.txt        |  1 +
 drivers/spi/spi-s3c64xx.c                          | 83 +++++++++++++++++-----
 2 files changed, 67 insertions(+), 17 deletions(-)

-- 
2.8.1

[PATCH 1/4] spi: s3c64xx: rename goto labels to meaningful names

[Andi Shyti]

In goto labels of the style of

  err4:
  err3:
  err2:
  err1:

is complex to insert in between new errors without renaming all
the goto statments. Replace the errX naming style to meaningful
names in order to make it easier to insert new goto exit ponts.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 6f2e91a..2ab42b2 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1071,7 +1071,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		if (ret < 0) {
 			dev_err(&pdev->dev, "failed to get alias id, errno %d\n",
 				ret);
-			goto err0;
+			goto err_deref_master;
 		}
 		sdd->port_id = ret;
 	} else {
@@ -1109,13 +1109,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	sdd->regs = devm_ioremap_resource(&pdev->dev, mem_res);
 	if (IS_ERR(sdd->regs)) {
 		ret = PTR_ERR(sdd->regs);
-		goto err0;
+		goto err_deref_master;
 	}
 
 	if (sci->cfg_gpio && sci->cfg_gpio()) {
 		dev_err(&pdev->dev, "Unable to config gpio\n");
 		ret = -EBUSY;
-		goto err0;
+		goto err_deref_master;
 	}
 
 	/* Setup clocks */
@@ -1123,13 +1123,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	if (IS_ERR(sdd->clk)) {
 		dev_err(&pdev->dev, "Unable to acquire clock 'spi'\n");
 		ret = PTR_ERR(sdd->clk);
-		goto err0;
+		goto err_deref_master;
 	}
 
 	if (clk_prepare_enable(sdd->clk)) {
 		dev_err(&pdev->dev, "Couldn't enable clock 'spi'\n");
 		ret = -EBUSY;
-		goto err0;
+		goto err_deref_master;
 	}
 
 	sprintf(clk_name, "spi_busclk%d", sci->src_clk_nr);
@@ -1138,13 +1138,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev,
 			"Unable to acquire clock '%s'\n", clk_name);
 		ret = PTR_ERR(sdd->src_clk);
-		goto err2;
+		goto err_disable_clk;
 	}
 
 	if (clk_prepare_enable(sdd->src_clk)) {
 		dev_err(&pdev->dev, "Couldn't enable clock '%s'\n", clk_name);
 		ret = -EBUSY;
-		goto err2;
+		goto err_disable_clk;
 	}
 
 	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
@@ -1164,7 +1164,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	if (ret != 0) {
 		dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n",
 			irq, ret);
-		goto err3;
+		goto err_power_off;
 	}
 
 	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
@@ -1174,7 +1174,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 	ret = devm_spi_register_master(&pdev->dev, master);
 	if (ret != 0) {
 		dev_err(&pdev->dev, "cannot register SPI master: %d\n", ret);
-		goto err3;
+		goto err_power_off;
 	}
 
 	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Slaves attached\n",
@@ -1188,15 +1188,15 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 
 	return 0;
 
-err3:
+err_power_off:
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 
 	clk_disable_unprepare(sdd->src_clk);
-err2:
+err_disable_clk:
 	clk_disable_unprepare(sdd->clk);
-err0:
+err_deref_master:
 	spi_master_put(master);
 
 	return ret;
-- 
2.8.1

[PATCH 2/4] bindings: spi-samsung: add exynos5433 spi compatible

[Andi Shyti]

The samsung,exynos5433-spi has some peculiarities that bring the
need of creating a new compatible in the binding.

One of those is the 3-clocks controller management where the spi
is fed with three clocks: "spi", "busclkN" and "ioclk".

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 Documentation/devicetree/bindings/spi/spi-samsung.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 6dbdeb3..d4294c7 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
@@ -10,6 +10,7 @@ Required SoC Specific Properties:
     - samsung,s3c6410-spi: for s3c6410 platforms
     - samsung,s5pv210-spi: for s5pv210 and s5pc110 platforms
     - samsung,exynos7-spi: for exynos7 platforms
+    - samsung,exynos5433-spi: for exynos5433 compatible controllers
 
 - reg: physical base address of the controller and length of memory mapped
   region.
-- 
2.8.1

[PATCH 3/4] spi: s3c64xx: add nexynos5433 compatible for ioclk handling

[Andi Shyti]

The new compatible is related to the Samsunx exynos5433 SoC. The
difference between the previous is that in the exynos5433 the SPI
controller is driven by three clocks instead of only one.

The new clock (ioclk) is controlling the input/output clock
whenever the controller is slave or master.

The presence of the clock line is detected from the compatibility
structure (exynos5433_spi_port_config) as a boolean value.

The probe function checks whether the ioclk is present and if so,
it acquires.

The runtime suspend and resume functions will handle the clock
enabling and disabling as well.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 2ab42b2..1874bc5 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -156,12 +156,14 @@ struct s3c64xx_spi_port_config {
 	int	quirks;
 	bool	high_speed;
 	bool	clk_from_cmu;
+	bool	clk_ioclk;
 };
 
 /**
  * struct s3c64xx_spi_driver_data - Runtime info holder for SPI driver.
  * @clk: Pointer to the spi clock.
  * @src_clk: Pointer to the clock used to generate SPI signals.
+ * @ioclk: Pointer to the i/o clock between master and slave
  * @master: Pointer to the SPI Protocol master.
  * @cntrlr_info: Platform specific data for the controller this driver manages.
  * @tgl_spi: Pointer to the last CS left untoggled by the cs_change hint.
@@ -181,6 +183,7 @@ struct s3c64xx_spi_driver_data {
 	void __iomem                    *regs;
 	struct clk                      *clk;
 	struct clk                      *src_clk;
+	struct clk                      *ioclk;
 	struct platform_device          *pdev;
 	struct spi_master               *master;
 	struct s3c64xx_spi_info  *cntrlr_info;
@@ -1147,6 +1150,21 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
 		goto err_disable_clk;
 	}
 
+	if (sdd->port_conf->clk_ioclk) {
+		sdd->ioclk = devm_clk_get(&pdev->dev, "spi_ioclk");
+		if (IS_ERR(sdd->ioclk)) {
+			dev_err(&pdev->dev, "Unable to acquire 'ioclk'\n");
+			ret = PTR_ERR(sdd->ioclk);
+			goto err_disable_src_clk;
+		}
+
+		if (clk_prepare_enable(sdd->ioclk)) {
+			dev_err(&pdev->dev, "Couldn't enable clock 'ioclk'\n");
+			ret = -EBUSY;
+			goto err_disable_src_clk;
+		}
+	}
+
 	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
@@ -1193,6 +1211,8 @@ err_power_off:
 	pm_runtime_disable(&pdev->dev);
 	pm_runtime_set_suspended(&pdev->dev);
 
+	clk_disable_unprepare(sdd->ioclk);
+err_disable_src_clk:
 	clk_disable_unprepare(sdd->src_clk);
 err_disable_clk:
 	clk_disable_unprepare(sdd->clk);
@@ -1211,6 +1231,8 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 
 	writel(0, sdd->regs + S3C64XX_SPI_INT_EN);
 
+	clk_disable_unprepare(sdd->ioclk);
+
 	clk_disable_unprepare(sdd->src_clk);
 
 	clk_disable_unprepare(sdd->clk);
@@ -1269,6 +1291,7 @@ static int s3c64xx_spi_runtime_suspend(struct device *dev)
 
 	clk_disable_unprepare(sdd->clk);
 	clk_disable_unprepare(sdd->src_clk);
+	clk_disable_unprepare(sdd->ioclk);
 
 	return 0;
 }
@@ -1279,17 +1302,28 @@ static int s3c64xx_spi_runtime_resume(struct device *dev)
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
 	int ret;
 
+	if (sdd->port_conf->clk_ioclk) {
+		ret = clk_prepare_enable(sdd->ioclk);
+		if (ret != 0)
+			return ret;
+	}
+
 	ret = clk_prepare_enable(sdd->src_clk);
 	if (ret != 0)
-		return ret;
+		goto err_disable_ioclk;
 
 	ret = clk_prepare_enable(sdd->clk);
-	if (ret != 0) {
-		clk_disable_unprepare(sdd->src_clk);
-		return ret;
-	}
+	if (ret != 0)
+		goto err_disable_src_clk;
 
 	return 0;
+
+err_disable_src_clk:
+	clk_disable_unprepare(sdd->src_clk);
+err_disable_ioclk:
+	clk_disable_unprepare(sdd->ioclk);
+
+	return ret;
 }
 #endif /* CONFIG_PM */
 
@@ -1345,6 +1379,16 @@ static struct s3c64xx_spi_port_config exynos7_spi_port_config = {
 	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
 };
 
+static struct s3c64xx_spi_port_config exynos5433_spi_port_config = {
+	.fifo_lvl_mask	= { 0x1ff, 0x7F, 0x7F, 0x7F, 0x7F, 0x1ff},
+	.rx_lvl_offset	= 15,
+	.tx_st_done	= 25,
+	.high_speed	= true,
+	.clk_from_cmu	= true,
+	.clk_ioclk	= true,
+	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
+};
+
 static const struct platform_device_id s3c64xx_spi_driver_ids[] = {
 	{
 		.name		= "s3c2443-spi",
@@ -1375,6 +1419,9 @@ static const struct of_device_id s3c64xx_spi_dt_match[] = {
 	{ .compatible = "samsung,exynos7-spi",
 			.data = (void *)&exynos7_spi_port_config,
 	},
+	{ .compatible = "samsung,exynos5433-spi",
+			.data = (void *)&exynos5433_spi_port_config,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, s3c64xx_spi_dt_match);
-- 
2.8.1

[PATCH 4/4] spi: s3c64xx: restore removed comments

[Andi Shyti]

Patch a9e93e8 has erroneously removed some comments which are
important to understand why the bus frequency is multiplied by
two during the spi transfer.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 1874bc5..d1fe100 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -639,6 +639,8 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
 	writel(val, regs + S3C64XX_SPI_MODE_CFG);
 
 	if (sdd->port_conf->clk_from_cmu) {
+		/* Configure Clock */
+		/* There is half-multiplier before the SPI */
 		clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
 	} else {
 		/* Configure Clock */
-- 
2.8.1

Re: [PATCH 2/4] bindings: spi-samsung: add exynos5433 spi compatible

[Krzysztof Kozlowski]

On 07/08/2016 02:18 PM, Andi Shyti wrote:
> The samsung,exynos5433-spi has some peculiarities that bring the
> need of creating a new compatible in the binding.

That is not accurate. This for both: exynos5433 and exynos7.

> 
> One of those is the 3-clocks controller management where the spi
> is fed with three clocks: "spi", "busclkN" and "ioclk".
> 
> Signed-off-by: Andi Shyti <andi.shyti-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-samsung.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
> index 6dbdeb3..d4294c7 100644
> --- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
> +++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
> @@ -10,6 +10,7 @@ Required SoC Specific Properties:
>      - samsung,s3c6410-spi: for s3c6410 platforms
>      - samsung,s5pv210-spi: for s5pv210 and s5pc110 platforms
>      - samsung,exynos7-spi: for exynos7 platforms
> +    - samsung,exynos5433-spi: for exynos5433 compatible controllers

As I wrote offline, proper order (alpha-numerical), so 5 before 7.

>  
>  - reg: physical base address of the controller and length of memory mapped
>    region.
> 

Thanks for the patch, nice work. I see that you ignored parts of my
offline comments so I guess you leave that work to others (describe
required clock properties, deprecate exynos7-spi). I'll send the follow
up then.

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] spi: s3c64xx: restore removed comments

[Krzysztof Kozlowski]

On 07/08/2016 02:18 PM, Andi Shyti wrote:
> Patch a9e93e8 has erroneously removed some comments which are
> important to understand why the bus frequency is multiplied by
> two during the spi transfer.
> 
> Signed-off-by: Andi Shyti <andi.shyti-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/spi/spi-s3c64xx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 1874bc5..d1fe100 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -639,6 +639,8 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
>  	writel(val, regs + S3C64XX_SPI_MODE_CFG);
>  
>  	if (sdd->port_conf->clk_from_cmu) {
> +		/* Configure Clock */
> +		/* There is half-multiplier before the SPI */

Not a kernel coding style comment. :) Either proper one or just skip the
"configure clock" as it is meaningless.

BR,
Krzysztof

>  		clk_set_rate(sdd->src_clk, sdd->cur_speed * 2);
>  	} else {
>  		/* Configure Clock */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] bindings: spi-samsung: add exynos5433 spi compatible

[Andi Shyti]

> >  
> >  - reg: physical base address of the controller and length of memory mapped
> >    region.
> > 
> 
> Thanks for the patch, nice work. I see that you ignored parts of my
> offline comments so I guess you leave that work to others (describe
> required clock properties, deprecate exynos7-spi). I'll send the follow
> up then.

I was actually doing the follow up, I think it's a different
concept and therefore goes on a different patch.

Andi
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] bindings: spi-samsung: add exynos5433 spi compatible

[Krzysztof Kozlowski]

On 07/08/2016 02:56 PM, Andi Shyti wrote:
>>>  
>>>  - reg: physical base address of the controller and length of memory mapped
>>>    region.
>>>
>>
>> Thanks for the patch, nice work. I see that you ignored parts of my
>> offline comments so I guess you leave that work to others (describe
>> required clock properties, deprecate exynos7-spi). I'll send the follow
>> up then.
> 
> I was actually doing the follow up, I think it's a different
> concept and therefore goes on a different patch.

Ah.... but the exynos5433-spi compatible is related to three clocks -
you even mention it in commit message.

BR,
Krzysztof

Re: [PATCH 1/4] spi: s3c64xx: rename goto labels to meaningful names

[Krzysztof Kozlowski]

On 07/08/2016 02:18 PM, Andi Shyti wrote:
> In goto labels of the style of

s/In/The/

> 
>   err4:
>   err3:
>   err2:
>   err1:
> 
> is complex to insert in between new errors without renaming all

s/is/are/ (or just rewrite it... "The style of such goto labels... is")

> the goto statments. Replace the errX naming style to meaningful

s/statments/satements/

> names in order to make it easier to insert new goto exit ponts.

s/ponts/points/


> 
> Signed-off-by: Andi Shyti <andi.shyti-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/spi/spi-s3c64xx.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 6f2e91a..2ab42b2 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1071,7 +1071,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>  		if (ret < 0) {
>  			dev_err(&pdev->dev, "failed to get alias id, errno %d\n",
>  				ret);
> -			goto err0;
> +			goto err_deref_master;
>  		}
>  		sdd->port_id = ret;
>  	} else {
> @@ -1109,13 +1109,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>  	sdd->regs = devm_ioremap_resource(&pdev->dev, mem_res);
>  	if (IS_ERR(sdd->regs)) {
>  		ret = PTR_ERR(sdd->regs);
> -		goto err0;
> +		goto err_deref_master;
>  	}
>  
>  	if (sci->cfg_gpio && sci->cfg_gpio()) {
>  		dev_err(&pdev->dev, "Unable to config gpio\n");
>  		ret = -EBUSY;
> -		goto err0;
> +		goto err_deref_master;
>  	}
>  
>  	/* Setup clocks */
> @@ -1123,13 +1123,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>  	if (IS_ERR(sdd->clk)) {
>  		dev_err(&pdev->dev, "Unable to acquire clock 'spi'\n");
>  		ret = PTR_ERR(sdd->clk);
> -		goto err0;
> +		goto err_deref_master;
>  	}
>  
>  	if (clk_prepare_enable(sdd->clk)) {
>  		dev_err(&pdev->dev, "Couldn't enable clock 'spi'\n");
>  		ret = -EBUSY;
> -		goto err0;
> +		goto err_deref_master;
>  	}
>  
>  	sprintf(clk_name, "spi_busclk%d", sci->src_clk_nr);
> @@ -1138,13 +1138,13 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev,
>  			"Unable to acquire clock '%s'\n", clk_name);
>  		ret = PTR_ERR(sdd->src_clk);
> -		goto err2;
> +		goto err_disable_clk;
>  	}
>  
>  	if (clk_prepare_enable(sdd->src_clk)) {
>  		dev_err(&pdev->dev, "Couldn't enable clock '%s'\n", clk_name);
>  		ret = -EBUSY;
> -		goto err2;
> +		goto err_disable_clk;
>  	}
>  
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
> @@ -1164,7 +1164,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>  	if (ret != 0) {
>  		dev_err(&pdev->dev, "Failed to request IRQ %d: %d\n",
>  			irq, ret);
> -		goto err3;
> +		goto err_power_off;
>  	}
>  
>  	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
> @@ -1174,7 +1174,7 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>  	ret = devm_spi_register_master(&pdev->dev, master);
>  	if (ret != 0) {
>  		dev_err(&pdev->dev, "cannot register SPI master: %d\n", ret);
> -		goto err3;
> +		goto err_power_off;
>  	}
>  
>  	dev_dbg(&pdev->dev, "Samsung SoC SPI Driver loaded for Bus SPI-%d with %d Slaves attached\n",
> @@ -1188,15 +1188,15 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -err3:
> +err_power_off:

err_pm_put? There is no real power off here (noidle).

It's a nit so anyway:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Best regards,
Krzysztof

>  	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_set_suspended(&pdev->dev);
>  
>  	clk_disable_unprepare(sdd->src_clk);
> -err2:
> +err_disable_clk:
>  	clk_disable_unprepare(sdd->clk);
> -err0:
> +err_deref_master:
>  	spi_master_put(master);
>  
>  	return ret;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] spi: s3c64xx: rename goto labels to meaningful names

[Krzysztof Kozlowski]

On 07/08/2016 03:11 PM, Krzysztof Kozlowski wrote:
> On 07/08/2016 02:18 PM, Andi Shyti wrote:
>> In goto labels of the style of
> 
> s/In/The/
> 
>>
>>   err4:
>>   err3:
>>   err2:
>>   err1:
>>
>> is complex to insert in between new errors without renaming all
> 
> s/is/are/ (or just rewrite it... "The style of such goto labels... is")
> 
>> the goto statments. Replace the errX naming style to meaningful
> 
> s/statments/satements/

Yeah, nice job (to me)... error in error correction. Of course
"statements"...

BR,
Krzysztof

Re: [PATCH 3/4] spi: s3c64xx: add nexynos5433 compatible for ioclk handling

[Krzysztof Kozlowski]

On 07/08/2016 02:18 PM, Andi Shyti wrote:
> The new compatible is related to the Samsunx exynos5433 SoC. The

s/Samsunx/Samsung
Capitalize first letter: Exynos5433

> difference between the previous is that in the exynos5433 the SPI
> controller is driven by three clocks instead of only one.
> 
> The new clock (ioclk) is controlling the input/output clock
> whenever the controller is slave or master.
> 
> The presence of the clock line is detected from the compatibility
> structure (exynos5433_spi_port_config) as a boolean value.
> 
> The probe function checks whether the ioclk is present and if so,
> it acquires.
> 
> The runtime suspend and resume functions will handle the clock
> enabling and disabling as well.
> 
> Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 52 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 2ab42b2..1874bc5 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -156,12 +156,14 @@ struct s3c64xx_spi_port_config {
>  	int	quirks;
>  	bool	high_speed;
>  	bool	clk_from_cmu;
> +	bool	clk_ioclk;
>  };
>  
>  /**
>   * struct s3c64xx_spi_driver_data - Runtime info holder for SPI driver.
>   * @clk: Pointer to the spi clock.
>   * @src_clk: Pointer to the clock used to generate SPI signals.
> + * @ioclk: Pointer to the i/o clock between master and slave
>   * @master: Pointer to the SPI Protocol master.
>   * @cntrlr_info: Platform specific data for the controller this driver manages.
>   * @tgl_spi: Pointer to the last CS left untoggled by the cs_change hint.
> @@ -181,6 +183,7 @@ struct s3c64xx_spi_driver_data {
>  	void __iomem                    *regs;
>  	struct clk                      *clk;
>  	struct clk                      *src_clk;
> +	struct clk                      *ioclk;
>  	struct platform_device          *pdev;
>  	struct spi_master               *master;
>  	struct s3c64xx_spi_info  *cntrlr_info;
> @@ -1147,6 +1150,21 @@ static int s3c64xx_spi_probe(struct platform_device *pdev)
>  		goto err_disable_clk;
>  	}
>  
> +	if (sdd->port_conf->clk_ioclk) {
> +		sdd->ioclk = devm_clk_get(&pdev->dev, "spi_ioclk");
> +		if (IS_ERR(sdd->ioclk)) {
> +			dev_err(&pdev->dev, "Unable to acquire 'ioclk'\n");
> +			ret = PTR_ERR(sdd->ioclk);
> +			goto err_disable_src_clk;
> +		}
> +
> +		if (clk_prepare_enable(sdd->ioclk)) {
> +			dev_err(&pdev->dev, "Couldn't enable clock 'ioclk'\n");
> +			ret = -EBUSY;

Why not returning real error code from clk_prepare_enable()?

> +			goto err_disable_src_clk;
> +		}
> +	}
> +
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
> @@ -1193,6 +1211,8 @@ err_power_off:
>  	pm_runtime_disable(&pdev->dev);
>  	pm_runtime_set_suspended(&pdev->dev);
>  
> +	clk_disable_unprepare(sdd->ioclk);
> +err_disable_src_clk:
>  	clk_disable_unprepare(sdd->src_clk);
>  err_disable_clk:
>  	clk_disable_unprepare(sdd->clk);
> @@ -1211,6 +1231,8 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
>  
>  	writel(0, sdd->regs + S3C64XX_SPI_INT_EN);
>  
> +	clk_disable_unprepare(sdd->ioclk);
> +
>  	clk_disable_unprepare(sdd->src_clk);
>  
>  	clk_disable_unprepare(sdd->clk);
> @@ -1269,6 +1291,7 @@ static int s3c64xx_spi_runtime_suspend(struct device *dev)
>  
>  	clk_disable_unprepare(sdd->clk);
>  	clk_disable_unprepare(sdd->src_clk);
> +	clk_disable_unprepare(sdd->ioclk);
>  
>  	return 0;
>  }
> @@ -1279,17 +1302,28 @@ static int s3c64xx_spi_runtime_resume(struct device *dev)
>  	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
>  	int ret;
>  
> +	if (sdd->port_conf->clk_ioclk) {
> +		ret = clk_prepare_enable(sdd->ioclk);
> +		if (ret != 0)
> +			return ret;
> +	}
> +
>  	ret = clk_prepare_enable(sdd->src_clk);
>  	if (ret != 0)
> -		return ret;
> +		goto err_disable_ioclk;
>  
>  	ret = clk_prepare_enable(sdd->clk);
> -	if (ret != 0) {
> -		clk_disable_unprepare(sdd->src_clk);
> -		return ret;
> -	}
> +	if (ret != 0)
> +		goto err_disable_src_clk;
>  
>  	return 0;
> +
> +err_disable_src_clk:
> +	clk_disable_unprepare(sdd->src_clk);
> +err_disable_ioclk:
> +	clk_disable_unprepare(sdd->ioclk);
> +
> +	return ret;
>  }
>  #endif /* CONFIG_PM */
>  
> @@ -1345,6 +1379,16 @@ static struct s3c64xx_spi_port_config exynos7_spi_port_config = {
>  	.quirks		= S3C64XX_SPI_QUIRK_CS_AUTO,
>  };
>  
> +static struct s3c64xx_spi_port_config exynos5433_spi_port_config = {
> +	.fifo_lvl_mask	= { 0x1ff, 0x7F, 0x7F, 0x7F, 0x7F, 0x1ff},

Don't mix lower and upper cases of hex. I know that existing ones mixed
it... but let us fix at least new code.

Best regards,
Krzysztof