Re: [PATCH] SPI: Add driver for Cadence SPI controller

Hi Rob,

On 03/17/2014 01:47 PM, Rob Herring wrote:

On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam [off-list ref] wrote:

quoted

Add driver for Cadence SPI controller. This is used in Xilinx Zynq.

Signed-off-by: Harini Katakam <redacted>
---
 .../devicetree/bindings/spi/spi-cadence.txt        |   25 +

We prefer binding docs in separate patch.

I have heard several times that also for binding review you need driver
to look if this binding make sense that's why Harini sent this together.
It means 2 emails instead of one.
But if you wish to have this in two files no problem to split it
but then I believe both should be copied to DT mailing list.

quoted

 drivers/spi/Kconfig                                |    7 +
 drivers/spi/Makefile                               |    1 +
 drivers/spi/spi-cadence.c                          |  804 ++++++++++++++++++++
 4 files changed, 837 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spi/spi-cadence.txt
 create mode 100644 drivers/spi/spi-cadence.c

diff --git a/Documentation/devicetree/bindings/spi/spi-cadence.txt b/Documentation/devicetree/bindings/spi/spi-cadence.txt
new file mode 100644
index 0000000..d25bc2d
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/spi-cadence.txt

@@ -0,0 +1,25 @@
+Cadence SPI controller Device Tree Bindings
+-------------------------------------------
+
+Required properties:
+- compatible           : Should be "cdns,spi-r1p6".

One problem with using IP vendor info in the compatible string is an
IP block typically has a variety of configuration options so the
actual implementations may be very different. I would recommend adding
a Xilinx compatible string as well even if you don't use it now.

It means xlnx,zynq-spi-r1p6 should be fine.

quoted

+- reg                  : Physical base address and size of SPI registers map.
+- interrupts           : Property with a value describing the interrupt
+                         number.
+- interrupt-parent     : Must be core interrupt controller
+- clock-names          : List of input clock names - "ref_clk", "pclk"
+                         (See clock bindings for details).
+- clocks               : Clock phandles (see clock bindings for details).
+- num-chip-select      : Number of chip selects used.

See Documentation/devicetree/bindings/spi/spi-bus.txt. Use "num-cs" here.

quoted

+
+Example:
+
+       spi@e0007000 {
+               clock-names = "ref_clk", "pclk";
+               clocks = <&clkc 26>, <&clkc 35>;
+               compatible = "cdns,spi-r1p6";

Nit. We usually put compatible first in the node.

Our device-tree generator sorts them that's why it is just like this
but not a problem to fix just in documentation.

quoted

+               interrupt-parent = <&intc>;
+               interrupts = <0 49 4>;
+               num-chip-select = /bits/ 16 <4>;

I was expecting you will comment this a little bit. :-)
Because all just reading this num-cs as 32bit and then
assigning this value to master->num_chipselect which is 16bit.

<snip>

quoted

+/* Macros for the SPI controller read/write */
+#define cdns_spi_read(addr)    readl_relaxed(addr)
+#define cdns_spi_write(addr, val)      writel_relaxed((val), (addr))

Just use readl/writel directly.

There shouldn't be any problem to use these helper macros
and even I think this is better than using readl/writel directly
because you are more flexible if you want to change it to different
IO.

<snip>

quoted

+/**
+ * cdns_spi_probe - Probe method for the SPI driver
+ * @pdev:      Pointer to the platform_device structure
+ *
+ * This function initializes the driver data structures and the hardware.
+ *
+ * Return:     0 on success and error value on error
+ */
+static int cdns_spi_probe(struct platform_device *pdev)
+{
+       int ret = 0, irq;
+       struct spi_master *master;
+       struct cdns_spi *xspi;
+       struct resource *res;
+
+       master = spi_alloc_master(&pdev->dev, sizeof(*xspi));
+       if (master == NULL)
+               return -ENOMEM;
+
+       xspi = spi_master_get_devdata(master);
+       master->dev.of_node = pdev->dev.of_node;
+       platform_set_drvdata(pdev, master);
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       xspi->regs = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(xspi->regs)) {
+               ret = PTR_ERR(xspi->regs);
+               goto remove_master;
+       }
+
+       irq = platform_get_irq(pdev, 0);
+       if (irq < 0) {

I believe this returns NO_IRQ which could be 0.

s/</<=/

Are you sure regarding this?
NO_IRQ on ARM is -1.
And IRC irq = 0 should be just valid number.

But if you are right then others drivers have to fixed too.

quoted

+       return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(cdns_spi_dev_pm_ops, cdns_spi_suspend,
+                        cdns_spi_resume);
+
+/* Work with hotplug and coldplug */
+MODULE_ALIAS("platform:" CDNS_SPI_NAME);

Not sure, but I don't think this should be needed.

I don't know too.

Thanks for review,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform