[PATCH v2 0/5] spi: dw: device tree and generic queue support

[PATCH v2 0/5] spi: dw: device tree and generic queue support

[Baruch Siach]

Unfortunately, I probably have no chance access to thest this series on
hardware, I's only compile tested. So I'm sending this as RFC for code review.
If this series doesn't get merged, it may be used at least as a starting point
for someone with hardware access.

These patches add the following:

   * Device tree support for the platform driver (dw-mmio)

   * GPIO controlled chip-select implementing the generic SPI GPIO CS binding

   * Generic queue migration

Comments are welcome.

v2:
   * Use managed resources for GPIO allocation (Mark Brown)

   * Add device tree binding documentation (Mark Brown)

Baruch Siach (5):
  spi: dw-mmio: allow multiple instances
  spi: dw-mmio: add device tree support
  spi: dw: add support for gpio controlled chip select
  spi: dw: document device tree binding
  spi: dw: migrate to generic queue infrastructure

 Documentation/devicetree/bindings/spi/spi-dw.txt |  24 +++
 drivers/spi/spi-dw-mmio.c                        |  19 ++-
 drivers/spi/spi-dw.c                             | 209 ++++-------------------
 drivers/spi/spi-dw.h                             |  14 +-
 4 files changed, 81 insertions(+), 185 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-dw.txt

-- 
1.8.5.2

--
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

[PATCH v2 1/5] spi: dw-mmio: allow multiple instances

[Baruch Siach]

Use the platform_device id to uniquely identify each SPI master instance.

Cc: Jean-Hugues Deschenes <jean-hugues.deschenes-YGVykHU+fedBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/spi/spi-dw-mmio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 9af56cdf1540..1492f5ee9aaa 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -66,7 +66,7 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	dws->bus_num = 0;
+	dws->bus_num = pdev->id;
 	dws->num_cs = 4;
 	dws->max_freq = clk_get_rate(dwsmmio->clk);
 
-- 
1.8.5.2

--
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

[PATCH v2 2/5] spi: dw-mmio: add device tree support

[Baruch Siach]

Cc: Jean-Hugues Deschenes <jean-hugues.deschenes-YGVykHU+fedBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/spi/spi-dw-mmio.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c
index 1492f5ee9aaa..b694f3f9b901 100644
--- a/drivers/spi/spi-dw-mmio.c
+++ b/drivers/spi/spi-dw-mmio.c
@@ -16,6 +16,7 @@
 #include <linux/spi/spi.h>
 #include <linux/scatterlist.h>
 #include <linux/module.h>
+#include <linux/of.h>
 
 #include "spi-dw.h"
 
@@ -32,6 +33,7 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 	struct dw_spi *dws;
 	struct resource *mem;
 	int ret;
+	u32 tmp;
 
 	dwsmmio = devm_kzalloc(&pdev->dev, sizeof(struct dw_spi_mmio),
 			GFP_KERNEL);
@@ -67,7 +69,11 @@ static int dw_spi_mmio_probe(struct platform_device *pdev)
 		return ret;
 
 	dws->bus_num = pdev->id;
-	dws->num_cs = 4;
+	ret = of_property_read_u32(pdev->dev.of_node, "num-cs", &tmp);
+	if (ret == 0)
+		dws->num_cs = tmp;
+	else
+		dws->num_cs = 4;
 	dws->max_freq = clk_get_rate(dwsmmio->clk);
 
 	ret = dw_spi_add_host(&pdev->dev, dws);
@@ -92,12 +98,21 @@ static int dw_spi_mmio_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id dw_spi_dt_ids[] = {
+	{ .compatible = "snps,designware-spi" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, dw_spi_dt_ids);
+#endif
+
 static struct platform_driver dw_spi_mmio_driver = {
 	.probe		= dw_spi_mmio_probe,
 	.remove		= dw_spi_mmio_remove,
 	.driver		= {
 		.name	= DRIVER_NAME,
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(dw_spi_dt_ids),
 	},
 };
 module_platform_driver(dw_spi_mmio_driver);
-- 
1.8.5.2

--
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

[PATCH v2 3/5] spi: dw: add support for gpio controlled chip select

[Baruch Siach]

Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/spi/spi-dw.c | 26 +++++++++++++++++++++++---
 drivers/spi/spi-dw.h |  6 +++++-
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index bf98d63d92b3..69b8772b3ed8 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -24,6 +24,7 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
+#include <linux/gpio.h>
 
 #include "spi-dw.h"
 
@@ -265,6 +266,8 @@ static void giveback(struct dw_spi *dws)
 	struct spi_transfer *last_transfer;
 	unsigned long flags;
 	struct spi_message *msg;
+	int cs_gpio = dws->cur_msg->spi->cs_gpio;
+	u16 mode = dws->cur_msg->spi->mode;
 
 	spin_lock_irqsave(&dws->lock, flags);
 	msg = dws->cur_msg;
@@ -280,8 +283,12 @@ static void giveback(struct dw_spi *dws)
 					struct spi_transfer,
 					transfer_list);
 
