diff mbox

[06/10,V2] spi: Add SPI driver for mx233/mx28

Message ID 1341555449-17507-6-git-send-email-marex@denx.de (mailing list archive)
State New, archived
Headers show

Commit Message

Marek Vasut July 6, 2012, 6:17 a.m. UTC
This is slightly reworked version of the SPI driver.
Support for DT has been added and it's been converted
to queued API.

Based on previous attempt by:
Fabio Estevam <fabio.estevam@freescale.com>

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chris Ball <cjb@laptop.org>
Cc: Detlev Zundel <dzu@denx.de>
CC: Dong Aisheng <b29396@freescale.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org>
Cc: Rob Herring <rob.herring@calxeda.com>
CC: Shawn Guo <shawn.guo@linaro.org>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
---
 drivers/spi/Kconfig   |    7 +
 drivers/spi/Makefile  |    1 +
 drivers/spi/spi-mxs.c |  427 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 435 insertions(+)
 create mode 100644 drivers/spi/spi-mxs.c

V2: Fix my patch version management
    Select STMP_DEVICE (thanks Shawn for pointing this out)

Comments

Guenter Roeck July 31, 2012, 8:53 p.m. UTC | #1
On Fri, Jul 06, 2012 at 06:17:25AM -0000, Marek Vasut wrote:
> This is slightly reworked version of the SPI driver.
> Support for DT has been added and it's been converted
> to queued API.
> 
> Based on previous attempt by:
> Fabio Estevam <fabio.estevam@freescale.com>
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chris Ball <cjb@laptop.org>
> Cc: Detlev Zundel <dzu@denx.de>
> CC: Dong Aisheng <b29396@freescale.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> CC: Shawn Guo <shawn.guo@linaro.org>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>
> 
> ---
> drivers/spi/Kconfig   |    7 +
>  drivers/spi/Makefile  |    1 +
>  drivers/spi/spi-mxs.c |  427 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 435 insertions(+)
>  create mode 100644 drivers/spi/spi-mxs.c
> 
> V2: Fix my patch version management
>     Select STMP_DEVICE (thanks Shawn for pointing this out)
> 
Hi,

I have one question about this patch.

[ ... ]

> index 0000000..3c0b1ac
> --- /dev/null
> +++ b/drivers/spi/spi-mxs.c

[ ... ]

> +
> +static int __devinit mxs_spi_probe(struct platform_device *pdev)
> +{

[ ... ]

> +out_host_free:
> +	clk_disable_unprepare(ssp->clk);
> +	spi_master_put(host);
> +	kfree(host);
> +	return ret;
> +}
> +
> +static int __devexit mxs_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_master *host;
> +	struct mxs_spi *spi;
> +	struct mxs_ssp *ssp;
> +
> +	host = platform_get_drvdata(pdev);
> +	spi = spi_master_get_devdata(host);
> +	ssp = &spi->ssp;
> +
> +	spi_unregister_master(host);
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	clk_disable_unprepare(ssp->clk);
> +
> +	spi_master_put(host);
> +	kfree(host);
> +

Is the kfree() here and in the probe function really necessary ? 

Couple of reasons for asking: No other SPI master driver calls it in the remove
function (unless I missed it), most drivers don't call it in the probe
function error path, and if I call it in the remove function in a SPI master
driver I am working on, and load/unload the module several times in a row, I get
a nasty kernel crash.

Thanks,
Guenter
Marek Vasut Aug. 1, 2012, 2:31 a.m. UTC | #2
Dear Guenter Roeck,

[...]

