linux-kernel - Re: [PATCH] SPI: Add driver for Cadence SPI controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL_JsqL9z_4T+cz425bHHpwx_dXaZjJEHwZKFCOibWYuWM4rkA@mail.gmail.com>
Date:	Mon, 17 Mar 2014 14:00:05 -0500
From:	Rob Herring <robherring2@...il.com>
To:	Michal Simek <monstr@...str.eu>
Cc:	Harini Katakam <harinik@...inx.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Rob Landley <rob@...dley.net>, Mark Brown <broonie@...nel.org>,
	Grant Likely <grant.likely@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-spi@...r.kernel.org" <linux-spi@...r.kernel.org>
Subject: Re: [PATCH] SPI: Add driver for Cadence SPI controller

On Mon, Mar 17, 2014 at 8:22 AM, Michal Simek <monstr@...str.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@...inx.com> wrote:
>>> Add driver for Cadence SPI controller. This is used in Xilinx Zynq.
>>>
>>> Signed-off-by: Harini Katakam <harinik@...inx.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.


>>> +- 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@...07000 {
>>> +               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.

>>> +/* 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.


>>> +       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.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/