-	if (!last_transfer->cs_change && dws->cs_control)
-		dws->cs_control(MRST_SPI_DEASSERT);
+	if (!last_transfer->cs_change) {
+		if (dws->cs_control)
+			dws->cs_control(MRST_SPI_DEASSERT);
+		if (gpio_is_valid(cs_gpio))
+			gpio_set_value(cs_gpio, !(mode & SPI_CS_HIGH));
+	}
 
 	msg->state = NULL;
 	if (msg->complete)
@@ -509,7 +516,8 @@ static void pump_transfers(unsigned long data)
 			dw_writew(dws, DW_SPI_CTRL0, cr0);
 
 		spi_set_clk(dws, clk_div ? clk_div : chip->clk_div);
-		spi_chip_sel(dws, spi->chip_select);
+		spi_chip_sel(dws, spi->chip_select, spi->cs_gpio,
+				spi->mode & SPI_CS_HIGH);
 
 		/* Set the interrupt mask, for poll mode just disable all int */
 		spi_mask_intr(dws, 0xff);
@@ -615,6 +623,7 @@ static int dw_spi_setup(struct spi_device *spi)
 {
 	struct dw_spi_chip *chip_info = NULL;
 	struct chip_data *chip;
+	int ret;
 
 	/* Only alloc on first setup */
 	chip = spi_get_ctldata(spi);
@@ -668,6 +677,17 @@ static int dw_spi_setup(struct spi_device *spi)
 			| (spi->mode  << SPI_MODE_OFFSET)
 			| (chip->tmode << SPI_TMOD_OFFSET);
 
+	if (gpio_is_valid(spi->cs_gpio)) {
+		ret = devm_gpio_request(&spi->dev, spi->cs_gpio,
+				dev_name(&spi->dev));
+		if (ret)
+			return ret;
+		ret = gpio_direction_output(spi->cs_gpio,
+				!(spi->mode & SPI_CS_HIGH));
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 587643dae11e..4a3a6d764b48 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -3,6 +3,7 @@
 
 #include <linux/io.h>
 #include <linux/scatterlist.h>
+#include <linux/gpio.h>
 
 /* Register offsets */
 #define DW_SPI_CTRL0			0x00
@@ -186,13 +187,16 @@ static inline void spi_set_clk(struct dw_spi *dws, u16 div)
 	dw_writel(dws, DW_SPI_BAUDR, div);
 }
 
-static inline void spi_chip_sel(struct dw_spi *dws, u16 cs)
+static inline void spi_chip_sel(struct dw_spi *dws, u16 cs, int cs_gpio,
+		int gpio_active_val)
 {
 	if (cs > dws->num_cs)
 		return;
 
 	if (dws->cs_control)
 		dws->cs_control(1);
+	if (gpio_is_valid(cs_gpio))
+		gpio_set_value(cs_gpio, gpio_active_val);
 
 	dw_writel(dws, DW_SPI_SER, 1 << cs);
 }
-- 
1.8.5.2

--
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

[PATCH v2 4/5] spi: dw: document device tree binding

[Baruch Siach]

Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 Documentation/devicetree/bindings/spi/spi-dw.txt | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-dw.txt

diff --git a/Documentation/devicetree/bindings/spi/spi-dw.txt b/Documentation/devicetree/bindings/spi/spi-dw.txt
new file mode 100644
index 000000000000..7b63ed601990
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-dw.txt
@@ -0,0 +1,24 @@
+Synopsys DesignWare SPI master
+
+Required properties:
+- compatible: should be "snps,designware-spi"
+- #address-cells: see spi-bus.txt
+- #size-cells: see spi-bus.txt
+- reg: address and length of the spi master registers
+- interrupts: should contain one interrupt
+- clocks: spi clock phandle
+- num-cs: see spi-bus.txt
+
+Optional properties:
+- cs-gpios: see spi-bus.txt
+
+Example:
+
+spi: spi@4020a000 {
+	compatible = "snps,designware-spi";
+	interrupts = <11 1>;
+	reg = <0x4020a000 0x1000>;
+	clocks = <&pclk>;
+	num-cs = <2>;
+	cs-gpios = <&banka 0 0>;
+};
-- 
1.8.5.2

--
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

[PATCH v2 5/5] spi: dw: migrate to generic queue infrastructure

[Baruch Siach]

Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/spi/spi-dw.c | 183 ++++-----------------------------------------------
 drivers/spi/spi-dw.h |   8 ---
 2 files changed, 12 insertions(+), 179 deletions(-)

diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
index 69b8772b3ed8..c7cd19fe7d1a 100644
--- a/drivers/spi/spi-dw.c
+++ b/drivers/spi/spi-dw.c
@@ -37,9 +37,6 @@
 #define DONE_STATE	((void *)2)
 #define ERROR_STATE	((void *)-1)
 
-#define QUEUE_RUNNING	0
-#define QUEUE_STOPPED	1
-
 #define MRST_SPI_DEASSERT	0
 #define MRST_SPI_ASSERT		1
 
@@ -264,20 +261,16 @@ static int map_dma_buffers(struct dw_spi *dws)
 static void giveback(struct dw_spi *dws)
 {
 	struct spi_transfer *last_transfer;
-	unsigned long flags;
 	struct spi_message *msg;
 	int cs_gpio = dws->cur_msg->spi->cs_gpio;
 	u16 mode = dws->cur_msg->spi->mode;
 
-	spin_lock_irqsave(&dws->lock, flags);
 	msg = dws->cur_msg;
 	dws->cur_msg = NULL;
 	dws->cur_transfer = NULL;
 	dws->prev_chip = dws->cur_chip;
 	dws->cur_chip = NULL;
 	dws->dma_mapped = 0;
-	queue_work(dws->workqueue, &dws->pump_messages);
-	spin_unlock_irqrestore(&dws->lock, flags);
 
 	last_transfer = list_entry(msg->transfers.prev,
 					struct spi_transfer,
@@ -290,9 +283,7 @@ static void giveback(struct dw_spi *dws)
 			gpio_set_value(cs_gpio, !(mode & SPI_CS_HIGH));
 	}
 
-	msg->state = NULL;
-	if (msg->complete)
-		msg->complete(msg->context);
+	spi_finalize_current_message(dws->master);
 }
 
 static void int_error_stop(struct dw_spi *dws, const char *msg)
@@ -544,30 +535,12 @@ early_exit:
 	return;
 }
 
