Skip to contentSkip to conversation

xilinx_spi: Split into of driver and generic part.

PATCH Superseded Richard Röjfors 2 participants 2 messages Nov 11, 2009

Subsystems (2)
SPI Arm/Zynq architecture

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Richard Röjfors

author
#0 lore raw

PATCH 1/4 xilinx_spi: Split into of driver and generic part.

This patch splits the xilinx_spi driver into a generic part and a OF driver part.

The reason for this is to later add in a platform driver as well.

Signed-off-by Richard Röjfors <[email protected]>

---

drivers/spi/Kconfig +7 -1
index 4b6f7cb..e60b264 100644
@@ -236,7 +236,7 @@ config SPI_TXX9
 
 config SPI_XILINX
 	tristate "Xilinx SPI controller"
-	depends on (XILINX_VIRTEX || MICROBLAZE) && EXPERIMENTAL
+	depends on EXPERIMENTAL
 	select SPI_BITBANG
 	help
 	  This exposes the SPI controller IP from the Xilinx EDK.

@@ -244,6 +244,12 @@ config SPI_XILINX
 	  See the "OPB Serial Peripheral Interface (SPI) (v1.00e)"
 	  Product Specification document (DS464) for hardware details.
 
+config SPI_XILINX_OF
+	tristate "Xilinx SPI controller OF device"
+	depends on SPI_XILINX && (XILINX_VIRTEX || MICROBLAZE)
+	help
+	  This is the OF driver for the SPI controller IP from the Xilinx EDK.
+
 #
 # Add new SPI master controllers in alphabetical order above this line
 #
drivers/spi/Makefile +1 -0
index 21a1182..97dee8f 100644
@@ -31,6 +31,7 @@ obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
 obj-$(CONFIG_SPI_TXX9)			+= spi_txx9.o
 obj-$(CONFIG_SPI_XILINX)		+= xilinx_spi.o
+obj-$(CONFIG_SPI_XILINX_OF)		+= xilinx_spi_of.o
 obj-$(CONFIG_SPI_SH_SCI)		+= spi_sh_sci.o
 obj-$(CONFIG_SPI_STMP3XXX)		+= spi_stmp.o
 # 	... add above this line ...
drivers/spi/xilinx_spi.c +34 -120

220 lines not on this page · raw message

drivers/spi/xilinx_spi.h new file +31 -0
new file mode 100644
index 0000000..84c98ee
@@ -0,0 +1,31 @@
+/*
+ * xilinx_spi.h
+ * Copyright (c) 2009 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef _XILINX_SPI_H_
+#define _XILINX_SPI_H_ 1
+
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_bitbang.h>
+
+#define XILINX_SPI_NAME "xilinx_spi"
+
+struct spi_master *xilinx_spi_init(struct device *dev, struct resource *mem,
+	u32 irq, s16 bus_num, u16 num_chipselect);
+
+void xilinx_spi_deinit(struct spi_master *master);
+#endif
drivers/spi/xilinx_spi_of.c new file +126 -0

126 lines not on this page · raw message

#1 lore raw
Quoted 4 lines · Richard Röjfors

This patch splits the xilinx_spi driver into a generic part and a OF driver part.

The reason for this is to later add in a platform driver as well.

Hey Richard,

Thanks for the quick response. A couple of important comments, and a bunch of nitpicks. Comments below.

Quoted 26 lines
drivers/spi/xilinx_spi.c +1 -6
index 46b8c5c..1562e9b 100644
@@ -14,11 +14,6 @@
  #include <linux/module.h>
  #include <linux/init.h>
  #include <linux/interrupt.h>
-#include <linux/platform_device.h>
-
-#include <linux/of_platform.h>
-#include <linux/of_device.h>
-#include <linux/of_spi.h>
 
  #include <linux/spi/spi.h>
  #include <linux/spi/spi_bitbang.h>

@@ -78,7 +73,7 @@ struct xilinx_spi {
        /* bitbang has to be first */
        struct spi_bitbang bitbang;
        struct completion done;
-
+       struct resource mem; /* phys mem */

       void __iomem    *regs;  /* virt. address of the control registers */

       u32             irq;

drivers/spi/xilinx_spi.c static irqreturn_t xilinx_spi_irq(int irq, void *dev_id) · line 282

Quoted 7 lines
drivers/spi/xilinx_spi.c +2 -2
@@ -283,40 +278,17 @@ static irqreturn_t xilinx_spi_irq(int irq, void *dev_id)
        return IRQ_HANDLED;
  }
 
-static int __init xilinx_spi_of_probe(struct of_device *ofdev,
-                                       const struct of_device_id *match)
+struct spi_master *xilinx_spi_init(struct device *dev, struct resource *mem,
+       u32 irq, s16 bus_num, u16 num_chipselect)

