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

[Michal Simek]

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