On Fri, 30 Nov 2012 17:57:38 -0500, Murali Karicheri <m-karicheri2@xxxxxx> wrote:
> On 11/15/2012 11:20 AM, Grant Likely wrote:
> > On Mon, 12 Nov 2012 16:28:22 -0500, Murali Karicheri <m-karicheri2@xxxxxx> wrote:
> >> This adds OF support to DaVinci SPI controller to configure platform
> >> data through device bindings.
> >>
> >> Signed-off-by: Murali Karicheri <m-karicheri2@xxxxxx>
> > Hi Murali,
> >
> > Comments below...
> >
> >> ---
> >> .../devicetree/bindings/spi/spi-davinci.txt | 50 ++++++++++++
> >> drivers/spi/spi-davinci.c | 80 +++++++++++++++++++-
> >> 2 files changed, 126 insertions(+), 4 deletions(-)
> >> create mode 100644 Documentation/devicetree/bindings/spi/spi-davinci.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/spi/spi-davinci.txt b/Documentation/devicetree/bindings/spi/spi-davinci.txt
> >> new file mode 100644
> >> index 0000000..0369369
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/spi/spi-davinci.txt
> >> @@ -0,0 +1,50 @@
> >> +Davinci SPI controller device bindings
> >> +
> >> +Required properties:
> >> +- #address-cells: number of cells required to define a chip select
> >> + address on the SPI bus.
> > Will this *ever* be something other than '1'?
> >
> >> +- #size-cells: should be zero.
> >> +- compatible:
> >> + - "ti,davinci-spi"
> > Please use the actual model of the chip here. Don't try to be generic
> > with the compatible string. A driver can bind against more than one
> > compatible value and new devices can claim compatiblity with old by
> > including the old compatible string in this list.
> >
> >> +- reg: Offset and length of SPI controller register space
> >> +- ti,davinci-spi-version: SPI version :- "1.0" or "2.0"
> > Usually this is determined from the compatible value directly (which is
> > why compatible strings shouldn't be generic). Don't use a separate
> > property for it.
> >
> >> +- ti,davinci-spi-num-cs: Number of chip selects
> >> +- ti,davinci-spi-intr-line: interrupt line used to connect the SPI
> >> + IP to the ARM interrupt controller withn the SoC. Possible values
> >> + are 0 and 1.
> > ? Isn't that what the interrupts property is for? I don't understand why
> > this is needed from the description.
> >
> >> +- interrupts: interrupt number offset at the irq parent
> >> +- clocks: spi clk phandle
> >> +
> >> +Example of a NOR flash slave device (n25q032) connected to DaVinci
> >> +SPI controller device over the SPI bus.
> >> +
> >> +spi:spi0@20BF0000 {
> > spi@20BF0000 (use 'spi' not 'spi0')
> >
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> + compatible = "ti,davinci-spi";
> >> + reg = <0x20BF0000 0x1000>;
> >> + ti,davinci-spi-version = "1.0";
> >> + ti,davinci-spi-num-cs = <4>;
> >> + ti,davinci-spi-intr-line = <0>;
> >> + interrupts = <338>;
> >> + clocks = <&clkspi>;
> >> +
> >> + flash: n25q032@0 {
> >> + #address-cells = <1>;
> >> + #size-cells = <1>;
> >> + compatible = "st,m25p32";
> >> + spi-max-frequency = <25000000>;
> >> + reg = <0>;
> >> +
> >> + partition@0 {
> >> + label = "u-boot-spl";
> >> + reg = <0x0 0x80000>;
> >> + read-only;
> >> + };
> >> +
> >> + partition@1 {
> >> + label = "test";
> >> + reg = <0x80000 0x380000>;
> >> + };
> >> + };
> >> +};
> >> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
> >> index 71a6423..0f50319 100644
> >> --- a/drivers/spi/spi-davinci.c
> >> +++ b/drivers/spi/spi-davinci.c
> >> @@ -26,6 +26,9 @@
> >> #include <linux/err.h>
> >> #include <linux/clk.h>
> >> #include <linux/dma-mapping.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/of_irq.h>
> >> #include <linux/spi/spi.h>
> >> #include <linux/spi/spi_bitbang.h>
> >> #include <linux/slab.h>
> >> @@ -788,6 +791,69 @@ rx_dma_failed:
> >> return r;
> >> }
> >>
> >> +#if defined(CONFIG_OF)
> >> +static const struct of_device_id davinci_spi_of_match[] = {
> >> + {
> >> + .compatible = "ti,davinci-spi",
> >> + },
> >> + { },
> >> +};
> >> +MODULE_DEVICE_TABLE(of, davini_spi_of_match);
> >> +
> >> +/**
> >> + * spi_davinci_get_pdata - Get platform_data from DTS binding
> >> + * @pdev: ptr to platform data
> >> + *
> >> + * Parses and populate platform_data from device tree bindings.
> >> + *
> >> + * NOTE: Not all platform_data params are supported currently.
> >> + */
> >> +static struct davinci_spi_platform_data
> >> + *spi_davinci_get_pdata(struct platform_device *pdev)
> >> +{
> >> + struct device_node *node = pdev->dev.of_node;
> >> + struct davinci_spi_platform_data *pdata;
> >> + unsigned int num_cs, intr_line = 0;
> >> + const char *version;
> >> +
> >> + if (pdev->dev.platform_data)
> >> + return pdev->dev.platform_data;
> >> +
> >> + if (!pdev->dev.of_node)
> >> + return NULL;
> >> +
> >> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> >> + pdev->dev.platform_data = pdata;
> >> + if (!pdata)
> >> + return NULL;
> > Since pdata must always be present, roll it directly into struct
> > spi_davinci and get rid of the kzmalloc here. It is less expensive and
> > is simpler code.
> Grant,
>
> Could you elaborate a bit? What you mean by rolling pdata into
> spi_davinci? I believe you are referring
> to davinci_spi. Are you saying change following:-
>
> struct davinci_spi {
>
> ...
> struct davinci_spi_platform_data *pdata; <= to struct
> davinci_spi_platform_data pdata
>
> };
Not quite. I mean adding this:
struct davinci_spi {
...
struct davinci_spi_platform_data pdata;
};
And then in the probe path do something like:
{
struct davinci_spi_platform_data *pdata = pdev.dev.platform_data;
...
if (pdata)
dspi->pdata = *pdata; /* Copy from platform_data */
else
spi_davinci_get_pdata(pdev); /* decode from DT */
};
My point is that the driver needs a copy of the pdata. By embedding it
into struct davinci_spi it doesn't need to be allocated with a separate
devm_kzalloc() call.
This is a minor point though. You could do it either way.
*However* make sure that the driver *does not* save the pointer into
pdev->dev.platform_data. That field is read-only for drivers. If a
driver allocates a separate pdata structure, then it needs to store the
pointer to it inside its private data structure.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