> > +static int __devexit mxs_spi_remove(struct platform_device *pdev)
> > +{
> > +	struct spi_master *host;
> > +	struct mxs_spi *spi;
> > +	struct mxs_ssp *ssp;
> > +
> > +	host = platform_get_drvdata(pdev);
> > +	spi = spi_master_get_devdata(host);
> > +	ssp = &spi->ssp;
> > +
> > +	spi_unregister_master(host);
> > +
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	clk_disable_unprepare(ssp->clk);
> > +
> > +	spi_master_put(host);
> > +	kfree(host);
> > +
> 
> Is the kfree() here and in the probe function really necessary ?

It certainly would seem that way.

> Couple of reasons for asking: No other SPI master driver calls it in the
> remove function (unless I missed it), most drivers don't call it in the
> probe function error path, and if I call it in the remove function in a
> SPI master driver I am working on, and load/unload the module several
> times in a row, I get a nasty kernel crash.

It seems the spi_master class takes care of that kfree() in 
spi.c:spi_master_release() . Good catch, thanks!

> Thanks,
> Guenter

Best regards,
Marek Vasut
Guenter Roeck Aug. 1, 2012, 3:34 a.m. UTC | #3
Hi Marek,

On Wed, Aug 01, 2012 at 04:31:04AM +0200, Marek Vasut wrote:
> Dear Guenter Roeck,
> 
> [...]
> 
> > > +static int __devexit mxs_spi_remove(struct platform_device *pdev)
> > > +{
> > > +	struct spi_master *host;
> > > +	struct mxs_spi *spi;
> > > +	struct mxs_ssp *ssp;
> > > +
> > > +	host = platform_get_drvdata(pdev);
> > > +	spi = spi_master_get_devdata(host);
> > > +	ssp = &spi->ssp;
> > > +
> > > +	spi_unregister_master(host);
> > > +
> > > +	platform_set_drvdata(pdev, NULL);
> > > +
> > > +	clk_disable_unprepare(ssp->clk);
> > > +
> > > +	spi_master_put(host);
> > > +	kfree(host);
> > > +
> > 
> > Is the kfree() here and in the probe function really necessary ?
> 
> It certainly would seem that way.
> 
> > Couple of reasons for asking: No other SPI master driver calls it in the
> > remove function (unless I missed it), most drivers don't call it in the
> > probe function error path, and if I call it in the remove function in a
> > SPI master driver I am working on, and load/unload the module several
> > times in a row, I get a nasty kernel crash.
> 
> It seems the spi_master class takes care of that kfree() in 
> spi.c:spi_master_release() . Good catch, thanks!
> 
Given that, and assuming that spi_master_put() results in the call to
spi_master_release() for both the error case in the probe function and for
the release function, I take it that the kfree() is not needed at all,
and that the documentation for spi_alloc_master() is wrong. Does that sound
reasonable ?

Thanks,
Guenter
Guenter Roeck Aug. 1, 2012, 3:35 a.m. UTC | #4
On Wed, Aug 01, 2012 at 11:53:36AM +0800, Shawn Guo wrote:
> On Wed, Aug 01, 2012 at 04:31:04AM +0200, Marek Vasut wrote:
> > > Couple of reasons for asking: No other SPI master driver calls it in the
> > > remove function (unless I missed it), most drivers don't call it in the
> > > probe function error path, and if I call it in the remove function in a
> > > SPI master driver I am working on, and load/unload the module several
> > > times in a row, I get a nasty kernel crash.
> > 
> > It seems the spi_master class takes care of that kfree() in 
> > spi.c:spi_master_release() . Good catch, thanks!
> > 
> I do not hardware setup to confirm that right away.  When
> spi_master_release will be called exactly?  The time that
> spi_master_put gets called?  I'm trying to understand if the kfree
> is not needed only in remove function, or both probe and remove.
> 
I think the call to spi_master_put() triggers the call to spi_master_release().
If so, kfree() would not be needed at all, and the documentation is wrong.

Thanks,
Guenter
Shawn Guo Aug. 1, 2012, 3:48 a.m. UTC | #5
On Tue, Jul 31, 2012 at 01:53:00PM -0700, Guenter Roeck wrote:
> > +	spi_master_put(host);
> > +	kfree(host);
> > +
> 
> Is the kfree() here and in the probe function really necessary ? 
> 
The following is how the kerneldoc of spi_alloc_master says.

 * The caller is responsible for assigning the bus number and initializing
 * the master's methods before calling spi_register_master(); and (after errors
 * adding the device) calling spi_master_put() and kfree() to prevent a memory
 * leak.

> Couple of reasons for asking: No other SPI master driver calls it in the remove
> function (unless I missed it), most drivers don't call it in the probe
> function error path, and if I call it in the remove function in a SPI master
> driver I am working on, and load/unload the module several times in a row, I get
> a nasty kernel crash.
> 
So sounds like either code or the kerneldoc needs patching?

Regards,
Shawn
Shawn Guo Aug. 1, 2012, 3:53 a.m. UTC | #6
On Wed, Aug 01, 2012 at 04:31:04AM +0200, Marek Vasut wrote:
> > Couple of reasons for asking: No other SPI master driver calls it in the
> > remove function (unless I missed it), most drivers don't call it in the
> > probe function error path, and if I call it in the remove function in a
> > SPI master driver I am working on, and load/unload the module several
> > times in a row, I get a nasty kernel crash.
> 
> It seems the spi_master class takes care of that kfree() in 
> spi.c:spi_master_release() . Good catch, thanks!
> 
I do not hardware setup to confirm that right away.  When
spi_master_release will be called exactly?  The time that
spi_master_put gets called?  I'm trying to understand if the kfree
is not needed only in remove function, or both probe and remove.

Regards,
Shawn
Marek Vasut Aug. 1, 2012, 4:48 a.m. UTC | #7
Dear Shawn Guo,

> On Wed, Aug 01, 2012 at 04:31:04AM +0200, Marek Vasut wrote:
> > > Couple of reasons for asking: No other SPI master driver calls it in
> > > the remove function (unless I missed it), most drivers don't call it
> > > in the probe function error path, and if I call it in the remove
> > > function in a SPI master driver I am working on, and load/unload the
> > > module several times in a row, I get a nasty kernel crash.
> > 
> > It seems the spi_master class takes care of that kfree() in
> > spi.c:spi_master_release() . Good catch, thanks!
> 
> I do not hardware setup to confirm that right away.  When
> spi_master_release will be called exactly?  The time that
> spi_master_put gets called?  I'm trying to understand if the kfree
> is not needed only in remove function, or both probe and remove.

I checked, it's called in both cases ... (if .probe() crashes, release() is 
called, so kfree() is unneeded)

> Regards,
> Shawn

Best regards,
Marek Vasut
Shawn Guo Aug. 1, 2012, 4:50 a.m. UTC | #8
On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
> I think the call to spi_master_put() triggers the call to spi_master_release().
> If so, kfree() would not be needed at all, and the documentation is wrong.
> 
Also those drivers calling kfree in probe.

Regards,
Shawn
Marek Vasut Aug. 1, 2012, 5 a.m. UTC | #9
Dear Shawn Guo,