-static void pump_messages(struct work_struct *work)
+static int dw_spi_transfer_one_message(struct spi_master *master,
+		struct spi_message *msg)
 {
-	struct dw_spi *dws =
-		container_of(work, struct dw_spi, pump_messages);
-	unsigned long flags;
-
-	/* Lock queue and check for queue work */
-	spin_lock_irqsave(&dws->lock, flags);
-	if (list_empty(&dws->queue) || dws->run == QUEUE_STOPPED) {
-		dws->busy = 0;
-		spin_unlock_irqrestore(&dws->lock, flags);
-		return;
-	}
-
-	/* Make sure we are not already running a message */
-	if (dws->cur_msg) {
-		spin_unlock_irqrestore(&dws->lock, flags);
-		return;
-	}
-
-	/* Extract head of queue */
-	dws->cur_msg = list_entry(dws->queue.next, struct spi_message, queue);
-	list_del_init(&dws->cur_msg->queue);
+	struct dw_spi *dws = spi_master_get_devdata(master);
 
+	dws->cur_msg = msg;
 	/* Initial message state*/
 	dws->cur_msg->state = START_STATE;
 	dws->cur_transfer = list_entry(dws->cur_msg->transfers.next,
@@ -575,46 +548,9 @@ static void pump_messages(struct work_struct *work)
 						transfer_list);
 	dws->cur_chip = spi_get_ctldata(dws->cur_msg->spi);
 
-	/* Mark as busy and launch transfers */
+	/* Launch transfers */
 	tasklet_schedule(&dws->pump_transfers);
 
-	dws->busy = 1;
-	spin_unlock_irqrestore(&dws->lock, flags);
-}
-
-/* spi_device use this to queue in their spi_msg */
-static int dw_spi_transfer(struct spi_device *spi, struct spi_message *msg)
-{
-	struct dw_spi *dws = spi_master_get_devdata(spi->master);
-	unsigned long flags;
-
-	spin_lock_irqsave(&dws->lock, flags);
-
-	if (dws->run == QUEUE_STOPPED) {
-		spin_unlock_irqrestore(&dws->lock, flags);
-		return -ESHUTDOWN;
-	}
-
-	msg->actual_length = 0;
-	msg->status = -EINPROGRESS;
-	msg->state = START_STATE;
-
-	list_add_tail(&msg->queue, &dws->queue);
-
-	if (dws->run == QUEUE_RUNNING && !dws->busy) {
-
-		if (dws->cur_transfer || dws->cur_msg)
-			queue_work(dws->workqueue,
-					&dws->pump_messages);
-		else {
-			/* If no other data transaction in air, just go */
-			spin_unlock_irqrestore(&dws->lock, flags);
-			pump_messages(&dws->pump_messages);
-			return 0;
-		}
-	}
-
-	spin_unlock_irqrestore(&dws->lock, flags);
 	return 0;
 }
 
