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

[Michal Simek <monstr@monstr.eu>]

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

On 03/17/2014 08:00 PM, Rob Herring wrote:
> On Mon, Mar 17, 2014 at 8:22 AM, Michal Simek <monstr@monstr.eu> wrote:
>> Hi Rob,
>>
>> On 03/17/2014 01:47 PM, Rob Herring wrote:
>>> On Mon, Mar 17, 2014 at 7:05 AM, Harini Katakam <harinik@xilinx.com> wrote:
>>>> Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
>>>>
>>>> Signed-off-by: Harini Katakam <harinik@xilinx.com>
>>>> ---
>>>>  .../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.
> 
> Yes, for 2 reasons:
> - To prepare for DT bindings to get merged to separate repo if we ever
> get there.
> - So DT maintainers can review/ack just the bindings.

ok. No problem to divide it.

>>>> +- 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.
> 
> Well, everyone else has that problem then. Obviously it takes a bit
> more care than just reading into a u32, but that is a kernel problem
> and not a problem of the binding.

They are not reading it directly with read_u32 but they are using
intermediate u32 value which is assigned to u16 which is fine.
This pattern is in most drivers(maybe all).

The point is if binding should or can't simplify driver code.
And from your reaction above I expect that it is up to driver
owner and binding doc how you want to do it.

>>>> +/* 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.
> 
> Then when I read the code I have to go lookup what they do while I
> know exactly what writel/readl do already. It is really the same
> reasons as why the kernel doesn't have register set and clear bits
> accessors.

I understand your reasons but on the other hand if we want to run
ARM-BE then using direct readl/writel functions is also not correct.
Mark proposed one solution and I will see how it looks like.


>>>> +       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.
> 
> The definition varies by arch being 0 or -1, so drivers need to deal
> with both. The preference is 0 is NO_IRQ. It has been decreed by
> Linus. ARM is actually pretty close to being able to change NO_IRQ to
> 0.

I think that resolution was to completely remove NO_IRQ.

Thanks,
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]