> On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
> > I think the call to spi_master_put() triggers the call to
> > spi_master_release(). If so, kfree() would not be needed at all, and the
> > documentation is wrong.
> 
> Also those drivers calling kfree in probe.

Looks like that to me ...

> Regards,
> Shawn

Best regards,
Marek Vasut
Guenter Roeck Aug. 1, 2012, 5:29 a.m. UTC | #10
On Wed, Aug 01, 2012 at 07:00:54AM +0200, Marek Vasut wrote:
> Dear Shawn Guo,
> 
> > On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
> > > I think the call to spi_master_put() triggers the call to
> > > spi_master_release(). If so, kfree() would not be needed at all, and the
> > > documentation is wrong.
> > 
> > Also those drivers calling kfree in probe.
> 
> Looks like that to me ...
> 
Doesn't seem to be far spread, fortunately. Only spi-davinci.c, spi-imx.c, and
spi-omap2-mcspi.c as far as I can see, plus the misleading comment in spi.c.

Anyone up for writing some patches ? If not, I'll do it.

Thanks,
Guenter
Guenter Roeck Aug. 1, 2012, 5:42 a.m. UTC | #11
On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
> On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
> > Anyone up for writing some patches ? If not, I'll do it.
> > 
> Go ahead.
> 
Ok, will do. It isn't that simple, actually, since at least some of the drivers
also call spi_master_get(), and thus need two calls to spi_master_put() (or a
call to spi_master_put and a call to kfree).

Guenter
Shubhrajyoti Datta Aug. 1, 2012, 5:46 a.m. UTC | #12
On Wed, Aug 1, 2012 at 10:59 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Wed, Aug 01, 2012 at 07:00:54AM +0200, Marek Vasut wrote:
>> Dear Shawn Guo,
>>
>> > On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
>> > > I think the call to spi_master_put() triggers the call to
>> > > spi_master_release(). If so, kfree() would not be needed at all, and the
>> > > documentation is wrong.
>> >
>> > Also those drivers calling kfree in probe.
>>
>> Looks like that to me ...
>>
> Doesn't seem to be far spread, fortunately. Only spi-davinci.c, spi-imx.c, and
> spi-omap2-mcspi.c

I have a  omapsdp I could patch spi-omap2-mcspi.c  file thanks for the catch.

>as far as I can see, plus the misleading comment in spi.c.
>
> Anyone up for writing some patches ? If not, I'll do it.
>
> Thanks,
> Guenter
>
> ------------------------------------------------------------------------------
> Live Security Virtual Conference
> Exclusive live event will cover all the ways today's security and
> threat landscape has changed and how IT managers can respond. Discussions
> will include endpoint security, mobile security and the latest in malware
> threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Shawn Guo Aug. 1, 2012, 5:58 a.m. UTC | #13
On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
> Anyone up for writing some patches ? If not, I'll do it.
> 
Go ahead.

Regards,
Shawn
Marek Vasut Aug. 1, 2012, 6:10 a.m. UTC | #14
Dear Shawn Guo,

> On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
> > On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
> > > On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
> > > > Anyone up for writing some patches ? If not, I'll do it.
> > > 
> > > Go ahead.
> > 
> > Ok, will do. It isn't that simple, actually, since at least some of the
> > drivers also call spi_master_get(), and thus need two calls to
> > spi_master_put() (or a call to spi_master_put and a call to kfree).
> 
> Hmm, are you saying that there must be a spi_master_put call matching
> spi_alloc_master?  I think we only need to have spi_master_get and
> spi_master_put matched.

Naw, spi_master_get() does refcounting, spi_alloc_master() doesnt. You don't 
need to match spi_alloc_master() with spi_master_put()

> Regards,
> Shawn

Best regards,
Marek Vasut
Shawn Guo Aug. 1, 2012, 6:28 a.m. UTC | #15
On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
> On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
> > On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
> > > Anyone up for writing some patches ? If not, I'll do it.
> > > 
> > Go ahead.
> > 
> Ok, will do. It isn't that simple, actually, since at least some of the drivers
> also call spi_master_get(), and thus need two calls to spi_master_put() (or a
> call to spi_master_put and a call to kfree).
> 
Hmm, are you saying that there must be a spi_master_put call matching
spi_alloc_master?  I think we only need to have spi_master_get and
spi_master_put matched.

Regards,
Shawn
Guenter Roeck Aug. 1, 2012, 6:33 a.m. UTC | #16
On Wed, Aug 01, 2012 at 02:28:40PM +0800, Shawn Guo wrote:
> On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
> > On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
> > > On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
> > > > Anyone up for writing some patches ? If not, I'll do it.
> > > > 
> > > Go ahead.
> > > 
> > Ok, will do. It isn't that simple, actually, since at least some of the drivers
> > also call spi_master_get(), and thus need two calls to spi_master_put() (or a
> > call to spi_master_put and a call to kfree).
> > 
> Hmm, are you saying that there must be a spi_master_put call matching
> spi_alloc_master?  I think we only need to have spi_master_get and
> spi_master_put matched.
> 
Yes, I think that may be so. Of course, I may be wrong, but ultimately that is
what almost all drivers do in the probe error path. Some of the drivers do it in
the remove path as well, though many don't. I suspect that all drivers using
spi_alloc_master() which do not call spi_master_put() in the remove function may
have a memory leak.

Someone who knows the spi infrastructure better than I should have a closer
look, though. The question is really quite simple: For example, in spi-atmel.c,
how is the allocated master freed in the _remove function ? If it doesn't need
the call to spi_master_put(), why do, for example, spi-stmp.c or spi-mpc52xx.c
call it ?