@@ -697,81 +633,6 @@ static void dw_spi_cleanup(struct spi_device *spi)
 	kfree(chip);
 }
 
-static int init_queue(struct dw_spi *dws)
-{
-	INIT_LIST_HEAD(&dws->queue);
-	spin_lock_init(&dws->lock);
-
-	dws->run = QUEUE_STOPPED;
-	dws->busy = 0;
-
-	tasklet_init(&dws->pump_transfers,
-			pump_transfers,	(unsigned long)dws);
-
-	INIT_WORK(&dws->pump_messages, pump_messages);
-	dws->workqueue = create_singlethread_workqueue(
-					dev_name(dws->master->dev.parent));
-	if (dws->workqueue == NULL)
-		return -EBUSY;
-
-	return 0;
-}
-
-static int start_queue(struct dw_spi *dws)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&dws->lock, flags);
-
-	if (dws->run == QUEUE_RUNNING || dws->busy) {
-		spin_unlock_irqrestore(&dws->lock, flags);
-		return -EBUSY;
-	}
-
-	dws->run = QUEUE_RUNNING;
-	dws->cur_msg = NULL;
-	dws->cur_transfer = NULL;
-	dws->cur_chip = NULL;
-	dws->prev_chip = NULL;
-	spin_unlock_irqrestore(&dws->lock, flags);
-
-	queue_work(dws->workqueue, &dws->pump_messages);
-
-	return 0;
-}
-
-static int stop_queue(struct dw_spi *dws)
-{
-	unsigned long flags;
-	unsigned limit = 50;
-	int status = 0;
-
-	spin_lock_irqsave(&dws->lock, flags);
-	dws->run = QUEUE_STOPPED;
-	while ((!list_empty(&dws->queue) || dws->busy) && limit--) {
-		spin_unlock_irqrestore(&dws->lock, flags);
-		msleep(10);
-		spin_lock_irqsave(&dws->lock, flags);
-	}
-
-	if (!list_empty(&dws->queue) || dws->busy)
-		status = -EBUSY;
-	spin_unlock_irqrestore(&dws->lock, flags);
-
-	return status;
-}
-
-static int destroy_queue(struct dw_spi *dws)
-{
-	int status;
-
-	status = stop_queue(dws);
-	if (status != 0)
-		return status;
-	destroy_workqueue(dws->workqueue);
-	return 0;
-}
-
 /* Restart the controller, disable all interrupts, clean rx fifo */
 static void spi_hw_init(struct dw_spi *dws)
 {
@@ -828,7 +689,7 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 	master->num_chipselect = dws->num_cs;
 	master->cleanup = dw_spi_cleanup;
 	master->setup = dw_spi_setup;
-	master->transfer = dw_spi_transfer;
+	master->transfer_one_message = dw_spi_transfer_one_message;
 
 	/* Basic HW init */
 	spi_hw_init(dws);
@@ -841,33 +702,21 @@ int dw_spi_add_host(struct device *dev, struct dw_spi *dws)
 		}
 	}
 
-	/* Initial and start queue */
-	ret = init_queue(dws);
-	if (ret) {
-		dev_err(&master->dev, "problem initializing queue\n");
-		goto err_diable_hw;
-	}
-	ret = start_queue(dws);
-	if (ret) {
-		dev_err(&master->dev, "problem starting queue\n");
-		goto err_diable_hw;
-	}
+	tasklet_init(&dws->pump_transfers, pump_transfers, (unsigned long)dws);
 
 	spi_master_set_devdata(master, dws);
 	ret = devm_spi_register_master(dev, master);
 	if (ret) {
 		dev_err(&master->dev, "problem registering spi master\n");
-		goto err_queue_alloc;
+		goto err_dma_exit;
 	}
 
 	mrst_spi_debugfs_init(dws);
 	return 0;
 
-err_queue_alloc:
-	destroy_queue(dws);
+err_dma_exit:
 	if (dws->dma_ops && dws->dma_ops->dma_exit)
 		dws->dma_ops->dma_exit(dws);
-err_diable_hw:
 	spi_enable_chip(dws, 0);
 err_free_master:
 	spi_master_put(master);
@@ -877,18 +726,10 @@ EXPORT_SYMBOL_GPL(dw_spi_add_host);
 
 void dw_spi_remove_host(struct dw_spi *dws)
 {
-	int status = 0;
-
 	if (!dws)
 		return;
 	mrst_spi_debugfs_remove(dws);
 
-	/* Remove the queue */
-	status = destroy_queue(dws);
-	if (status != 0)
-		dev_err(&dws->master->dev,
-			"dw_spi_remove: workqueue will not complete, message memory not freed\n");
-
 	if (dws->dma_ops && dws->dma_ops->dma_exit)
 		dws->dma_ops->dma_exit(dws);
 	spi_enable_chip(dws, 0);
@@ -901,7 +742,7 @@ int dw_spi_suspend_host(struct dw_spi *dws)
 {
 	int ret = 0;
 
-	ret = stop_queue(dws);
+	ret = spi_master_suspend(dws->master);
 	if (ret)
 		return ret;
 	spi_enable_chip(dws, 0);
@@ -915,7 +756,7 @@ int dw_spi_resume_host(struct dw_spi *dws)
 	int ret;
 
 	spi_hw_init(dws);
-	ret = start_queue(dws);
+	ret = spi_master_resume(dws->master);
 	if (ret)
 		dev_err(&dws->master->dev, "fail to start queue (%d)\n", ret);
 	return ret;
diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
index 4a3a6d764b48..39844f262bd1 100644
--- a/drivers/spi/spi-dw.h
+++ b/drivers/spi/spi-dw.h
@@ -105,14 +105,6 @@ struct dw_spi {
 	u16			bus_num;
 	u16			num_cs;		/* supported slave numbers */
 
-	/* Driver message queue */
-	struct workqueue_struct	*workqueue;
-	struct work_struct	pump_messages;
-	spinlock_t		lock;
-	struct list_head	queue;
-	int			busy;
-	int			run;
-
 	/* Message Transfer pump */
 	struct tasklet_struct	pump_transfers;
 
-- 
1.8.5.2

--
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 v2 2/5] spi: dw-mmio: add device tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 383 bytes --]

On Sun, Jan 26, 2014 at 10:14:33AM +0200, Baruch Siach wrote:
> Cc: Jean-Hugues Deschenes <jean-hugues.deschenes-YGVykHU+fedBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> ---
>  drivers/spi/spi-dw-mmio.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)

Any new bindings need to be documented.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/5] spi: dw-mmio: add device tree support

[Baruch Siach]

Hi Mark,

On Mon, Jan 27, 2014 at 06:50:04PM +0000, Mark Brown wrote:
> On Sun, Jan 26, 2014 at 10:14:33AM +0200, Baruch Siach wrote:
> > Cc: Jean-Hugues Deschenes <jean-hugues.deschenes-YGVykHU+fedBDgjK7y7TUQ@public.gmane.org>
> > Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> > ---
> >  drivers/spi/spi-dw-mmio.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> Any new bindings need to be documented.

It should be all documented in patch 4. Is there anything missing?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
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 v2 3/5] spi: dw: add support for gpio controlled chip select

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

