[PATCH 1/4] spi/s3c64xx: Fix handling of errors in gpio_request()

[PATCH 1/4] spi/s3c64xx: Fix handling of errors in gpio_request()

[Mark Brown]

When gpio_request() fails the driver logged the failure but while it'd
try to print an error code in the non-DT case it didn't pass the error
code in so garbage would be logged and in the DT case the error wasn't
logged.

Further, in the non-DT case the error code was then overwritten with -EBUSY
depriving the caller of information and breaking automatic probe deferral
pushing back from the GPIO level.  Also reformat the non-DT log message
so it's not word wrapped and we can grep for it.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/spi/spi-s3c64xx.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 999154a0..7258b18 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -894,9 +894,9 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 	if (!spi_get_ctldata(spi)) {
 		err = gpio_request(cs->line, dev_name(&spi->dev));
 		if (err) {
-			dev_err(&spi->dev, "request for slave select gpio "
-					"line [%d] failed\n", cs->line);
-			err = -EBUSY;
+			dev_err(&spi->dev,
+				"Failed to get /CS gpio [%d]: %d\n",
+				cs->line, err);
 			goto err_gpio_req;
 		}
 		spi_set_ctldata(spi, cs);
@@ -1114,7 +1114,8 @@ static int s3c64xx_spi_parse_dt_gpio(struct s3c64xx_spi_driver_data *sdd)
 
 		ret = gpio_request(gpio, "spi-bus");
 		if (ret) {
-			dev_err(dev, "gpio [%d] request failed\n", gpio);
+			dev_err(dev, "gpio [%d] request failed: %d\n",
+				gpio, ret);
 			goto free_gpio;
 		}
 	}
-- 
1.7.10.4

[PATCH 2/4] spi/s3c64xx: Put the /CS GPIO into output mode

[Mark Brown]

No call was being made by the GPIO driver to put the GPIO into output
mode meaning that the calls to gpio_set_value() which were being done
were not valid. A similar issue appears to exist with the DT GPIO
requests but as they appear to be being used for pinmux it's less clear
to me that we want to configure them.

