Hi Rob,
On 03/17/2014 01:47 PM, Rob Herring wrote:
> On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam <harinik@xxxxxxxxxx> wrote:
>> Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
>>
>> Signed-off-by: Harini Katakam <harinik@xxxxxxxxxx>
>> ---
>> .../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.
>> 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.
>
>> +- 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.
>
>> +
>> +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.
>> + 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>
>> +/* 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>
>> +/**
>> + * 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.
>> + 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
Attachment:
signature.asc
Description: OpenPGP digital signature