On Sun, Jan 26, 2014 at 10:14:34AM +0200, Baruch Siach wrote:

>  
> -	if (!last_transfer->cs_change && dws->cs_control)
> -		dws->cs_control(MRST_SPI_DEASSERT);
> +	if (!last_transfer->cs_change) {
> +		if (dws->cs_control)
> +			dws->cs_control(MRST_SPI_DEASSERT);
> +		if (gpio_is_valid(cs_gpio))
> +			gpio_set_value(cs_gpio, !(mode & SPI_CS_HIGH));
> +	}

Why is this not using spi_chip_sel()?  I know the old code wasn't either
but since both sites need to be changed...

> +		spi_chip_sel(dws, spi->chip_select, spi->cs_gpio,
> +				spi->mode & SPI_CS_HIGH);

I would expect the /CS polarity handling to be abstracted inside the
function?

> +	if (gpio_is_valid(spi->cs_gpio)) {
> +		ret = devm_gpio_request(&spi->dev, spi->cs_gpio,
> +				dev_name(&spi->dev));
> +		if (ret)
> +			return ret;
> +		ret = gpio_direction_output(spi->cs_gpio,
> +				!(spi->mode & SPI_CS_HIGH));
> +		if (ret)
> +			return ret;
> +	}

Should be devm_gpio_request_one().  This won't work with probe deferral,
it's outside the probe() function so nothing will retry.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/5] spi: dw-mmio: add device tree support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

On Mon, Jan 27, 2014 at 08:54:44PM +0200, Baruch Siach wrote:
> On Mon, Jan 27, 2014 at 06:50:04PM +0000, Mark Brown wrote:

> > Any new bindings need to be documented.

> It should be all documented in patch 4. Is there anything missing?

Gah, please document bindings before adding the code - given that it's a
hard requirement I didn't even get past the diffstat here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 1/5] spi: dw-mmio: allow multiple instances

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 156 bytes --]

On Sun, Jan 26, 2014 at 10:14:32AM +0200, Baruch Siach wrote:
> Use the platform_device id to uniquely identify each SPI master instance.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 4/5] spi: dw: document device tree binding

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 209 bytes --]

On Sun, Jan 26, 2014 at 10:14:35AM +0200, Baruch Siach wrote:

> +Required properties:

> +- num-cs: see spi-bus.txt

Why is this mandatory, in the code it was optional?  One or the other
ought to be updated.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 5/5] spi: dw: migrate to generic queue infrastructure

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 159 bytes --]

On Sun, Jan 26, 2014 at 10:14:36AM +0200, Baruch Siach wrote:
> Signed-off-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>

This looks good.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 4/5] spi: dw: document device tree binding

[Baruch Siach]

Hi Mark,

On Mon, Jan 27, 2014 at 07:08:39PM +0000, Mark Brown wrote:
> On Sun, Jan 26, 2014 at 10:14:35AM +0200, Baruch Siach wrote:
> 
> > +Required properties:
> 
> > +- num-cs: see spi-bus.txt
> 
> Why is this mandatory, in the code it was optional?  One or the other
> ought to be updated.

The code defaults to 4. I guess that's what the hardware this code was tested 
on had. But this default doesn't make much sense in the general case. Anyway, 
for backwards compatibility I'll document this property as optional, note that 
the default is 4, and recommend to add this property anyway. Does this sound 
reasonable?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
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 v2 4/5] spi: dw: document device tree binding

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 795 bytes --]

On Mon, Jan 27, 2014 at 09:18:10PM +0200, Baruch Siach wrote:
> On Mon, Jan 27, 2014 at 07:08:39PM +0000, Mark Brown wrote:
> > On Sun, Jan 26, 2014 at 10:14:35AM +0200, Baruch Siach wrote:

> > > +Required properties:

> > > +- num-cs: see spi-bus.txt

> > Why is this mandatory, in the code it was optional?  One or the other
> > ought to be updated.

> The code defaults to 4. I guess that's what the hardware this code was tested 
> on had. But this default doesn't make much sense in the general case. Anyway, 
> for backwards compatibility I'll document this property as optional, note that 
> the default is 4, and recommend to add this property anyway. Does this sound 
> reasonable?

This is a new binding, why would it have to be backwards compatible with
anything?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 4/5] spi: dw: document device tree binding