Without this fix Cragganmore systems can't talk to their SPI devices.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/spi/spi-s3c64xx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 7258b18..6ed3ba8 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -892,7 +892,8 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 	}
 
 	if (!spi_get_ctldata(spi)) {
-		err = gpio_request(cs->line, dev_name(&spi->dev));
+		err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH,
+				       dev_name(&spi->dev));
 		if (err) {
 			dev_err(&spi->dev,
 				"Failed to get /CS gpio [%d]: %d\n",
-- 
1.7.10.4

[PATCH 3/4] spi/s3c64xx: Convert to devm_request_and_ioremap()

[Mark Brown]

Saves some error handling and a small amount of code.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-s3c64xx.c |   18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 6ed3ba8..1781365 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1278,14 +1278,7 @@ static int __init s3c64xx_spi_probe(struct platform_device *pdev)
 	/* the spi->mode bits understood by this driver: */
 	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
 
-	if (request_mem_region(mem_res->start,
-			resource_size(mem_res), pdev->name) == NULL) {
-		dev_err(&pdev->dev, "Req mem region failed\n");
-		ret = -ENXIO;
-		goto err0;
-	}
-
-	sdd->regs = ioremap(mem_res->start, resource_size(mem_res));
+	sdd->regs = devm_request_and_ioremap(&pdev->dev, mem_res);
 	if (sdd->regs == NULL) {
 		dev_err(&pdev->dev, "Unable to remap IO\n");
 		ret = -ENXIO;
@@ -1379,9 +1372,7 @@ err3:
 	if (!sdd->cntrlr_info->cfg_gpio && pdev->dev.of_node)
 		s3c64xx_spi_dt_gpio_free(sdd);
 err2:
-	iounmap((void *) sdd->regs);
 err1:
-	release_mem_region(mem_res->start, resource_size(mem_res));
 err0:
 	platform_set_drvdata(pdev, NULL);
 	spi_master_put(master);
@@ -1393,7 +1384,6 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 {
 	struct spi_master *master = spi_master_get(platform_get_drvdata(pdev));
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(master);
-	struct resource	*mem_res;
 
 	pm_runtime_disable(&pdev->dev);
 
@@ -1412,12 +1402,6 @@ static int s3c64xx_spi_remove(struct platform_device *pdev)
 	if (!sdd->cntrlr_info->cfg_gpio && pdev->dev.of_node)
 		s3c64xx_spi_dt_gpio_free(sdd);
 
-	iounmap((void *) sdd->regs);
-
-	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (mem_res != NULL)
-		release_mem_region(mem_res->start, resource_size(mem_res));
-
 	platform_set_drvdata(pdev, NULL);
 	spi_master_put(master);
 
-- 
1.7.10.4

[PATCH 4/4] spi/s3c64xx: Expand S3C64XX_SPI_{DE,}ACT macros at call sites

[Mark Brown]

They have very few users and they're both just doing a single register
write so the advantage of having the macro is a bit limited. An inline
function might make sense but it's as easy to just do the writes directly.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/spi/spi-s3c64xx.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 1781365..d24ec80 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -79,11 +79,6 @@
 #define S3C64XX_SPI_SLAVE_AUTO			(1<<1)
 #define S3C64XX_SPI_SLAVE_SIG_INACT		(1<<0)
 
-#define S3C64XX_SPI_ACT(c) writel(0, (c)->regs + S3C64XX_SPI_SLAVE_SEL)
-
-#define S3C64XX_SPI_DEACT(c) writel(S3C64XX_SPI_SLAVE_SIG_INACT, \
-					(c)->regs + S3C64XX_SPI_SLAVE_SEL)
-
 #define S3C64XX_SPI_INT_TRAILING_EN		(1<<6)
 #define S3C64XX_SPI_INT_RX_OVERRUN_EN		(1<<5)
 #define S3C64XX_SPI_INT_RX_UNDERRUN_EN		(1<<4)
@@ -737,14 +732,15 @@ static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 		enable_cs(sdd, spi);
 
 		/* Start the signals */
-		S3C64XX_SPI_ACT(sdd);
+		writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 
 		spin_unlock_irqrestore(&sdd->lock, flags);
 
 		status = wait_for_xfer(sdd, xfer, use_dma);
 
 		/* Quiese the signals */
-		S3C64XX_SPI_DEACT(sdd);
+		writel(S3C64XX_SPI_SLAVE_SIG_INACT,
+		       sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 
 		if (status) {
 			dev_err(&spi->dev, "I/O Error: "
@@ -1030,7 +1026,7 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
 
 	sdd->cur_speed = 0;
 
-	S3C64XX_SPI_DEACT(sdd);
+	writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 
 	/* Disable Interrupts - we use Polling if not DMA mode */
 	writel(0, regs + S3C64XX_SPI_INT_EN);
-- 
1.7.10.4

RE: [PATCH 1/4] spi/s3c64xx: Fix handling of errors in gpio_request()

[Kukjin Kim]

Mark Brown wrote:
> 
> When gpio_request() fails the driver logged the failure but while it'd
> try to print an error code in the non-DT case it didn't pass the error
> code in so garbage would be logged and in the DT case the error wasn't
> logged.
> 
> Further, in the non-DT case the error code was then overwritten with -
> EBUSY
> depriving the caller of information and breaking automatic probe deferral
> pushing back from the GPIO level.  Also reformat the non-DT log message
> so it's not word wrapped and we can grep for it.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/spi/spi-s3c64xx.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 999154a0..7258b18 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -894,9 +894,9 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>  	if (!spi_get_ctldata(spi)) {
>  		err = gpio_request(cs->line, dev_name(&spi->dev));
>  		if (err) {
> -			dev_err(&spi->dev, "request for slave select gpio "
> -					"line [%d] failed\n", cs->line);
> -			err = -EBUSY;
> +			dev_err(&spi->dev,
> +				"Failed to get /CS gpio [%d]: %d\n",
> +				cs->line, err);
>  			goto err_gpio_req;
>  		}
>  		spi_set_ctldata(spi, cs);
> @@ -1114,7 +1114,8 @@ static int s3c64xx_spi_parse_dt_gpio(struct
> s3c64xx_spi_driver_data *sdd)
> 
>  		ret = gpio_request(gpio, "spi-bus");
>  		if (ret) {
> -			dev_err(dev, "gpio [%d] request failed\n", gpio);
> +			dev_err(dev, "gpio [%d] request failed: %d\n",
> +				gpio, ret);
>  			goto free_gpio;
>  		}
>  	}
> --
> 1.7.10.4

Mark, thanks for your re-work 3th and 4th patches in this series :-)

Looks good to me, applied this series.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.