On the other side, I must admit I am getting more and more confused after
looking into the code. For example, the probe function error path in spi-mpc52xx.c
accesses the master's devdata after the call to spi_master_put(). If
spi_master_put() frees the memory as we think it does, the code would access
freed memory. The same happens in the remove path. And spi_master_put() is not
always called, meaning there is either a memory leak or I am completely confused.

Thanks,
Guenter
Guenter Roeck Aug. 1, 2012, 6:36 a.m. UTC | #17
On Wed, Aug 01, 2012 at 11:16:15AM +0530, Shubhrajyoti Datta wrote:
> On Wed, Aug 1, 2012 at 10:59 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Wed, Aug 01, 2012 at 07:00:54AM +0200, Marek Vasut wrote:
> >> Dear Shawn Guo,
> >>
> >> > On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
> >> > > I think the call to spi_master_put() triggers the call to
> >> > > spi_master_release(). If so, kfree() would not be needed at all, and the
> >> > > documentation is wrong.
> >> >
> >> > Also those drivers calling kfree in probe.
> >>
> >> Looks like that to me ...
> >>
> > Doesn't seem to be far spread, fortunately. Only spi-davinci.c, spi-imx.c, and
> > spi-omap2-mcspi.c
> 
> I have a  omapsdp I could patch spi-omap2-mcspi.c  file thanks for the catch.
>
For that it would be good to determine if there is a memory leak when removing
the driver (I don't see where the memory allocated with spi_alloc_master is
removed).

Thanks,
Guenter
Guenter Roeck Aug. 1, 2012, 6:39 a.m. UTC | #18
On Wed, Aug 01, 2012 at 08:10:37AM +0200, Marek Vasut wrote:
> Dear Shawn Guo,
> 
> > On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
> > > On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
> > > > On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
> > > > > Anyone up for writing some patches ? If not, I'll do it.
> > > > 
> > > > Go ahead.
> > > 
> > > Ok, will do. It isn't that simple, actually, since at least some of the
> > > drivers also call spi_master_get(), and thus need two calls to
> > > spi_master_put() (or a call to spi_master_put and a call to kfree).
> > 
> > Hmm, are you saying that there must be a spi_master_put call matching
> > spi_alloc_master?  I think we only need to have spi_master_get and
> > spi_master_put matched.
> 
> Naw, spi_master_get() does refcounting, spi_alloc_master() doesnt. You don't 
> need to match spi_alloc_master() with spi_master_put()
> 
I must be missing something.  Why do almost all spi drivers call it in the error
path, even if there is no call to spi_master_get ?

Thanks,
Guenter
Marek Vasut Aug. 1, 2012, 6:40 a.m. UTC | #19
Dear Guenter Roeck,

> On Wed, Aug 01, 2012 at 02:28:40PM +0800, Shawn Guo wrote:
> > On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
> > > On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
> > > > On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
> > > > > Anyone up for writing some patches ? If not, I'll do it.
> > > > 
> > > > Go ahead.
> > > 
> > > Ok, will do. It isn't that simple, actually, since at least some of the
> > > drivers also call spi_master_get(), and thus need two calls to
> > > spi_master_put() (or a call to spi_master_put and a call to kfree).
> > 
> > Hmm, are you saying that there must be a spi_master_put call matching
> > spi_alloc_master?  I think we only need to have spi_master_get and
> > spi_master_put matched.
> 
> Yes, I think that may be so. Of course, I may be wrong, but ultimately that
> is what almost all drivers do in the probe error path. Some of the drivers
> do it in the remove path as well, though many don't. I suspect that all
> drivers using spi_alloc_master() which do not call spi_master_put() in the
> remove function may have a memory leak.

CCing Mark.

> Someone who knows the spi infrastructure better than I should have a closer
> look, though. The question is really quite simple: For example, in
> spi-atmel.c, how is the allocated master freed in the _remove function ?
> If it doesn't need the call to spi_master_put(), why do, for example,
> spi-stmp.c or spi-mpc52xx.c call it ?
> 
> On the other side, I must admit I am getting more and more confused after
> looking into the code. For example, the probe function error path in
> spi-mpc52xx.c accesses the master's devdata after the call to
> spi_master_put(). If spi_master_put() frees the memory as we think it
> does, the code would access freed memory. The same happens in the remove
> path. And spi_master_put() is not always called, meaning there is either a
> memory leak or I am completely confused.

I'll poke through the stuff later if you won't get your answers (later == around 
tomorrow)

> Thanks,
> Guenter

Best regards,
Marek Vasut
Marek Vasut Aug. 1, 2012, 6:41 a.m. UTC | #20
Dear Guenter Roeck,

> On Wed, Aug 01, 2012 at 11:16:15AM +0530, Shubhrajyoti Datta wrote:
> > On Wed, Aug 1, 2012 at 10:59 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > > On Wed, Aug 01, 2012 at 07:00:54AM +0200, Marek Vasut wrote:
> > >> Dear Shawn Guo,
> > >> 
> > >> > On Tue, Jul 31, 2012 at 08:35:59PM -0700, Guenter Roeck wrote:
> > >> > > I think the call to spi_master_put() triggers the call to
> > >> > > spi_master_release(). If so, kfree() would not be needed at all,
> > >> > > and the documentation is wrong.
> > >> > 
> > >> > Also those drivers calling kfree in probe.
> > >> 
> > >> Looks like that to me ...
> > > 
> > > Doesn't seem to be far spread, fortunately. Only spi-davinci.c,
> > > spi-imx.c, and spi-omap2-mcspi.c
> > 
> > I have a  omapsdp I could patch spi-omap2-mcspi.c  file thanks for the
> > catch.
> 
> For that it would be good to determine if there is a memory leak when
> removing the driver (I don't see where the memory allocated with
> spi_alloc_master is removed).