[Baruch Siach]

Hi Mark,

On Mon, Jan 27, 2014 at 07:22:51PM +0000, Mark Brown wrote:
> On Mon, Jan 27, 2014 at 09:18:10PM +0200, Baruch Siach wrote:
> > On Mon, Jan 27, 2014 at 07:08:39PM +0000, Mark Brown wrote:
> > > On Sun, Jan 26, 2014 at 10:14:35AM +0200, Baruch Siach wrote:
> 
> > > > +Required properties:
> 
> > > > +- num-cs: see spi-bus.txt
> 
> > > Why is this mandatory, in the code it was optional?  One or the other
> > > ought to be updated.
> 
> > The code defaults to 4. I guess that's what the hardware this code was tested 
> > on had. But this default doesn't make much sense in the general case. Anyway, 
> > for backwards compatibility I'll document this property as optional, note that 
> > the default is 4, and recommend to add this property anyway. Does this sound 
> > reasonable?
> 
> This is a new binding, why would it have to be backwards compatible with
> anything?

I meant driver compatibility. I used to work that way, and should continue 
doing so in non-DT environment. I suggest to make num-cs mandatory for DT, but 
keep the existing behaviour for non-DT. I'll update the code then.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
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 v2 3/5] spi: dw: add support for gpio controlled chip select

[Baruch Siach]

Hi Mark,

On Mon, Jan 27, 2014 at 06:57:54PM +0000, Mark Brown wrote:
> > +	if (gpio_is_valid(spi->cs_gpio)) {
> > +		ret = devm_gpio_request(&spi->dev, spi->cs_gpio,
> > +				dev_name(&spi->dev));
> > +		if (ret)
> > +			return ret;
> > +		ret = gpio_direction_output(spi->cs_gpio,
> > +				!(spi->mode & SPI_CS_HIGH));
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> Should be devm_gpio_request_one().  This won't work with probe deferral,
> it's outside the probe() function so nothing will retry.

So how about doing devm_gpio_request_one() in probe() right after 
devm_spi_register_master()? This should be compatible with deferral, and yet 
avoid open coding the "cs-gpios" property parsing as the pl022 driver does.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
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 v2 3/5] spi: dw: add support for gpio controlled chip select

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 556 bytes --]

On Tue, Jan 28, 2014 at 02:35:06PM +0200, Baruch Siach wrote:

> So how about doing devm_gpio_request_one() in probe() right after 
> devm_spi_register_master()? This should be compatible with deferral, and yet 
> avoid open coding the "cs-gpios" property parsing as the pl022 driver does.

It needs to before otherwise we'll try to instantiate the slaves without
their chip select which isn't going to be a great idea.  The ideal thing
would be for this to be handled in the core rather than in the
individual drivers, the property is standard.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 3/5] spi: dw: add support for gpio controlled chip select

[Baruch Siach]

Hi Mark,

On Tue, Jan 28, 2014 at 03:42:12PM +0000, Mark Brown wrote:
> On Tue, Jan 28, 2014 at 02:35:06PM +0200, Baruch Siach wrote:
> > So how about doing devm_gpio_request_one() in probe() right after 
> > devm_spi_register_master()? This should be compatible with deferral, and 
> > yet avoid open coding the "cs-gpios" property parsing as the pl022 driver 
> > does.
> 
> It needs to before otherwise we'll try to instantiate the slaves without
> their chip select which isn't going to be a great idea.  The ideal thing
> would be for this to be handled in the core rather than in the
> individual drivers, the property is standard.

So until the core code gains this ability to request gpios what is the best 
option? Do as pl022 is doing? Leave it at .setup? Something else?

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
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 v2 3/5] spi: dw: add support for gpio controlled chip select

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

On Tue, Jan 28, 2014 at 08:11:16PM +0200, Baruch Siach wrote:
> On Tue, Jan 28, 2014 at 03:42:12PM +0000, Mark Brown wrote:

> > It needs to before otherwise we'll try to instantiate the slaves without
> > their chip select which isn't going to be a great idea.  The ideal thing
> > would be for this to be handled in the core rather than in the
> > individual drivers, the property is standard.

> So until the core code gains this ability to request gpios what is the best 
> option? Do as pl022 is doing? Leave it at .setup? Something else?

There's always the option of enhancing the core yourself! :P  But yeah,
copy pl022.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]