[PATCH RESEND v4] spi/pl022: add devicetree support

[PATCH RESEND v4] spi/pl022: add devicetree support

[Alexandre Pereira da Silva]

Add the chipselect array and cur_cs properties to pl022 main structure

Add a wrapper function to decide if the cs should be controlled by the
cs_control callback or the chipselect gpio

Populate chipselect property from cs-gpios

Populate master->dev.of_node, so the spi bus can find child spi
devices and register them

At pl022 setup, fill chip_data structure from dt nodes, if not provided
by platform.

Signed-off-by: Alexandre Pereira da Silva <aletes.xgr-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org>
---
Applies to v3.6-rc1

Changes since v3:
* Proper use of IS_ENABLED

Changes since v2:
* Use IS_ENABLED instead of #ifdef
* Remove bogus const change

Changes since v1:
* return EPROBE_DEFFER if gpios are not initialized yet

Thanks Thierry Reding for reviewing and improvements suggestions

 .../devicetree/bindings/spi/spi_pl022.txt          |   15 +++
 drivers/spi/spi-pl022.c                            |  114 ++++++++++++++++----
 2 files changed, 110 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi_pl022.txt b/Documentation/devicetree/bindings/spi/spi_pl022.txt
index 306ec3f..b089ec7 100644
--- a/Documentation/devicetree/bindings/spi/spi_pl022.txt
+++ b/Documentation/devicetree/bindings/spi/spi_pl022.txt
@@ -6,7 +6,22 @@ Required properties:
 - interrupts : Should contain SPI controller interrupt
 
 Optional properties:
+- pl022,num-chipselects : total number of chipselects
 - cs-gpios : should specify GPIOs used for chipselects.
   The gpios will be referred to as reg = <index> in the SPI child nodes.
   If unspecified, a single SPI device without a chip select can be used.
 
+SPI slave nodes must be children of the SPI master node and can
+contain the following properties.
+See include/linux/amba/pl022.h for more details
+
+- pl022,hierarchy : master or slave interface
+- pl022,interface : interface type
+- pl022,slave-tx-disable : disconnect tx line in slave mode
+- pl022,com-mode : polling, interrupt or dma
+- pl022,rx-level-trig : Rx FIFO watermark level
+- pl022,tx-level-trig : Tx FIFO watermark level
+- pl022,ctrl-len : Microwire interface: Control length
+- pl022,wait-state : Microwire interface: Wait state
+- pl022,duplex : Microwire interface: Full/Half duplex
+
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
index 400ae21..d8d7d21 100644
--- a/drivers/spi/spi-pl022.c
+++ b/drivers/spi/spi-pl022.c
@@ -40,6 +40,8 @@
 #include <linux/dma-mapping.h>
 #include <linux/scatterlist.h>
 #include <linux/pm_runtime.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
 
 /*
  * This macro is used to define some register default values.
@@ -389,6 +391,8 @@ struct pl022 {
 	char				*dummypage;
 	bool				dma_running;
 #endif
+	int cur_cs;
+	int chipselect[0];
 };
 
 /**
@@ -433,6 +437,14 @@ static void null_cs_control(u32 command)
 	pr_debug("pl022: dummy chip select control, CS=0x%x\n", command);
 }
 
+static void pl022_cs_control(struct pl022 *pl022, u32 command)
+{
+	if (gpio_is_valid(pl022->cur_cs))
+		gpio_set_value(pl022->cur_cs, command);
+	else
+		pl022->cur_chip->cs_control(command);
+}
+
 /**
  * giveback - current spi_message is over, schedule next message and call
  * callback of this message. Assumes that caller already
@@ -479,7 +491,7 @@ static void giveback(struct pl022 *pl022)
 		if (next_msg && next_msg->spi != pl022->cur_msg->spi)
 			next_msg = NULL;
 		if (!next_msg || pl022->cur_msg->state == STATE_ERROR)
-			pl022->cur_chip->cs_control(SSP_CHIP_DESELECT);
+			pl022_cs_control(pl022, SSP_CHIP_DESELECT);
 		else
 			pl022->next_msg_cs_active = true;
 
@@ -813,8 +825,7 @@ static void dma_callback(void *data)
 	/* Update total bytes transferred */
 	msg->actual_length += pl022->cur_transfer->len;
 	if (pl022->cur_transfer->cs_change)
-		pl022->cur_chip->
-			cs_control(SSP_CHIP_DESELECT);
+		pl022_cs_control(pl022, SSP_CHIP_DESELECT);
 
 	/* Move to next transfer */
 	msg->state = next_transfer(pl022);
@@ -1247,8 +1258,7 @@ static irqreturn_t pl022_interrupt_handler(int irq, void *dev_id)
 		/* Update total bytes transferred */
 		msg->actual_length += pl022->cur_transfer->len;
 		if (pl022->cur_transfer->cs_change)
-			pl022->cur_chip->
-				cs_control(SSP_CHIP_DESELECT);
+			pl022_cs_control(pl022, SSP_CHIP_DESELECT);
 		/* Move to next transfer */
 		msg->state = next_transfer(pl022);
 		tasklet_schedule(&pl022->pump_transfers);
@@ -1333,7 +1343,7 @@ static void pump_transfers(unsigned long data)
 
 		/* Reselect chip select only if cs_change was requested */
 		if (previous->cs_change)