When the refcounting for the device reaches 0, it's deallocated. (Aka _put long 
enough and it'll disappear)

> Thanks,
> Guenter

Best regards,
Marek Vasut
Marek Vasut Aug. 1, 2012, 6:45 a.m. UTC | #21
Dear Guenter Roeck,

> On Wed, Aug 01, 2012 at 08:10:37AM +0200, Marek Vasut wrote:
> > Dear Shawn Guo,
> > 
> > > On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
> > > > On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
> > > > > On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
> > > > > > Anyone up for writing some patches ? If not, I'll do it.
> > > > > 
> > > > > Go ahead.
> > > > 
> > > > Ok, will do. It isn't that simple, actually, since at least some of
> > > > the drivers also call spi_master_get(), and thus need two calls to
> > > > spi_master_put() (or a call to spi_master_put and a call to kfree).
> > > 
> > > Hmm, are you saying that there must be a spi_master_put call matching
> > > spi_alloc_master?  I think we only need to have spi_master_get and
> > > spi_master_put matched.
> > 
> > Naw, spi_master_get() does refcounting, spi_alloc_master() doesnt. You
> > don't need to match spi_alloc_master() with spi_master_put()
> 
> I must be missing something.  Why do almost all spi drivers call it in the
> error path, even if there is no call to spi_master_get ?

To push the refcounting to 0, to deallocate the device, I'd say ...

Best regards,
Marek Vasut
Guenter Roeck Aug. 1, 2012, 6:56 a.m. UTC | #22
On Wed, Aug 01, 2012 at 08:45:19AM +0200, Marek Vasut wrote:
> Dear Guenter Roeck,
> 
> > On Wed, Aug 01, 2012 at 08:10:37AM +0200, Marek Vasut wrote:
> > > Dear Shawn Guo,
> > > 
> > > > On Tue, Jul 31, 2012 at 10:42:28PM -0700, Guenter Roeck wrote:
> > > > > On Wed, Aug 01, 2012 at 01:58:56PM +0800, Shawn Guo wrote:
> > > > > > On Tue, Jul 31, 2012 at 10:29:47PM -0700, Guenter Roeck wrote:
> > > > > > > Anyone up for writing some patches ? If not, I'll do it.
> > > > > > 
> > > > > > Go ahead.
> > > > > 
> > > > > Ok, will do. It isn't that simple, actually, since at least some of
> > > > > the drivers also call spi_master_get(), and thus need two calls to
> > > > > spi_master_put() (or a call to spi_master_put and a call to kfree).
> > > > 
> > > > Hmm, are you saying that there must be a spi_master_put call matching
> > > > spi_alloc_master?  I think we only need to have spi_master_get and
> > > > spi_master_put matched.
> > > 
> > > Naw, spi_master_get() does refcounting, spi_alloc_master() doesnt. You
> > > don't need to match spi_alloc_master() with spi_master_put()
> > 
> > I must be missing something.  Why do almost all spi drivers call it in the
> > error path, even if there is no call to spi_master_get ?
> 
> To push the refcounting to 0, to deallocate the device, I'd say ...
> 

Guess we are in violent agreement. The sequence would then either be
	master = spi_alloc_device();
	...
	spi_master_put(master);
or
	master = spi_alloc_device();
	...
	kfree(master);

which makes sense to me. Question still is why most drivers neither call kfree()
nor spi_master_put() in the remove function.

Thanks,
Guenter
Lothar Waßmann Aug. 1, 2012, 7:38 a.m. UTC | #23
Shawn Guo writes:
> On Wed, Aug 01, 2012 at 08:45:19AM +0200, Marek Vasut wrote:
> > > I must be missing something.  Why do almost all spi drivers call it in the
> > > error path, even if there is no call to spi_master_get ?
> > 
> > To push the refcounting to 0, to deallocate the device, I'd say ...
> > 
> It's not going to work if spi_master_put is called without
> spi_master_get being called before that.
> 
spi_alloc_master() calls device_initialize() which resuires a
device_put() (called from spi_master_put()) to free the device.

Thus each call of either spi_alloc_master() or spi_master_get() must
be paired with an spi_master_put() call to free the resources.

The kfree() is taken care of by the spi_master_release() function that
is called once the last reference to the underlying struct device has
been released. Thus kfree() must not be called after
spi_alloc_master().


Lothar Waßmann
Shawn Guo Aug. 1, 2012, 7:40 a.m. UTC | #24
On Wed, Aug 01, 2012 at 08:45:19AM +0200, Marek Vasut wrote:
> > I must be missing something.  Why do almost all spi drivers call it in the
> > error path, even if there is no call to spi_master_get ?
> 
> To push the refcounting to 0, to deallocate the device, I'd say ...
> 
It's not going to work if spi_master_put is called without
spi_master_get being called before that.

