Message-ID: <50941960.3060407@wwwdotorg.org>
Date: Fri, 02 Nov 2012 13:05:04 -0600
From: Stephen Warren <swarren@wwwdotorg.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120912 Thunderbird/15.0.1
MIME-Version: 1.0
To: Murali Karicheri <m-karicheri2@ti.com>
CC: dwmw2@infradead.org, artem.bityutskiy@linux.intel.com,
        b32955@freescale.com, shubhrajyoti@ti.com, wim@iguana.be, hs@denx.de,
        nsekhar@ti.com, linux-mtd@lists.infradead.org,
        linux-kernel@vger.kernel.org, grant.likely@secretlab.ca,
        rob.herring@calxeda.com, rob@landley.net, gregkh@linuxfoundation.org,
        hdoyu@nvidia.com, santosh.shilimkar@ti.com,
        devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org,
        davinci-linux-open-source@linux.davincidsp.com
Subject: Re: [RFC PATCH 1/2] memory: davinci - add aemif controller platform
 driver
References: <1351873278-27325-1-git-send-email-m-karicheri2@ti.com> <1351873278-27325-2-git-send-email-m-karicheri2@ti.com>
In-Reply-To: <1351873278-27325-2-git-send-email-m-karicheri2@ti.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3631
Lines: 91

On 11/02/2012 10:21 AM, Murali Karicheri wrote:
> This is a platform driver for asynchronous external memory interface
> available on TI SoCs. This driver was previously located inside the
> mach-davinci folder. As this DaVinci IP is re-used across multiple
> family of devices such as c6x, keystone etc, the driver is moved to drivers.
> The driver configures async bus parameters associated with a particular
> chip select. For DaVinci controller driver and driver for other async
> devices such as NOR flash, ASRAM etc, the bus confuguration is
> done by this driver at init time. A set of APIs (set/get) provided to
> update the values based on specific driver usage.

> +++ b/Documentation/devicetree/bindings/arm/davinci/aemif.txt

If the HW/binding is generic, I'd assume the documentation would be
place somewhere more like
Documentation/devicetree/bindings/memory/davinci-aemif.txt?

> @@ -0,0 +1,62 @@
> +* Texas Instruments Davinci AEMIF bus interface
> +
> +This file provides information for the davinci-emif chip select
> +bindings.

Shouldn't the binding be for an IP block (the AEMIF bus controller I
assume), rather than for a particular chip-select generated by the
controller?

> +This is a sub device node inside the davinci-emif device node
> +to describe a async bus for a specific chip select. For NAND,
> +CFI flash device bindings described inside an aemif node,
> +etc, a cs sub node is defined to associate the bus parameter
> +bindings used by the device.

Oh, this file only documents part of the controller's node? It seems
unusual to do that; the documentation for an entire node would usually
be in a single file, which seems to be
Documentation/devicetree/bindings/arm/davinci/nand.txt right now. Is
this "cs" sub-node really something that gets re-used across multiple
different memory controller IPs?

If so, I guess this file should be called something more like
davinci-aemif-cs.txt than davinci-aemif.txt. I'd suggest moving
arm/davinci/nand.txt into a common location too (and renaming it to
davici-nand.txt to better represent the compatible value it documents).

> +Required properties:=
> +- compatible: "ti,davinci-cs";
> +- #address-cells = <1>;
> +- #size-cells = <1>;
> +- cs - cs used by the device (NAND, CFI flash etc. values in the range: 2-5
> +
> +Optional properties:-
> +- asize - asynchronous data bus width (0 - 8bit, 1 - 16 bit)
> +  All of the params below in nanoseconds
> +
> +- ta - Minimum turn around time
> +- rhold - read hold width
> +- rstobe - read strobe width
> +- rsetup - read setup width
> +- whold - write hold width
> +- wstrobe - write strobe width
> +- wsetup - write setup width
> +- ss - enable/disable select strobe (0 - disable, 1 - enable)
> +- ew - enable/disable extended wait cycles (0 - disable, 1 - enable)

I assume all of those are pretty custom to this binding, and not
something you'd expect to re-use across multiple vendors' bindings? If
so, shouldn't they have a "ti," vendor prefix?

> +Example for davinci nand chip select
> +
> +aemif@60000000 {
> +
> +	compatible = "ti,davinci-aemif";

You need a reg property here.

> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +
> +	nand_cs:cs2@70000000 {
> +		compatible = "ti,davinci-cs";

You need a reg property here. The unit address "@70000000" in the node
name only has one address cell, whereas the parent node sets
#address-cells = <2>.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/