-			pl022->cur_chip->cs_control(SSP_CHIP_SELECT);
+			pl022_cs_control(pl022, SSP_CHIP_SELECT);
 	} else {
 		/* STATE_START */
 		message->state = STATE_RUNNING;
@@ -1372,7 +1382,7 @@ static void do_interrupt_dma_transfer(struct pl022 *pl022)
 
 	/* Enable target chip, if not already active */
 	if (!pl022->next_msg_cs_active)
-		pl022->cur_chip->cs_control(SSP_CHIP_SELECT);
+		pl022_cs_control(pl022, SSP_CHIP_SELECT);
 
 	if (set_up_next_transfer(pl022, pl022->cur_transfer)) {
 		/* Error path */
@@ -1424,12 +1434,12 @@ static void do_polling_transfer(struct pl022 *pl022)
 			if (previous->delay_usecs)
 				udelay(previous->delay_usecs);
 			if (previous->cs_change)
-				pl022->cur_chip->cs_control(SSP_CHIP_SELECT);
+				pl022_cs_control(pl022, SSP_CHIP_SELECT);
 		} else {
 			/* STATE_START */
 			message->state = STATE_RUNNING;
 			if (!pl022->next_msg_cs_active)
-				pl022->cur_chip->cs_control(SSP_CHIP_SELECT);
+				pl022_cs_control(pl022, SSP_CHIP_SELECT);
 		}
 
 		/* Configuration Changing Per Transfer */
@@ -1461,7 +1471,7 @@ static void do_polling_transfer(struct pl022 *pl022)
 		/* Update total byte transferred */
 		message->actual_length += pl022->cur_transfer->len;
 		if (pl022->cur_transfer->cs_change)
-			pl022->cur_chip->cs_control(SSP_CHIP_DESELECT);
+			pl022_cs_control(pl022, SSP_CHIP_DESELECT);
 		/* Move to next transfer */
 		message->state = next_transfer(pl022);
 	}