Regards,
Shawn
Shawn Guo Aug. 1, 2012, 7:47 a.m. UTC | #25
On Wed, Aug 01, 2012 at 03:40:53PM +0800, Shawn Guo wrote:
> On Wed, Aug 01, 2012 at 08:45:19AM +0200, Marek Vasut wrote:
> > > I must be missing something.  Why do almost all spi drivers call it in the
> > > error path, even if there is no call to spi_master_get ?
> > 
> > To push the refcounting to 0, to deallocate the device, I'd say ...
> > 
> It's not going to work if spi_master_put is called without
> spi_master_get being called before that.
> 
I'm saying that kref_put on a kref that is never called on by kref_get
will not trigger the release.

Regards,
Shawn
Shawn Guo Aug. 1, 2012, 7:50 a.m. UTC | #26
On Tue, Jul 31, 2012 at 11:56:50PM -0700, Guenter Roeck wrote:
> Guess we are in violent agreement. The sequence would then either be
> 	master = spi_alloc_device();

The discussion is around spi_alloc_master rather than spi_alloc_device,
isn't it?

Regards,
Shawn

> 	...
> 	spi_master_put(master);
> or
> 	master = spi_alloc_device();
> 	...
> 	kfree(master);
> 
> which makes sense to me. Question still is why most drivers neither call kfree()
> nor spi_master_put() in the remove function.
Shawn Guo Aug. 1, 2012, 8:20 a.m. UTC | #27
On Wed, Aug 01, 2012 at 09:38:07AM +0200, Lothar Waßmann wrote:
> Shawn Guo writes:
> > On Wed, Aug 01, 2012 at 08:45:19AM +0200, Marek Vasut wrote:
> > > > I must be missing something.  Why do almost all spi drivers call it in the
> > > > error path, even if there is no call to spi_master_get ?
> > > 
> > > To push the refcounting to 0, to deallocate the device, I'd say ...
> > > 
> > It's not going to work if spi_master_put is called without
> > spi_master_get being called before that.
> > 
> spi_alloc_master() calls device_initialize() which resuires a
> device_put() (called from spi_master_put()) to free the device.
> 
Ah, indeed.  I missed that kref_init() will initialize the refcount
to 1.

