[PATCH v6 1/3] spi/pl022: Add chip select handling via GPIO

[PATCH v6 1/3] spi/pl022: Add chip select handling via GPIO

[Roland Stigge]

This patch adds the ability for the driver to control the chip select directly.
This enables independence from cs_control callbacks.  Configurable via
platform_data, to be extended as DT in the following patch.

Based on the initial patch by Alexandre Pereira da Silva <aletes.xgr-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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

Patch set changes since v5:
* struct pl022: renamed "chipselect" -> "chipselects"
* Fixed dev_warn() message
* Improved pointer construction for chipselect data
* Added DT documentation for "num-cs" property (patch 3/3)

Patch set changes since v4:
* Rename DT property: "pl022,num-chipselects" -> "num-cs"
* Removed reference to Linux code in DT binding documentation
* Removed property "pl022,hierarchy" - only support SPI master for now
* Documented DT property pl022,interface
* Removed property "pl022,slave-tx-disable" - not relevant in master mode
* Added kerneldoc for cur_cs and chipselect list
* Reorganized struct pl022 (int *chipselects)
* Introduced int *chipselects to struct pl022_ssp_controller
* Let platform data override DT data
* Split patches into CS handling vs. DT support

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, Rob Herring and Linus Walleij for reviewing!

 drivers/spi/spi-pl022.c    |   48 +++++++++++++++++++++++++++++++--------------
 include/linux/amba/pl022.h |    2 +
 2 files changed, 36 insertions(+), 14 deletions(-)

--- linux-2.6.orig/drivers/spi/spi-pl022.c
+++ linux-2.6/drivers/spi/spi-pl022.c
@@ -40,6 +40,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/scatterlist.h>
 #include <linux/pm_runtime.h>
