Patchwork [v2,1/4] xilinx_spi: Split into of driver and generic part.

login
register
mail settings
Submitter John Linn
Date Nov. 12, 2009, 5:17 p.m.
Message ID <38eacaec-f0d3-436c-b461-40b5d795cdd9@VA3EHSMHS009.ehs.local>
Download mbox | patch
Permalink /patch/38260/
State Superseded
Delegated to: Grant Likely
Headers show

Comments

John Linn - Nov. 12, 2009, 5:17 p.m.
> -----Original Message-----
> From: glikely@secretlab.ca [mailto:glikely@secretlab.ca] On Behalf Of Grant Likely
> Sent: Thursday, November 12, 2009 7:56 AM
> To: Richard Röjfors
> Cc: spi-devel-general@lists.sourceforge.net; linuxppc-dev@ozlabs.org; Andrew Morton;
> dbrownell@users.sourceforge.net; John Linn
> Subject: Re: [PATCH v2 1/4] xilinx_spi: Split into of driver and generic part.
> 
> On Thu, Nov 12, 2009 at 7:26 AM, Richard Röjfors
> <richard.rojfors@mocean-labs.com> wrote:
> > This patch splits the xilinx_spi driver into a generic part and a
> > OF driver part.
> >
> > The reason for this is to later add in a platform driver as well.
> >
> > Signed-off-by: Richard Röjfors <richard.rojfors@mocean-labs.com>
> > ---

<snip>

> > +
> > +       if (!ofdev->dev.platform_data) {
> > +               ofdev->dev.platform_data =
> > +                       kzalloc(sizeof(struct xspi_platform_data), GFP_KERNEL);
> > +               if (!ofdev->dev.platform_data)
> > +                       return -ENOMEM;
> > +       }
> 
> Minor memory leak.  Anything alloced in the probe path should also be
> freed in the remove path.  It's not going to spiral out of control or
> anything, but it is important to be strict about such things.  Drop
> the outer if{} block here and kfree platform_data on remove.
> 
> > +
> > +       /* number of slave select bits is required */
> > +       prop = of_get_property(ofdev->node, "xlnx,num-ss-bits", &len);
> > +       if (!prop || len < sizeof(*prop)) {
> > +               dev_warn(&ofdev->dev, "no 'xlnx,num-ss-bits' property\n");
> > +               return -EINVAL;
> > +       }
> > +       ofdev->dev.platform_data->num_chipselect = *prop;
> 
> Have you compile tested this?  platform_data is a void*, the
> dereference will not work.  I know you don't have hardware; but
> getting the needed cross compiler is easy.

Here's the fixes I did to make it compile. On to testing :)

Thanks,
John


> 
> > +       master = xilinx_spi_init(&ofdev->dev, r_mem, r_irq->start, -1);
> > +       if (!master)
> > +               return -ENODEV;
> > +
> > +       dev_set_drvdata(&ofdev->dev, master);
> > +
> > +       /* Add any subnodes on the SPI bus */
> > +       of_register_spi_devices(master, ofdev->node);
> > +
> > +       return 0;
> > +}


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
Richard Röjfors - Nov. 12, 2009, 5:22 p.m.
On 11/12/09 6:17 PM, John Linn wrote:
>> Have you compile tested this?  platform_data is a void*, the
>> dereference will not work.  I know you don't have hardware; but
>> getting the needed cross compiler is easy.
> 
> Here's the fixes I did to make it compile. On to testing :)

Great! I tested this against a max7301 GPIO expander.

--Richard

Patch

diff --git a/drivers/spi/xilinx_spi_of.c b/drivers/spi/xilinx_spi_of.c
index 13e1591..2dea9f3 100644
--- a/drivers/spi/xilinx_spi_of.c
+++ b/drivers/spi/xilinx_spi_of.c
@@ -39,6 +39,7 @@ 
 static int __init xilinx_spi_of_probe(struct of_device *ofdev,
 					const struct of_device_id *match)
 {
+	struct xspi_platform_data *pdata = ofdev->dev.platform_data;
 	struct resource r_irq_struct;
 	struct resource r_mem_struct;
 	struct spi_master *master;
@@ -74,8 +75,8 @@  static int __init xilinx_spi_of_probe(struct of_device *ofdev,
 		dev_warn(&ofdev->dev, "no 'xlnx,num-ss-bits' property\n");
 		return -EINVAL;
 	}
-	ofdev->dev.platform_data->num_chipselect = *prop;
-	ofdev->dev.platform_data->bits_per_word = 8;
+	pdata->num_chipselect = *prop;
+	pdata->bits_per_word = 8;
 	master = xilinx_spi_init(&ofdev->dev, r_mem, r_irq->start, -1);
 	if (!master)
 		return -ENODEV;