Thanks,
Shawn
Guenter Roeck Aug. 1, 2012, 2:58 p.m. UTC | #28
On Wed, Aug 01, 2012 at 03:50:12PM +0800, Shawn Guo wrote:
> On Tue, Jul 31, 2012 at 11:56:50PM -0700, Guenter Roeck wrote:
> > Guess we are in violent agreement. The sequence would then either be
> > 	master = spi_alloc_device();
> 
> The discussion is around spi_alloc_master rather than spi_alloc_device,
> isn't it?
> 
Yes, sorry. Too late at night, too tired :(.

Guenter

> Regards,
> Shawn
> 
> > 	...
> > 	spi_master_put(master);
> > or
> > 	master = spi_alloc_device();
> > 	...
> > 	kfree(master);
> > 
> > which makes sense to me. Question still is why most drivers neither call kfree()
> > nor spi_master_put() in the remove function.
> 
>
diff mbox

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 8c8c680..6a1b06c 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -364,6 +364,13 @@  config SPI_STMP3XXX
 	help
 	  SPI driver for Freescale STMP37xx/378x SoC SSP interface
 
+config SPI_MXS
+	tristate "Freescale MXS SPI controller"
+	depends on ARCH_MXS
+	select STMP_DEVICE
+	help
+	  SPI driver for Freescale MXS devices.
+
 config SPI_TEGRA
 	tristate "Nvidia Tegra SPI controller"
 	depends on ARCH_TEGRA && TEGRA_SYSTEM_DMA
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index b5cbab2..e6a2ee4 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -36,6 +36,7 @@  obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
 obj-$(CONFIG_SPI_MPC512x_PSC)		+= spi-mpc512x-psc.o
 obj-$(CONFIG_SPI_MPC52xx_PSC)		+= spi-mpc52xx-psc.o
 obj-$(CONFIG_SPI_MPC52xx)		+= spi-mpc52xx.o
+obj-$(CONFIG_SPI_MXS)			+= spi-mxs.o
 obj-$(CONFIG_SPI_NUC900)		+= spi-nuc900.o
 obj-$(CONFIG_SPI_OC_TINY)		+= spi-oc-tiny.o
 obj-$(CONFIG_SPI_OMAP_UWIRE)		+= spi-omap-uwire.o
diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
new file mode 100644
index 0000000..3c0b1ac
--- /dev/null
+++ b/drivers/spi/spi-mxs.c
@@ -0,0 +1,427 @@ 
+/*
+ * Freescale MXS SPI master driver
+ *
+ * Copyright 2012 DENX Software Engineering, GmbH.
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2008 Embedded Alley Solutions, Inc All Rights Reserved.
+ *
+ * Rework and transition to new API by:
+ * Marek Vasut <marex@denx.de>
+ *
+ * Based on previous attempt by:
+ * Fabio Estevam <fabio.estevam@freescale.com>
+ *
+ * Based on code from U-Boot bootloader by:
+ * Marek Vasut <marex@denx.de>
+ *
+ * Based on spi-stmp.c, which is:
+ * Author: Dmitry Pervushin <dimka@embeddedalley.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/highmem.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/completion.h>
+#include <linux/gpio.h>
+#include <linux/regulator/consumer.h>
+#include <linux/module.h>
+#include <linux/fsl/mxs-dma.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/stmp_device.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/mxs-spi.h>
+
+#define DRIVER_NAME		"mxs-spi"
+
+#define SSP_TIMEOUT		1000	/* 1000 ms */
+
+struct mxs_spi {
+	struct mxs_ssp		ssp;
+};
+
+static int mxs_spi_setup_transfer(struct spi_device *dev,
+				struct spi_transfer *t)
+{
+	struct mxs_spi *spi = spi_master_get_devdata(dev->master);
+	struct mxs_ssp *ssp = &spi->ssp;
+	uint8_t bits_per_word;
+	uint32_t hz = 0;
+
+	bits_per_word = dev->bits_per_word;
+	if (t && t->bits_per_word)
+		bits_per_word = t->bits_per_word;
+
+	if (bits_per_word != 8) {
+		dev_err(&dev->dev, "%s, unsupported bits_per_word=%d\n",
+					__func__, bits_per_word);
+		return -EINVAL;
+	}
+
+	if (dev->max_speed_hz)
+		hz = dev->max_speed_hz;
+	if (t && t->speed_hz)
+		hz = t->speed_hz;
+	if (hz == 0) {
+		dev_err(&dev->dev, "Cannot continue with zero clock\n");
+		return -EINVAL;
+	}
+
+	mxs_ssp_set_clk_rate(ssp, hz);
+
+	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
+		     BF_SSP_CTRL1_WORD_LENGTH
+		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
+		     ((dev->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
+		     ((dev->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
+		     ssp->base + HW_SSP_CTRL1(ssp));
+
+	writel(0x0, ssp->base + HW_SSP_CMD0);
+	writel(0x0, ssp->base + HW_SSP_CMD1);
+
+	return 0;
+}
+
+static void mxs_spi_cleanup(struct spi_device *dev)
+{
+	return;
+}
+
+static int mxs_spi_setup(struct spi_device *dev)
+{
+	int err = 0;
+
+	if (!dev->bits_per_word)
+		dev->bits_per_word = 8;
+
+	if (dev->mode & ~(SPI_CPOL | SPI_CPHA))
+		return -EINVAL;
+
+	err = mxs_spi_setup_transfer(dev, NULL);
+	if (err) {
+		dev_err(&dev->dev,
+			"Failed to setup transfer, error = %d\n", err);
+	}
+
+	return err;
+}
+
+static uint32_t mxs_spi_cs_to_reg(unsigned cs)
+{
+	uint32_t select = 0;
+
+	if (cs & 1)
+		select |= BM_SSP_CTRL0_WAIT_FOR_CMD;
+	if (cs & 2)
+		select |= BM_SSP_CTRL0_WAIT_FOR_IRQ;
+
+	return select;
+}
+
+static void mxs_spi_set_cs(struct mxs_spi *spi, unsigned cs)
+{
+	const uint32_t mask =
+		BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ;
+	uint32_t select;
+	struct mxs_ssp *ssp = &spi->ssp;
+
+	writel(mask, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+	select = mxs_spi_cs_to_reg(cs);
+	writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+}
+
+static inline void mxs_spi_enable(struct mxs_spi *spi)
+{
+	struct mxs_ssp *ssp = &spi->ssp;
+
+	writel(BM_SSP_CTRL0_LOCK_CS,
+		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+	writel(BM_SSP_CTRL0_IGNORE_CRC,
+		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+}
+
+static inline void mxs_spi_disable(struct mxs_spi *spi)
+{
+	struct mxs_ssp *ssp = &spi->ssp;
+
+	writel(BM_SSP_CTRL0_LOCK_CS,
+		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+	writel(BM_SSP_CTRL0_IGNORE_CRC,
+		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+}
+
+static int mxs_ssp_wait(struct mxs_spi *spi, int offset, int mask, bool set)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
+	struct mxs_ssp *ssp = &spi->ssp;
+	uint32_t reg;
+
+	while (1) {
+		reg = readl_relaxed(ssp->base + offset);
+
+		if (set && ((reg & mask) == mask))
+			break;
+
+		if (!set && ((~reg & mask) == mask))
+			break;
+
+		udelay(1);
+
+		if (time_after(jiffies, timeout))
+			return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+static int mxs_spi_txrx_pio(struct mxs_spi *spi, int cs,
+			    unsigned char *buf, int len,
+			    int *first, int *last, int write)
+{
+	struct mxs_ssp *ssp = &spi->ssp;
+
+	if (*first)
+		mxs_spi_enable(spi);
+
+	mxs_spi_set_cs(spi, cs);
+
+	while (len--) {
+		if (*last && len == 0)
+			mxs_spi_disable(spi);
+
+		if (ssp->devid == IMX23_SSP) {
+			writel(BM_SSP_CTRL0_XFER_COUNT,
+				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+			writel(1,
+				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+		} else {
+			writel(1, ssp->base + HW_SSP_XFER_SIZE);
+		}
+
+		if (write)
+			writel(BM_SSP_CTRL0_READ,
+				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+		else
+			writel(BM_SSP_CTRL0_READ,
+				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+
+		writel(BM_SSP_CTRL0_RUN,
+				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+
+		if (mxs_ssp_wait(spi, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN, 1))
+			return -ETIMEDOUT;
+
+		if (write)
+			writel(*buf, ssp->base + HW_SSP_DATA);
+
+		writel(BM_SSP_CTRL0_DATA_XFER,
+			     ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+
+		if (!write) {
+			if (mxs_ssp_wait(spi, HW_SSP_STATUS(ssp),
+						BM_SSP_STATUS_FIFO_EMPTY, 0))
+				return -ETIMEDOUT;
+
+			*buf = (readl(ssp->base + HW_SSP_DATA) & 0xff);
+		}
+
+		if (mxs_ssp_wait(spi, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN, 0))
+			return -ETIMEDOUT;
+
+		buf++;
+	}
+
+	if (len <= 0)
+		return 0;
+
+	return -ETIMEDOUT;
+}
+
+static int mxs_spi_transfer_one(struct spi_master *host, struct spi_message *m)
+{
+	struct mxs_spi *spi = spi_master_get_devdata(host);
+	struct mxs_ssp *ssp = &spi->ssp;
+	int first, last;
+	struct spi_transfer *t, *tmp_t;
+	int status = 0;
+	int cs;
+
+	first = last = 0;
+
+	cs = m->spi->chip_select;
+
+	list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
+
+		mxs_spi_setup_transfer(m->spi, t);
+
+		if (&t->transfer_list == m->transfers.next)
+			first = 1;
+		if (&t->transfer_list == m->transfers.prev)
+			last = 1;
+		if (t->rx_buf && t->tx_buf) {
+			dev_err(ssp->dev,
+				"Cannot send and receive simultaneously\n");
+			return -EINVAL;
+		}
+
+		if (t->tx_buf)
+			status = mxs_spi_txrx_pio(spi, cs, (void *)t->tx_buf,
+					     t->len, &first, &last, 1);
+		if (t->rx_buf)
+			status = mxs_spi_txrx_pio(spi, cs, t->rx_buf,
+					     t->len, &first, &last, 0);
+
+		m->actual_length += t->len;
+		if (status)
+			break;
+
+		first = last = 0;
+	}
+
+	m->status = 0;
+	spi_finalize_current_message(host);
+
+	return status;
+}
+
+static const struct of_device_id mxs_spi_dt_ids[] = {
+	{ .compatible = "fsl,imx23-spi", .data = (void *) IMX23_SSP, },
+	{ .compatible = "fsl,imx28-spi", .data = (void *) IMX28_SSP, },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxs_spi_dt_ids);
+
+static int __devinit mxs_spi_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *of_id =
+			of_match_device(mxs_spi_dt_ids, &pdev->dev);
+	struct device_node *np = pdev->dev.of_node;
+	struct spi_master *host;
+	struct mxs_spi *spi;
+	struct mxs_ssp *ssp;
+	struct resource *iores;
+	struct pinctrl *pinctrl;
+	struct clk *clk;
+	void __iomem *base;
+	int devid;
+	int ret = 0;
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!iores)
+		return -EINVAL;
+
+	base = devm_request_and_ioremap(&pdev->dev, iores);
+	if (!base)
+		return -EADDRNOTAVAIL;
+
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		return PTR_ERR(pinctrl);
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	if (np)
+		devid = (enum mxs_ssp_id) of_id->data;
+	else
+		devid = pdev->id_entry->driver_data;
+
+	host = spi_alloc_master(&pdev->dev, sizeof(*spi));
+	if (!host)
+		return -ENOMEM;
+
+	host->transfer_one_message = mxs_spi_transfer_one;
+	host->setup = mxs_spi_setup;
+	host->cleanup = mxs_spi_cleanup;
+	host->mode_bits = SPI_CPOL | SPI_CPHA;
+	host->num_chipselect = 3;
+	host->dev.of_node = np;
+	host->flags = SPI_MASTER_HALF_DUPLEX;
+
+	spi = spi_master_get_devdata(host);
+	ssp = &spi->ssp;
+	ssp->dev = &pdev->dev;
+	ssp->clk = clk;
+	ssp->base = base;
+	ssp->devid = devid;
+
+	clk_prepare_enable(ssp->clk);
+	ssp->clk_rate = clk_get_rate(ssp->clk) / 1000;
+
+	stmp_reset_block(ssp->base);
+
+	platform_set_drvdata(pdev, host);
+
+	ret = spi_register_master(host);
+	if (ret) {
+		dev_err(&pdev->dev, "Cannot register SPI master, %d\n", ret);
+		goto out_host_free;
+	}
+
+	return 0;
+
+out_host_free:
+	clk_disable_unprepare(ssp->clk);
+	spi_master_put(host);
+	kfree(host);
+	return ret;
+}
+
+static int __devexit mxs_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *host;
+	struct mxs_spi *spi;
+	struct mxs_ssp *ssp;
+
+	host = platform_get_drvdata(pdev);
+	spi = spi_master_get_devdata(host);
+	ssp = &spi->ssp;
+
+	spi_unregister_master(host);
+
+	platform_set_drvdata(pdev, NULL);
+
+	clk_disable_unprepare(ssp->clk);
+
+	spi_master_put(host);
+	kfree(host);
+
+	return 0;
+}
+
+static struct platform_driver mxs_spi_driver = {
+	.probe	= mxs_spi_probe,
+	.remove	= __devexit_p(mxs_spi_remove),
+	.driver	= {
+		.name	= DRIVER_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = mxs_spi_dt_ids,
+	},
+};
+
+module_platform_driver(mxs_spi_driver);
+
+MODULE_AUTHOR("Dmitry Pervushin <dimka@embeddedalley.com>");
+MODULE_DESCRIPTION("MXS SPI master driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:mxs-spi");