+#include <linux/gpio.h>
 
 /*
  * This macro is used to define some register default values.
@@ -356,6 +357,8 @@ struct vendor_data {
  * @sgt_rx: scattertable for the RX transfer
  * @sgt_tx: scattertable for the TX transfer
  * @dummypage: a dummy page used for driving data on the bus with DMA
+ * @cur_cs: current chip select (gpio)
+ * @chipselects: list of chipselects (gpios)
  */
 struct pl022 {
 	struct amba_device		*adev;
@@ -389,6 +392,8 @@ struct pl022 {
 	char				*dummypage;
 	bool				dma_running;
 #endif
+	int cur_cs;
+	int *chipselects;
 };
 
 /**
@@ -433,6 +438,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 +492,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;
 
@@ -818,8 +831,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);
@@ -1252,8 +1264,7 @@ static irqreturn_t pl022_interrupt_handl
 		/* 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);
@@ -1338,7 +1349,7 @@ static void pump_transfers(unsigned long
 
 		/* 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;
@@ -1377,7 +1388,7 @@ static void do_interrupt_dma_transfer(st
 
 	/* 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 */
@@ -1429,12 +1440,12 @@ static void do_polling_transfer(struct p
 			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 */
@@ -1466,7 +1477,7 @@ static void do_polling_transfer(struct p
 		/* 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);
 	}
@@ -1495,6 +1506,7 @@ static int pl022_transfer_one_message(st
 
 	/* Setup the SPI using the per chip configuration */
 	pl022->cur_chip = spi_get_ctldata(msg->spi);
+	pl022->cur_cs = pl022->chipselects[msg->spi->chip_select];
 
 	restore_state(pl022);
 	flush(pl022);
@@ -1840,8 +1852,9 @@ static int pl022_setup(struct spi_device
 	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->chipselects[spi->chip_select]))
+			dev_warn(&spi->dev,
+				 "invalid chip select\n");
 	} else
 		chip->cs_control = chip_info->cs_control;
 
@@ -1993,7 +2006,7 @@ pl022_probe(struct amba_device *adev, co
 	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;
+	int status = 0, i;
 
 	dev_info(&adev->dev,
 		 "ARM PL022 driver, device ID: 0x%08x\n", adev->periphid);
@@ -2004,7 +2017,8 @@ pl022_probe(struct amba_device *adev, co
 	}
 
 	/* Allocate master with space for data */
-	master = spi_alloc_master(dev, sizeof(struct pl022));
+	master = spi_alloc_master(dev, sizeof(struct pl022) + sizeof(int) *
+				  platform_info->num_chipselect);
 	if (master == NULL) {
 		dev_err(&adev->dev, "probe - cannot alloc SPI master\n");
 		status = -ENOMEM;
@@ -2016,6 +2030,8 @@ pl022_probe(struct amba_device *adev, co
 	pl022->master_info = platform_info;
 	pl022->adev = adev;
 	pl022->vendor = id->data;
+	/* Point chipselects to allocated memory beyond the main struct */
+	pl022->chipselects = (int *) pl022 + sizeof(struct pl022);
 
 	/*
 	 * Bus Number Which has been Assigned to this SSP controller
@@ -2030,6 +2046,10 @@ pl022_probe(struct amba_device *adev, co
 	master->unprepare_transfer_hardware = pl022_unprepare_transfer_hardware;
 	master->rt = platform_info->rt;
 
+	if (platform_info->num_chipselect && platform_info->chipselects)
+		for (i = 0; i < platform_info->num_chipselect; i++)
+			pl022->chipselects[i] = platform_info->chipselects[i];
+
 	/*
 	 * Supports mode 0-3, loopback, and active low CS. Transfers are
 	 * always MS bit first on the original pl022.
--- linux-2.6.orig/include/linux/amba/pl022.h
+++ linux-2.6/include/linux/amba/pl022.h
@@ -244,6 +244,7 @@ struct dma_chan;
  *     indicates no delay and the device will be suspended immediately.
  * @rt: indicates the controller should run the message pump with realtime
  *     priority to minimise the transfer latency on the bus.
+ * @chipselects: list of <num_chipselects> chip select gpios
  */
 struct pl022_ssp_controller {
 	u16 bus_id;
@@ -254,6 +255,7 @@ struct pl022_ssp_controller {
 	void *dma_tx_param;
 	int autosuspend_delay;
 	bool rt;
+	int *chipselects;
 };
 
 /**

[PATCH v6 2/3] spi/pl022: Add devicetree support

[Roland Stigge]

This patch adds device tree support to the spi-pl022 driver.

Based on the initial patch by Alexandre Pereira da Silva <aletes.xgr-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

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

 Documentation/devicetree/bindings/spi/spi_pl022.txt |   15 +++
 drivers/spi/spi-pl022.c                             |   79 +++++++++++++++++---
 2 files changed, 85 insertions(+), 9 deletions(-)

--- linux-2.6.orig/Documentation/devicetree/bindings/spi/spi_pl022.txt
+++ linux-2.6/Documentation/devicetree/bindings/spi/spi_pl022.txt
@@ -6,7 +6,22 @@ Required properties:
 - interrupts : Should contain SPI controller interrupt
 
 Optional properties:
+- num-cs : 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.
+
+- pl022,interface : interface type:
+	0: SPI
+	1: Texas Instruments Synchronous Serial Frame Format
+	2: Microwire (Half Duplex)
+- 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
+
--- linux-2.6.orig/drivers/spi/spi-pl022.c
+++ linux-2.6/drivers/spi/spi-pl022.c
@@ -41,6 +41,7 @@
 #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.
@@ -1778,12 +1779,14 @@ static const struct pl022_config_chip pl
 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;
@@ -1806,10 +1809,32 @@ static int pl022_setup(struct spi_device
 	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;
+
+			chip_info_dt.hierarchy = SSP_MASTER;
+			of_property_read_u32(np, "pl022,interface",
+				&chip_info_dt.iface);
+			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");
@@ -2006,7 +2031,8 @@ pl022_probe(struct amba_device *adev, co
 	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, i;
+	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);
@@ -2016,9 +2042,19 @@ pl022_probe(struct amba_device *adev, co
 		goto err_no_pdata;
 	}
 
+	if (platform_info->num_chipselect) {
+		num_cs = platform_info->num_chipselect;
+	} else if (IS_ENABLED(CONFIG_OF)) {
+		of_property_read_u32(np, "num-cs", &num_cs);
+	} else {
+		dev_err(&adev->dev, "probe: no chip select defined\n");
+		status = -ENODEV;
+		goto err_no_pdata;
+	}
+
 	/* Allocate master with space for data */
 	master = spi_alloc_master(dev, sizeof(struct pl022) + sizeof(int) *
-				  platform_info->num_chipselect);
+				  num_cs);
 	if (master == NULL) {
 		dev_err(&adev->dev, "probe - cannot alloc SPI master\n");
 		status = -ENOMEM;
@@ -2038,17 +2074,41 @@ pl022_probe(struct amba_device *adev, co
 	 * 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 (platform_info->num_chipselect && platform_info->chipselects)
-		for (i = 0; i < platform_info->num_chipselect; i++)
+	if (platform_info->num_chipselect && platform_info->chipselects) {
+		for (i = 0; i < num_cs; i++)
 			pl022->chipselects[i] = platform_info->chipselects[i];
+	} else 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->chipselects[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);
+			}
+		}
+	}
 
 	/*
 	 * Supports mode 0-3, loopback, and active low CS. Transfers are
@@ -2158,6 +2218,7 @@ pl022_probe(struct amba_device *adev, co
  err_no_ioremap:
 	amba_release_regions(adev);
  err_no_ioregion:
+ err_no_gpio:
 	spi_master_put(master);
  err_no_master:
  err_no_pdata:

[PATCH v6 3/3] DT bindings documentation: "num-cs" property for SPI controllers

[Roland Stigge]

Several SPI controller drivers have defined differently named properties for
the number of chip selects.  Now adding "num-cs" as a reference name for new
bindings.

Signed-off-by: Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org>

---
 Documentation/devicetree/bindings/spi/spi-bus.txt |    3 +++
 1 file changed, 3 insertions(+)

--- linux-2.6.orig/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ linux-2.6/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -21,6 +21,9 @@ assumption that board specific platform
 chip selects.  Individual drivers can define additional properties to
 support describing the chip select layout.
 
+Optional property:
+- num-cs : total number of chipselects
+
 SPI slave nodes must be children of the SPI master node and can
 contain the following properties.
 - reg             - (required) chip select address of device.

Re: [PATCH v6 1/3] spi/pl022: Add chip select handling via GPIO

[Linus Walleij]

On Wed, Aug 22, 2012 at 3:49 PM, Roland Stigge <stigge@antcom.de> wrote:

> This patch adds the ability for the driver to control the chip select directly.
> This enables independence from cs_control callbacks.  Configurable via
> platform_data, to be extended as DT in the following patch.
>
> Based on the initial patch by Alexandre Pereira da Silva <aletes.xgr@gmail.com>
>
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> Acked-by: Alexandre Pereira da Silva <aletes.xgr@gmail.com>

Thanks Roland, excellent work!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v6 2/3] spi/pl022: Add devicetree support

[Linus Walleij]

On Wed, Aug 22, 2012 at 3:49 PM, Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org> wrote:

> This patch adds device tree support to the spi-pl022 driver.
>
> Based on the initial patch by Alexandre Pereira da Silva <aletes.xgr-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Signed-off-by: Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org>
> Acked-by: Alexandre Pereira da Silva <aletes.xgr-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij

Re: [PATCH v6 3/3] DT bindings documentation: "num-cs" property for SPI controllers

[Linus Walleij]

On Wed, Aug 22, 2012 at 3:49 PM, Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org> wrote:

> Several SPI controller drivers have defined differently named properties for
> the number of chip selects.  Now adding "num-cs" as a reference name for new
> bindings.
>
> Signed-off-by: Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org>

Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Yours,
Linus Walleij

Re: [PATCH v6 3/3] DT bindings documentation: "num-cs" property for SPI controllers

[Mark Brown]

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

On Wed, Aug 22, 2012 at 08:39:06PM +0200, Linus Walleij wrote:
> On Wed, Aug 22, 2012 at 3:49 PM, Roland Stigge <stigge@antcom.de> wrote:
> 
> > Several SPI controller drivers have defined differently named properties for
> > the number of chip selects.  Now adding "num-cs" as a reference name for new
> > bindings.

> > Signed-off-by: Roland Stigge <stigge@antcom.de>

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Applied all, thanks.

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

Re: [PATCH v6 1/3] spi/pl022: Add chip select handling via GPIO

[shiraz hashim]

Hi Roland,

On Wed, Aug 22, 2012 at 7:19 PM, Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org> wrote:
> @@ -2016,6 +2030,8 @@ pl022_probe(struct amba_device *adev, co
>         pl022->master_info = platform_info;
>         pl022->adev = adev;
>         pl022->vendor = id->data;
> +       /* Point chipselects to allocated memory beyond the main struct */
> +       pl022->chipselects = (int *) pl022 + sizeof(struct pl022);

This is going beyond memory allocated for chipselects
as it adds 4 * sizeof(struct pl022) bytes to pl022.

pl022->chipselects = (int *) &pl022[1];
can be musch safer.

-- 
regards
Shiraz Hashim

------------------------------------------------------------------------------
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 v6 1/3] spi/pl022: Add chip select handling via GPIO

[Linus Walleij]

On Sat, Sep 1, 2012 at 1:14 PM, shiraz hashim
<shiraz.linux.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Roland,
>
> On Wed, Aug 22, 2012 at 7:19 PM, Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org> wrote:
>> @@ -2016,6 +2030,8 @@ pl022_probe(struct amba_device *adev, co
>>         pl022->master_info = platform_info;
>>         pl022->adev = adev;
>>         pl022->vendor = id->data;
>> +       /* Point chipselects to allocated memory beyond the main struct */
>> +       pl022->chipselects = (int *) pl022 + sizeof(struct pl022);
>
> This is going beyond memory allocated for chipselects
> as it adds 4 * sizeof(struct pl022) bytes to pl022.

Yes that is why the allocation looks like this:

+       master = spi_alloc_master(dev, sizeof(struct pl022) + sizeof(int) *
+                                 platform_info->num_chipselect);

> pl022->chipselects = (int *) &pl022[1];
> can be musch safer.

I see absolutely no sematic difference between these two
methods to reach the first position beyond the first struct.

If we're gonna be debating this it's a safe sign that this is
not a good design pattern at all, so then it is better to simply
devm_kzalloc(sizeof(int) * platform_info->num_chipselect);
separately.

(But I'm happy with the patch as it is. And the other way
too, since I'm not very picky.)

Yours,
Linus Walleij

------------------------------------------------------------------------------
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 v6 1/3] spi/pl022: Add chip select handling via GPIO

[shiraz hashim]

Hi Linus,

On Sun, Sep 2, 2012 at 12:48 PM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Sat, Sep 1, 2012 at 1:14 PM, shiraz hashim
> <shiraz.linux.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hi Roland,
>>
>> On Wed, Aug 22, 2012 at 7:19 PM, Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org> wrote:
>>> @@ -2016,6 +2030,8 @@ pl022_probe(struct amba_device *adev, co
>>>         pl022->master_info = platform_info;
>>>         pl022->adev = adev;
>>>         pl022->vendor = id->data;
>>> +       /* Point chipselects to allocated memory beyond the main struct */
>>> +       pl022->chipselects = (int *) pl022 + sizeof(struct pl022);
>>
>> This is going beyond memory allocated for chipselects
>> as it adds 4 * sizeof(struct pl022) bytes to pl022.
>
> Yes that is why the allocation looks like this:
>
> +       master = spi_alloc_master(dev, sizeof(struct pl022) + sizeof(int) *
> +                                 platform_info->num_chipselect);
>

The allocation is such because type of chipselects is int.

The statement for allocation is correct, but

       pl022->chipselects = (int *) pl022 + sizeof(struct pl022);

is not adding  sizeof(struct pl022) bytes to pl022 base (which we want),
but infact 4 times the size of pl022 (because type of pl022 is now int *).

Do you get my point ?  This would go way beyond memory allocated
for chipselects.

-- 
regards
Shiraz Hashim

------------------------------------------------------------------------------
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 v6 1/3] spi/pl022: Add chip select handling via GPIO

[Roland Stigge]

Hi Shiraz,

On 01/09/12 13:14, shiraz hashim wrote:
> On Wed, Aug 22, 2012 at 7:19 PM, Roland Stigge <stigge@antcom.de> wrote:
>> @@ -2016,6 +2030,8 @@ pl022_probe(struct amba_device *adev, co
>>         pl022->master_info = platform_info;
>>         pl022->adev = adev;
>>         pl022->vendor = id->data;
>> +       /* Point chipselects to allocated memory beyond the main struct */
>> +       pl022->chipselects = (int *) pl022 + sizeof(struct pl022);
> 
> This is going beyond memory allocated for chipselects
> as it adds 4 * sizeof(struct pl022) bytes to pl022.
> 
> pl022->chipselects = (int *) &pl022[1];

Correct. Thanks for the heads up!

Funnily, my previous proposal way actually like you just proposed, but
we took the other one since it looked "better". ;-)

I'll provide an incremental bugfix patch since Mark already has the
commit in his misc.git tree.

Thanks again,

Roland

Re: [PATCH v6 1/3] spi/pl022: Add chip select handling via GPIO

[Linus Walleij]

On Sun, Sep 2, 2012 at 3:12 PM, shiraz hashim
<shiraz.linux.kernel-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Linus,

>> Yes that is why the allocation looks like this:
>>
>> +       master = spi_alloc_master(dev, sizeof(struct pl022) + sizeof(int) *
>> +                                 platform_info->num_chipselect);
>>
>
> The allocation is such because type of chipselects is int.
>
> The statement for allocation is correct, but
>
>        pl022->chipselects = (int *) pl022 + sizeof(struct pl022);
>
> is not adding  sizeof(struct pl022) bytes to pl022 base (which we want),
> but infact 4 times the size of pl022 (because type of pl022 is now int *).
>
> Do you get my point ?  This would go way beyond memory allocated
> for chipselects.

Yes of course ... how could I not see this. Sorry!

Yours,
Linus Walleij

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