@@ -1490,6 +1500,7 @@ static int pl022_transfer_one_message(struct spi_master *master,
 
 	/* Setup the SPI using the per chip configuration */
 	pl022->cur_chip = spi_get_ctldata(msg->spi);
+	pl022->cur_cs = pl022->chipselect[msg->spi->chip_select];
 
 	restore_state(pl022);
 	flush(pl022);
@@ -1761,12 +1772,14 @@ static const struct pl022_config_chip pl022_default_chip_info = {
 static int pl022_setup(struct spi_device *spi)
 {
 	struct pl022_config_chip const *chip_info;
+	struct pl022_config_chip chip_info_dt;
 	struct chip_data *chip;
 	struct ssp_clock_params clk_freq = { .cpsdvsr = 0, .scr = 0};
 	int status = 0;
 	struct pl022 *pl022 = spi_master_get_devdata(spi->master);
 	unsigned int bits = spi->bits_per_word;
 	u32 tmp;
+	struct device_node *np = spi->dev.of_node;
 
 	if (!spi->max_speed_hz)
 		return -EINVAL;
@@ -1789,10 +1802,36 @@ static int pl022_setup(struct spi_device *spi)
 	chip_info = spi->controller_data;
 
 	if (chip_info == NULL) {
-		chip_info = &pl022_default_chip_info;
-		/* spi_board_info.controller_data not is supplied */
-		dev_dbg(&spi->dev,
-			"using default controller_data settings\n");
+		if (np) {
+			chip_info_dt = pl022_default_chip_info;
+
+			of_property_read_u32(np, "pl022,hierarchy",
+				&chip_info_dt.hierarchy);
+			of_property_read_u32(np, "pl022,interface",
+				&chip_info_dt.iface);
+			chip_info_dt.slave_tx_disable =
+				of_property_read_bool(np,
+					"pl022,slave-tx-disable");
+			of_property_read_u32(np, "pl022,com-mode",
+				&chip_info_dt.com_mode);
+			of_property_read_u32(np, "pl022,rx-level-trig",
+				&chip_info_dt.rx_lev_trig);
+			of_property_read_u32(np, "pl022,tx-level-trig",
+				&chip_info_dt.tx_lev_trig);
+			of_property_read_u32(np, "pl022,ctrl-len",
+				&chip_info_dt.ctrl_len);
+			of_property_read_u32(np, "pl022,wait-state",
+				&chip_info_dt.wait_state);
+			of_property_read_u32(np, "pl022,duplex",
+				&chip_info_dt.duplex);
+
+			chip_info = &chip_info_dt;
+		} else {
+			chip_info = &pl022_default_chip_info;
+			/* spi_board_info.controller_data not is supplied */
+			dev_dbg(&spi->dev,
+				"using default controller_data settings\n");
+		}
 	} else
 		dev_dbg(&spi->dev,
 			"using user supplied controller_data settings\n");
@@ -1835,8 +1874,9 @@ static int pl022_setup(struct spi_device *spi)
 	chip->xfer_type = chip_info->com_mode;
 	if (!chip_info->cs_control) {
 		chip->cs_control = null_cs_control;
-		dev_warn(&spi->dev,
-			 "chip select function is NULL for this chip\n");
+		if (!gpio_is_valid(pl022->chipselect[spi->chip_select]))
+			dev_warn(&spi->dev,
+				 "chip select function is NULL for this chip\n");
 	} else
 		chip->cs_control = chip_info->cs_control;
 
@@ -1988,7 +2028,8 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	struct pl022_ssp_controller *platform_info = adev->dev.platform_data;
 	struct spi_master *master;
 	struct pl022 *pl022 = NULL;	/*Data for this driver */
-	int status = 0;
+	struct device_node *np = adev->dev.of_node;
+	int status = 0, i, num_cs;
 
 	dev_info(&adev->dev,
 		 "ARM PL022 driver, device ID: 0x%08x\n", adev->periphid);
@@ -1998,8 +2039,14 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 		goto err_no_pdata;
 	}
 
+	num_cs = platform_info->num_chipselect;
+	if (IS_ENABLED(CONFIG_OF))
+		of_property_read_u32(np, "pl022,num-chipselects", &num_cs);
+
+
 	/* Allocate master with space for data */
-	master = spi_alloc_master(dev, sizeof(struct pl022));
+	master = spi_alloc_master(dev,
+		sizeof(struct pl022) + sizeof(int) * num_cs);
 	if (master == NULL) {
 		dev_err(&adev->dev, "probe - cannot alloc SPI master\n");
 		status = -ENOMEM;
@@ -2017,13 +2064,41 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
 	 * on this board
 	 */
 	master->bus_num = platform_info->bus_id;
-	master->num_chipselect = platform_info->num_chipselect;
+	master->num_chipselect = num_cs;
 	master->cleanup = pl022_cleanup;
 	master->setup = pl022_setup;
 	master->prepare_transfer_hardware = pl022_prepare_transfer_hardware;
 	master->transfer_one_message = pl022_transfer_one_message;
 	master->unprepare_transfer_hardware = pl022_unprepare_transfer_hardware;
 	master->rt = platform_info->rt;
+	master->dev.of_node = dev->of_node;
+
+	if (IS_ENABLED(CONFIG_OF)) {
+		for (i = 0; i < num_cs; i++) {
+			int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
+
+			if (cs_gpio == -EPROBE_DEFER) {
+				status = -EPROBE_DEFER;
+				goto err_no_gpio;
+			}
+
+			pl022->chipselect[i] = cs_gpio;
+
+			if (gpio_is_valid(cs_gpio)) {
+				if (gpio_request(cs_gpio, "ssp-pl022"))
+					dev_err(&adev->dev,
+						"could not request %d gpio\n",
+						cs_gpio);
+				else if (gpio_direction_output(cs_gpio, 1))
+					dev_err(&adev->dev,
+						"could set gpio %d as output\n",
+						cs_gpio);
+			}
+		}
+	} else {
+		for (i = 0; i < num_cs; i++)
+			pl022->chipselect[i] = -EINVAL;
+	}
 
 	/*
 	 * Supports mode 0-3, loopback, and active low CS. Transfers are
@@ -2130,6 +2205,7 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
  err_no_ioremap:
 	amba_release_regions(adev);
  err_no_ioregion:
+ err_no_gpio:
 	spi_master_put(master);
  err_no_master:
  err_no_pdata:
-- 
1.7.10


------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [PATCH RESEND v4] spi/pl022: add devicetree support

[Roland Stigge]

Hi all,

On 08/20/2012 01:39 PM, Alexandre Pereira da Silva wrote:
> Add the chipselect array and cur_cs properties to pl022 main structure
>
> Add a wrapper function to decide if the cs should be controlled by the
> cs_control callback or the chipselect gpio
>
> Populate chipselect property from cs-gpios
>
> Populate master->dev.of_node, so the spi bus can find child spi
> devices and register them
>
> At pl022 setup, fill chip_data structure from dt nodes, if not provided
> by platform.
>
> Signed-off-by: Alexandre Pereira da Silva <aletes.xgr@gmail.com>
> Acked-by: Roland Stigge <stigge@antcom.de>

We somehow missed 3.6 with this one. Would be nice to have it in 3.7 now
since it's useful for mach-lpc32xx when it's available.

If there are any open issues, I can assist Alexandre on this patch.

Thanks in advance,

Roland

Re: [PATCH RESEND v4] spi/pl022: add devicetree support

[Rob Herring]

On 08/20/2012 07:58 AM, Roland Stigge wrote:
> Hi all,
> 
> On 08/20/2012 01:39 PM, Alexandre Pereira da Silva wrote:
>> Add the chipselect array and cur_cs properties to pl022 main structure
>>
>> Add a wrapper function to decide if the cs should be controlled by the
>> cs_control callback or the chipselect gpio
>>
>> Populate chipselect property from cs-gpios
>>
>> Populate master->dev.of_node, so the spi bus can find child spi
>> devices and register them
>>
>> At pl022 setup, fill chip_data structure from dt nodes, if not provided
>> by platform.
>>
>> Signed-off-by: Alexandre Pereira da Silva <aletes.xgr-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Acked-by: Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org>
> 
> We somehow missed 3.6 with this one. Would be nice to have it in 3.7 now
> since it's useful for mach-lpc32xx when it's available.

Mark Brown is handling SPI right now for Grant, so please send it to
him. I have some comments on the patch though.

Rob

> 
> If there are any open issues, I can assist Alexandre on this patch.
> 
> Thanks in advance,
> 
> Roland
> 

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and 
threat landscape has changed and how IT managers can respond. Discussions 
will include endpoint security, mobile security and the latest in malware 
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

Re: [PATCH RESEND v4] spi/pl022: add devicetree support

[Rob Herring]

On 08/20/2012 06:39 AM, Alexandre Pereira da Silva wrote:
> Add the chipselect array and cur_cs properties to pl022 main structure
> 
> Add a wrapper function to decide if the cs should be controlled by the
> cs_control callback or the chipselect gpio
> 
> Populate chipselect property from cs-gpios
> 
> Populate master->dev.of_node, so the spi bus can find child spi
> devices and register them
> 
> At pl022 setup, fill chip_data structure from dt nodes, if not provided
> by platform.
> 
> Signed-off-by: Alexandre Pereira da Silva <aletes.xgr@gmail.com>
> Acked-by: Roland Stigge <stigge@antcom.de>
> ---
> Applies to v3.6-rc1
> 
> Changes since v3:
> * Proper use of IS_ENABLED
> 
> Changes since v2:
> * Use IS_ENABLED instead of #ifdef
> * Remove bogus const change
> 
> Changes since v1:
> * return EPROBE_DEFFER if gpios are not initialized yet
> 
> Thanks Thierry Reding for reviewing and improvements suggestions
> 
>  .../devicetree/bindings/spi/spi_pl022.txt          |   15 +++
>  drivers/spi/spi-pl022.c                            |  114 ++++++++++++++++----
>  2 files changed, 110 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi_pl022.txt b/Documentation/devicetree/bindings/spi/spi_pl022.txt
> index 306ec3f..b089ec7 100644
> --- a/Documentation/devicetree/bindings/spi/spi_pl022.txt
> +++ b/Documentation/devicetree/bindings/spi/spi_pl022.txt
> @@ -6,7 +6,22 @@ Required properties:
>  - interrupts : Should contain SPI controller interrupt
>  
>  Optional properties:
> +- pl022,num-chipselects : total number of chipselects

We have this for other spi controllers too. Define a generic property.

>  - cs-gpios : should specify GPIOs used for chipselects.
>    The gpios will be referred to as reg = <index> in the SPI child nodes.
>    If unspecified, a single SPI device without a chip select can be used.
>  
> +SPI slave nodes must be children of the SPI master node and can
> +contain the following properties.
> +See include/linux/amba/pl022.h for more details

DT binding is independent of Linux, so you should not refer to Linux docs.

> +
> +- pl022,hierarchy : master or slave interface

Is attaching a master and using the pl022 as a slave really a supported
use case? The DT spi bindings are really designed for a master node with
N slave nodes. So there is more general question of how to describe a
spi controller as a slave. It's not really one I care to answer as the
Linux spi framework isn't designed to act as a slave.

> +- pl022,interface : interface type

And valid values are?

> +- pl022,slave-tx-disable : disconnect tx line in slave mode
> +- pl022,com-mode : polling, interrupt or dma
> +- pl022,rx-level-trig : Rx FIFO watermark level
> +- pl022,tx-level-trig : Tx FIFO watermark level
> +- pl022,ctrl-len : Microwire interface: Control length
> +- pl022,wait-state : Microwire interface: Wait state
> +- pl022,duplex : Microwire interface: Full/Half duplex

Most of these properties aren't used anywhere in the kernel other than
u300 and I'm not sure what to purpose of dummy_chip_info is. Perhaps
Linus has some input? I think either they can be decided by the spi
controller (com-mode, fifo watermark) or should be standard properties
(microwire settings). I would argue they should be removed if the spi
framework doesn't have support in a standard way.

> +
> diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c
> index 400ae21..d8d7d21 100644
> --- a/drivers/spi/spi-pl022.c
> +++ b/drivers/spi/spi-pl022.c
> @@ -40,6 +40,8 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/scatterlist.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
>  
>  /*
>   * This macro is used to define some register default values.
> @@ -389,6 +391,8 @@ struct pl022 {
>  	char				*dummypage;
>  	bool				dma_running;
>  #endif
> +	int cur_cs;
> +	int chipselect[0];
>  };
>  
>  /**
> @@ -433,6 +437,14 @@ static void null_cs_control(u32 command)
>  	pr_debug("pl022: dummy chip select control, CS=0x%x\n", command);
>  }
>  
> +static void pl022_cs_control(struct pl022 *pl022, u32 command)
> +{
> +	if (gpio_is_valid(pl022->cur_cs))
> +		gpio_set_value(pl022->cur_cs, command);
> +	else
> +		pl022->cur_chip->cs_control(command);
> +}
> +
>  /**
>   * giveback - current spi_message is over, schedule next message and call
>   * callback of this message. Assumes that caller already
> @@ -479,7 +491,7 @@ static void giveback(struct pl022 *pl022)
>  		if (next_msg && next_msg->spi != pl022->cur_msg->spi)
>  			next_msg = NULL;
>  		if (!next_msg || pl022->cur_msg->state == STATE_ERROR)
> -			pl022->cur_chip->cs_control(SSP_CHIP_DESELECT);
> +			pl022_cs_control(pl022, SSP_CHIP_DESELECT);
>  		else
>  			pl022->next_msg_cs_active = true;
>  
> @@ -813,8 +825,7 @@ static void dma_callback(void *data)
>  	/* Update total bytes transferred */
>  	msg->actual_length += pl022->cur_transfer->len;
>  	if (pl022->cur_transfer->cs_change)
> -		pl022->cur_chip->
> -			cs_control(SSP_CHIP_DESELECT);
> +		pl022_cs_control(pl022, SSP_CHIP_DESELECT);
>  
>  	/* Move to next transfer */
>  	msg->state = next_transfer(pl022);
> @@ -1247,8 +1258,7 @@ static irqreturn_t pl022_interrupt_handler(int irq, void *dev_id)
>  		/* Update total bytes transferred */
>  		msg->actual_length += pl022->cur_transfer->len;
>  		if (pl022->cur_transfer->cs_change)
> -			pl022->cur_chip->
> -				cs_control(SSP_CHIP_DESELECT);
> +			pl022_cs_control(pl022, SSP_CHIP_DESELECT);
>  		/* Move to next transfer */
>  		msg->state = next_transfer(pl022);
>  		tasklet_schedule(&pl022->pump_transfers);
> @@ -1333,7 +1343,7 @@ static void pump_transfers(unsigned long data)
>  
>  		/* Reselect chip select only if cs_change was requested */
>  		if (previous->cs_change)
> -			pl022->cur_chip->cs_control(SSP_CHIP_SELECT);
> +			pl022_cs_control(pl022, SSP_CHIP_SELECT);
>  	} else {
>  		/* STATE_START */
>  		message->state = STATE_RUNNING;
> @@ -1372,7 +1382,7 @@ static void do_interrupt_dma_transfer(struct pl022 *pl022)
>  
>  	/* Enable target chip, if not already active */
>  	if (!pl022->next_msg_cs_active)
> -		pl022->cur_chip->cs_control(SSP_CHIP_SELECT);
> +		pl022_cs_control(pl022, SSP_CHIP_SELECT);
>  
>  	if (set_up_next_transfer(pl022, pl022->cur_transfer)) {
>  		/* Error path */
> @@ -1424,12 +1434,12 @@ static void do_polling_transfer(struct pl022 *pl022)
>  			if (previous->delay_usecs)
>  				udelay(previous->delay_usecs);
>  			if (previous->cs_change)
> -				pl022->cur_chip->cs_control(SSP_CHIP_SELECT);
> +				pl022_cs_control(pl022, SSP_CHIP_SELECT);
>  		} else {
>  			/* STATE_START */
>  			message->state = STATE_RUNNING;
>  			if (!pl022->next_msg_cs_active)
> -				pl022->cur_chip->cs_control(SSP_CHIP_SELECT);
> +				pl022_cs_control(pl022, SSP_CHIP_SELECT);
>  		}
>  
>  		/* Configuration Changing Per Transfer */
> @@ -1461,7 +1471,7 @@ static void do_polling_transfer(struct pl022 *pl022)
>  		/* Update total byte transferred */
>  		message->actual_length += pl022->cur_transfer->len;
>  		if (pl022->cur_transfer->cs_change)
> -			pl022->cur_chip->cs_control(SSP_CHIP_DESELECT);
> +			pl022_cs_control(pl022, SSP_CHIP_DESELECT);
>  		/* Move to next transfer */
>  		message->state = next_transfer(pl022);
>  	}
> @@ -1490,6 +1500,7 @@ static int pl022_transfer_one_message(struct spi_master *master,
>  
>  	/* Setup the SPI using the per chip configuration */
>  	pl022->cur_chip = spi_get_ctldata(msg->spi);
> +	pl022->cur_cs = pl022->chipselect[msg->spi->chip_select];
>  
>  	restore_state(pl022);
>  	flush(pl022);
> @@ -1761,12 +1772,14 @@ static const struct pl022_config_chip pl022_default_chip_info = {
>  static int pl022_setup(struct spi_device *spi)
>  {
>  	struct pl022_config_chip const *chip_info;
> +	struct pl022_config_chip chip_info_dt;
>  	struct chip_data *chip;
>  	struct ssp_clock_params clk_freq = { .cpsdvsr = 0, .scr = 0};
>  	int status = 0;
>  	struct pl022 *pl022 = spi_master_get_devdata(spi->master);
>  	unsigned int bits = spi->bits_per_word;
>  	u32 tmp;
> +	struct device_node *np = spi->dev.of_node;
>  
>  	if (!spi->max_speed_hz)
>  		return -EINVAL;
> @@ -1789,10 +1802,36 @@ static int pl022_setup(struct spi_device *spi)
>  	chip_info = spi->controller_data;
>  
>  	if (chip_info == NULL) {
> -		chip_info = &pl022_default_chip_info;
> -		/* spi_board_info.controller_data not is supplied */
> -		dev_dbg(&spi->dev,
> -			"using default controller_data settings\n");
> +		if (np) {
> +			chip_info_dt = pl022_default_chip_info;
> +
> +			of_property_read_u32(np, "pl022,hierarchy",
> +				&chip_info_dt.hierarchy);
> +			of_property_read_u32(np, "pl022,interface",
> +				&chip_info_dt.iface);
> +			chip_info_dt.slave_tx_disable =
> +				of_property_read_bool(np,
> +					"pl022,slave-tx-disable");
> +			of_property_read_u32(np, "pl022,com-mode",
> +				&chip_info_dt.com_mode);
> +			of_property_read_u32(np, "pl022,rx-level-trig",
> +				&chip_info_dt.rx_lev_trig);
> +			of_property_read_u32(np, "pl022,tx-level-trig",
> +				&chip_info_dt.tx_lev_trig);
> +			of_property_read_u32(np, "pl022,ctrl-len",
> +				&chip_info_dt.ctrl_len);
> +			of_property_read_u32(np, "pl022,wait-state",
> +				&chip_info_dt.wait_state);
> +			of_property_read_u32(np, "pl022,duplex",
> +				&chip_info_dt.duplex);
> +
> +			chip_info = &chip_info_dt;
> +		} else {
> +			chip_info = &pl022_default_chip_info;
> +			/* spi_board_info.controller_data not is supplied */
> +			dev_dbg(&spi->dev,
> +				"using default controller_data settings\n");
> +		}
>  	} else
>  		dev_dbg(&spi->dev,
>  			"using user supplied controller_data settings\n");
> @@ -1835,8 +1874,9 @@ static int pl022_setup(struct spi_device *spi)
>  	chip->xfer_type = chip_info->com_mode;
>  	if (!chip_info->cs_control) {
>  		chip->cs_control = null_cs_control;
> -		dev_warn(&spi->dev,
> -			 "chip select function is NULL for this chip\n");
> +		if (!gpio_is_valid(pl022->chipselect[spi->chip_select]))
> +			dev_warn(&spi->dev,
> +				 "chip select function is NULL for this chip\n");
>  	} else
>  		chip->cs_control = chip_info->cs_control;
>  
> @@ -1988,7 +2028,8 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  	struct pl022_ssp_controller *platform_info = adev->dev.platform_data;
>  	struct spi_master *master;
>  	struct pl022 *pl022 = NULL;	/*Data for this driver */
> -	int status = 0;
> +	struct device_node *np = adev->dev.of_node;
> +	int status = 0, i, num_cs;
>  
>  	dev_info(&adev->dev,
>  		 "ARM PL022 driver, device ID: 0x%08x\n", adev->periphid);
> @@ -1998,8 +2039,14 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  		goto err_no_pdata;
>  	}
>  
> +	num_cs = platform_info->num_chipselect;
> +	if (IS_ENABLED(CONFIG_OF))
> +		of_property_read_u32(np, "pl022,num-chipselects", &num_cs);
> +
> +
>  	/* Allocate master with space for data */
> -	master = spi_alloc_master(dev, sizeof(struct pl022));
> +	master = spi_alloc_master(dev,
> +		sizeof(struct pl022) + sizeof(int) * num_cs);
>  	if (master == NULL) {
>  		dev_err(&adev->dev, "probe - cannot alloc SPI master\n");
>  		status = -ENOMEM;
> @@ -2017,13 +2064,41 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>  	 * on this board
>  	 */
>  	master->bus_num = platform_info->bus_id;
> -	master->num_chipselect = platform_info->num_chipselect;
> +	master->num_chipselect = num_cs;
>  	master->cleanup = pl022_cleanup;
>  	master->setup = pl022_setup;
>  	master->prepare_transfer_hardware = pl022_prepare_transfer_hardware;
>  	master->transfer_one_message = pl022_transfer_one_message;
>  	master->unprepare_transfer_hardware = pl022_unprepare_transfer_hardware;
>  	master->rt = platform_info->rt;
> +	master->dev.of_node = dev->of_node;
> +
> +	if (IS_ENABLED(CONFIG_OF)) {
> +		for (i = 0; i < num_cs; i++) {
> +			int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
> +
> +			if (cs_gpio == -EPROBE_DEFER) {
> +				status = -EPROBE_DEFER;
> +				goto err_no_gpio;
> +			}
> +
> +			pl022->chipselect[i] = cs_gpio;
> +
> +			if (gpio_is_valid(cs_gpio)) {
> +				if (gpio_request(cs_gpio, "ssp-pl022"))
> +					dev_err(&adev->dev,
> +						"could not request %d gpio\n",
> +						cs_gpio);
> +				else if (gpio_direction_output(cs_gpio, 1))
> +					dev_err(&adev->dev,
> +						"could set gpio %d as output\n",
> +						cs_gpio);
> +			}
> +		}
> +	} else {
> +		for (i = 0; i < num_cs; i++)
> +			pl022->chipselect[i] = -EINVAL;
> +	}
>  
>  	/*
>  	 * Supports mode 0-3, loopback, and active low CS. Transfers are
> @@ -2130,6 +2205,7 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>   err_no_ioremap:
>  	amba_release_regions(adev);
>   err_no_ioregion:
> + err_no_gpio:
>  	spi_master_put(master);
>   err_no_master:
>   err_no_pdata:
> 

Re: [PATCH RESEND v4] spi/pl022: add devicetree support

[Linus Walleij]

On Mon, Aug 20, 2012 at 4:22 PM, Rob Herring <robherring2@gmail.com> wrote:
> On 08/20/2012 06:39 AM, Alexandre Pereira da Silva wrote:

>> +- pl022,hierarchy : master or slave interface
>
> Is attaching a master and using the pl022 as a slave really a supported
> use case?

No not currently. People doing this crazy stuff need to have
the kernel with the hard real-time patches and lightening response
first. But it's in the platform data because it's a feature supported by
the hardware.

> The DT spi bindings are really designed for a master node with
> N slave nodes. So there is more general question of how to describe a
> spi controller as a slave. It's not really one I care to answer as the
> Linux spi framework isn't designed to act as a slave.

Currently the code should hardwire this to master and not define
a binding for it I think.

>> +- pl022,slave-tx-disable : disconnect tx line in slave mode

Applies also to this, then.

>> +- pl022,com-mode : polling, interrupt or dma
>> +- pl022,rx-level-trig : Rx FIFO watermark level
>> +- pl022,tx-level-trig : Tx FIFO watermark level
>> +- pl022,ctrl-len : Microwire interface: Control length
>> +- pl022,wait-state : Microwire interface: Wait state
>> +- pl022,duplex : Microwire interface: Full/Half duplex
>
> Most of these properties aren't used anywhere in the kernel other than
> u300 and I'm not sure what to purpose of dummy_chip_info is.

It is used for loopback-testing of the PL022. They're probably
not necessary there either.

> Perhaps
> Linus has some input? I think either they can be decided by the spi
> controller (com-mode, fifo watermark) or should be standard properties
> (microwire settings). I would argue they should be removed if the spi
> framework doesn't have support in a standard way.

Uncertain about this, others would need to comment.
At some point there has been a chip needing these to be set
to some magic values.

Yours,
Linus Walleij

Re: [PATCH RESEND v4] spi/pl022: add devicetree support

[Roland Stigge]

On 08/20/2012 04:22 PM, Rob Herring wrote:
>>  .../devicetree/bindings/spi/spi_pl022.txt          |   15 +++
>>  drivers/spi/spi-pl022.c                            |  114 ++++++++++++++++----
>>  2 files changed, 110 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi_pl022.txt b/Documentation/devicetree/bindings/spi/spi_pl022.txt
>> index 306ec3f..b089ec7 100644
>> --- a/Documentation/devicetree/bindings/spi/spi_pl022.txt
>> +++ b/Documentation/devicetree/bindings/spi/spi_pl022.txt
>> @@ -6,7 +6,22 @@ Required properties:
>>  - interrupts : Should contain SPI controller interrupt
>>  
>>  Optional properties:
>> +- pl022,num-chipselects : total number of chipselects
> 
> We have this for other spi controllers too. Define a generic property.

Currently, we have:

num-cs
ti,spi-num-cs
fsl,spi-num-chipselects
fsl,espi-num-chipselects
(pl022,num-chipselects)

Would "num-cs" be the right one to use?

Thanks,

Roland

Re: [PATCH RESEND v4] spi/pl022: add devicetree support

[Rob Herring]

On 08/20/2012 10:33 AM, Roland Stigge wrote:
> On 08/20/2012 04:22 PM, Rob Herring wrote:
>>>  .../devicetree/bindings/spi/spi_pl022.txt          |   15 +++
>>>  drivers/spi/spi-pl022.c                            |  114 ++++++++++++++++----
>>>  2 files changed, 110 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/spi/spi_pl022.txt b/Documentation/devicetree/bindings/spi/spi_pl022.txt
>>> index 306ec3f..b089ec7 100644
>>> --- a/Documentation/devicetree/bindings/spi/spi_pl022.txt
>>> +++ b/Documentation/devicetree/bindings/spi/spi_pl022.txt
>>> @@ -6,7 +6,22 @@ Required properties:
>>>  - interrupts : Should contain SPI controller interrupt
>>>  
>>>  Optional properties:
>>> +- pl022,num-chipselects : total number of chipselects
>>
>> We have this for other spi controllers too. Define a generic property.
> 
> Currently, we have:
> 
> num-cs
> ti,spi-num-cs
> fsl,spi-num-chipselects
> fsl,espi-num-chipselects
> (pl022,num-chipselects)
> 
> Would "num-cs" be the right one to use?

Well, it is either that or invent something new, so I think num-cs is fine.

Rob

Re: [PATCH RESEND v4] spi/pl022: add devicetree support

[Linus Walleij]

(Currently make sure to include Mark Brown (see To: line) on all SPI patches,
as he's collecting them.)

On Mon, Aug 20, 2012 at 1:39 PM, Alexandre Pereira da Silva
<aletes.xgr@gmail.com> wrote:

> Add the chipselect array and cur_cs properties to pl022 main structure

OK...

> diff --git a/Documentation/devicetree/bindings/spi/spi_pl022.txt b/Documentation/devicetree/bindings/spi/spi_pl022.txt

This looks fine to me.

> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>

Hm, it looks like two changes are embedded in this patch.

>
>  /*
>   * This macro is used to define some register default values.
> @@ -389,6 +391,8 @@ struct pl022 {
>         char                            *dummypage;
>         bool                            dma_running;
>  #endif
> +       int cur_cs;
> +       int chipselect[0];
>  };

You forgot to add kerneldoc for these two. Please add!

int chipselect[0] really? isn't int *chipselect what you really want
to store in there?

> +       if (gpio_is_valid(pl022->cur_cs))
> +               gpio_set_value(pl022->cur_cs, command);
> +       else
> +               pl022->cur_chip->cs_control(command);
> +}

So it seems like this should be two patches:

- One adding pl022->cur_cs and the ability for the driver to control
the chip select directly from msg->spi->chip_select
Make sure it is possible to populate pl022->chipselect also
from platform data since many people are using that
still.

- Another patch adding device tree and that stuff.

So please split it in two.

> -                       pl022->cur_chip->cs_control(SSP_CHIP_DESELECT);
> +                       pl022_cs_control(pl022, SSP_CHIP_DESELECT);

All these are OK and go in the first patch.

> +       pl022->cur_cs = pl022->chipselect[msg->spi->chip_select];

Goes into the first patch...

> @@ -1761,12 +1772,14 @@ static const struct pl022_config_chip pl022_default_chip_info = {
>  static int pl022_setup(struct spi_device *spi)
>  {
>         struct pl022_config_chip const *chip_info;
> +       struct pl022_config_chip chip_info_dt;

Goes into second patch.

>         struct chip_data *chip;
>         struct ssp_clock_params clk_freq = { .cpsdvsr = 0, .scr = 0};
>         int status = 0;
>         struct pl022 *pl022 = spi_master_get_devdata(spi->master);
>         unsigned int bits = spi->bits_per_word;
>         u32 tmp;
> +       struct device_node *np = spi->dev.of_node;

Does this compile if CONFIG_OF is not selected?

I have seen other drivers have #ifdef CONFIG_OF around these
things.

Make sure you test-compile without DT.

> @@ -1789,10 +1802,36 @@ static int pl022_setup(struct spi_device *spi)
>         chip_info = spi->controller_data;
>
>         if (chip_info == NULL) {
> -               chip_info = &pl022_default_chip_info;
> -               /* spi_board_info.controller_data not is supplied */
> -               dev_dbg(&spi->dev,
> -                       "using default controller_data settings\n");
> +               if (np) {
> +                       chip_info_dt = pl022_default_chip_info;
> +
> +                       of_property_read_u32(np, "pl022,hierarchy",
> +                               &chip_info_dt.hierarchy);
> +                       of_property_read_u32(np, "pl022,interface",
> +                               &chip_info_dt.iface);
> +                       chip_info_dt.slave_tx_disable =
> +                               of_property_read_bool(np,
> +                                       "pl022,slave-tx-disable");
> +                       of_property_read_u32(np, "pl022,com-mode",
> +                               &chip_info_dt.com_mode);
> +                       of_property_read_u32(np, "pl022,rx-level-trig",
> +                               &chip_info_dt.rx_lev_trig);
> +                       of_property_read_u32(np, "pl022,tx-level-trig",
> +                               &chip_info_dt.tx_lev_trig);
> +                       of_property_read_u32(np, "pl022,ctrl-len",
> +                               &chip_info_dt.ctrl_len);
> +                       of_property_read_u32(np, "pl022,wait-state",
> +                               &chip_info_dt.wait_state);
> +                       of_property_read_u32(np, "pl022,duplex",
> +                               &chip_info_dt.duplex);
> +
> +                       chip_info = &chip_info_dt;
> +               } else {
> +                       chip_info = &pl022_default_chip_info;
> +                       /* spi_board_info.controller_data not is supplied */
> +                       dev_dbg(&spi->dev,
> +                               "using default controller_data settings\n");
> +               }

Goes into second patch.

> @@ -1835,8 +1874,9 @@ static int pl022_setup(struct spi_device *spi)
>         chip->xfer_type = chip_info->com_mode;
>         if (!chip_info->cs_control) {
>                 chip->cs_control = null_cs_control;
> -               dev_warn(&spi->dev,
> -                        "chip select function is NULL for this chip\n");
> +               if (!gpio_is_valid(pl022->chipselect[spi->chip_select]))
> +                       dev_warn(&spi->dev,
> +                                "chip select function is NULL for this chip\n");
>         } else
>                 chip->cs_control = chip_info->cs_control;

Goes into first patch.

> @@ -1988,7 +2028,8 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>         struct pl022_ssp_controller *platform_info = adev->dev.platform_data;
>         struct spi_master *master;
>         struct pl022 *pl022 = NULL;     /*Data for this driver */
> -       int status = 0;
> +       struct device_node *np = adev->dev.of_node;

Does this compile without CONFIG_OF?
(Second patch)

> +       int status = 0, i, num_cs;
>
>         dev_info(&adev->dev,
>                  "ARM PL022 driver, device ID: 0x%08x\n", adev->periphid);
> @@ -1998,8 +2039,14 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>                 goto err_no_pdata;
>         }
>
> +       num_cs = platform_info->num_chipselect;
> +       if (IS_ENABLED(CONFIG_OF))
> +               of_property_read_u32(np, "pl022,num-chipselects", &num_cs);
> +
> +

Shouldn't it be the other way around: platform data overrides
DT data. Attempt DT and if platform_info->num_chipselect
!= 0 let it override.

(Second patch.)

>         /* Allocate master with space for data */
> -       master = spi_alloc_master(dev, sizeof(struct pl022));
> +       master = spi_alloc_master(dev,
> +               sizeof(struct pl022) + sizeof(int) * num_cs);

First patch.

>         if (master == NULL) {
>                 dev_err(&adev->dev, "probe - cannot alloc SPI master\n");
>                 status = -ENOMEM;
> @@ -2017,13 +2064,41 @@ pl022_probe(struct amba_device *adev, const struct amba_id *id)
>          * on this board
>          */
>         master->bus_num = platform_info->bus_id;
> -       master->num_chipselect = platform_info->num_chipselect;
> +       master->num_chipselect = num_cs;

OK first patch.

>         master->cleanup = pl022_cleanup;
>         master->setup = pl022_setup;
>         master->prepare_transfer_hardware = pl022_prepare_transfer_hardware;
>         master->transfer_one_message = pl022_transfer_one_message;
>         master->unprepare_transfer_hardware = pl022_unprepare_transfer_hardware;
>         master->rt = platform_info->rt;
> +       master->dev.of_node = dev->of_node;

Does this compile without CONFIG_OF?
Second patch.

> +       if (IS_ENABLED(CONFIG_OF)) {
> +               for (i = 0; i < num_cs; i++) {
> +                       int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
> +
> +                       if (cs_gpio == -EPROBE_DEFER) {
> +                               status = -EPROBE_DEFER;
> +                               goto err_no_gpio;
> +                       }
> +
> +                       pl022->chipselect[i] = cs_gpio;
> +
> +                       if (gpio_is_valid(cs_gpio)) {
> +                               if (gpio_request(cs_gpio, "ssp-pl022"))
> +                                       dev_err(&adev->dev,
> +                                               "could not request %d gpio\n",
> +                                               cs_gpio);
> +                               else if (gpio_direction_output(cs_gpio, 1))
> +                                       dev_err(&adev->dev,
> +                                               "could set gpio %d as output\n",
> +                                               cs_gpio);
> +                       }
> +               }
> +       } else {
> +               for (i = 0; i < num_cs; i++)
> +                       pl022->chipselect[i] = -EINVAL;

Why? Instead, add a  int *chipselects; field to the struct pl022_ssp_controller
platform data in include/linux/amba/pl022.h and copy it from there if
num_chipselects in the same data != 0.

(First patch.)

Yours,
Linus Walleij