Nit: I personally prefer like _setup and _teardown instead of _init and _deinit; but that's just me. (and bike sheds should be blue)

Also, in patch 4/4, a new platform_data structure is defined. The platform probe routine extracts the pdata and passes each item individually to _init. The of_driver probe does the same, except it extracts the data from the device tree. That is also the approach that I used to take when writing drivers with multiple bindings. However, it becomes a problem as a driver matures and more and more data needs to be passed.

Instead, how about getting the of_driver probe to allocate and populate the pdata structure and stow it in of_dev->dev.platform_data.
That way the _init function signature stays sane, the platform driver
code becomes simpler, and the driver is better prepared to handle the eventual deprecation of the of_platform bus.

Quoted 25 lines
drivers/spi/xilinx_spi.c +2 -18
  {
        struct spi_master *master;
        struct xilinx_spi *xspi;
-       struct resource r_irq_struct;
-       struct resource r_mem_struct;
-
-       struct resource *r_irq = &r_irq_struct;
-       struct resource *r_mem = &r_mem_struct;
-       int rc = 0;
-       const u32 *prop;
-       int len;
+       int ret = 0;
 
-       /* Get resources(memory, IRQ) associated with the device */
-       master = spi_alloc_master(&ofdev->dev, sizeof(struct xilinx_spi));
-
-       if (master == NULL) {
-               return -ENOMEM;
-       }
+       master = spi_alloc_master(dev, sizeof(struct xilinx_spi));
 
-       dev_set_drvdata(&ofdev->dev, master);
-
-       rc = of_address_to_resource(ofdev->node, 0, r_mem);
-       if (rc) {

drivers/spi/xilinx_spi.c static irqreturn_t xilinx_spi_irq(int irq, void *dev_id) · line 290

Quoted 10 lines
drivers/spi/xilinx_spi.c +1 -9
-               dev_warn(&ofdev->dev, "invalid address\n");
-               goto put_master;
-       }
-
-       rc = of_irq_to_resource(ofdev->node, 0, r_irq);
-       if (rc == NO_IRQ) {
-               dev_warn(&ofdev->dev, "no IRQ found\n");
-               goto put_master;
-       }
+       if (master == NULL)

Nit: 'if (!master)' is a pretty well accepted idiom.

Quoted 1 line
drivers/spi/xilinx_spi.c +0 -0
@@ -329,128 +301,70 @@ static int __init xilinx_spi_of_probe(struct of_device *ofdev,

[...]

Quoted 8 lines

 free_irq:
       free_irq(xspi->irq, xspi);
 unmap_io:
       iounmap(xspi->regs);

drivers/spi/xilinx_spi.c +2 -2
-release_mem:
-       release_mem_region(r_mem->start, resource_size(r_mem));
+map_failed:
+       release_mem_region(mem->start, resource_size(mem));
Quoted 4 lines

 put_master:
       spi_master_put(master);
-       return rc;
+       return ERR_PTR(ret);

The SPI subsystem does not use the ERR_PTR() pattern, and errors are already printed to the console when a failure occurs. Please return NULL on failure so that the driver isn't mixing idioms.

drivers/spi/xilinx_spi.h line 2

Quoted 8 lines
drivers/spi/xilinx_spi.h new file +2 -0
new file mode 100644
index 0000000..84c98ee
@@ -0,0 +1,31 @@
+/*
+ * xilinx_spi.h

Nit: filename is kind of meaningless. Say what that file /is/ instead.
ie: * Xilinx SPI device driver API and platform data header file

Quoted 8 lines
drivers/spi/xilinx_spi.h new file +8 -0
+ * Copyright (c) 2009 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of

drivers/spi/xilinx_spi.h line 20

Quoted 10 lines
drivers/spi/xilinx_spi.h new file +10 -0
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#ifndef _XILINX_SPI_H_
+#define _XILINX_SPI_H_ 1

Nit: The '1' is unnecessary

Quoted 17 lines
drivers/spi/xilinx_spi.h new file +11 -0
+
+#include <linux/spi/spi.h>
+#include <linux/spi/spi_bitbang.h>
+
+#define XILINX_SPI_NAME "xilinx_spi"
+
+struct spi_master *xilinx_spi_init(struct device *dev, struct resource *mem,
+       u32 irq, s16 bus_num, u16 num_chipselect);
+
+void xilinx_spi_deinit(struct spi_master *master);
+#endif
drivers/spi/xilinx_spi_of.c new file +0 -0
new file mode 100644
index 0000000..5440253

drivers/spi/xilinx_spi_of.c line 2

Quoted 2 lines
drivers/spi/xilinx_spi_of.c new file +2 -0
@@ -0,0 +1,126 @@
+/*
+ * xilinx_spi_of.c Support for Xilinx SPI OF devices

Ditto nit here.

Looking good. Thanks for this work.

g.

Signature · 2 lines
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.