* Re: mISDN still breaking the allmodconfig build...
From: Marcel Holtmann @ 2008-07-28 1:13 UTC (permalink / raw)
To: David Miller; +Cc: sfr, kkeil, linux-kernel, linuxppc-dev, akpm, torvalds
In-Reply-To: <20080727.180736.74131389.davem@davemloft.net>
Hi Dave,
>>> More fallout from the premature mISDN driver merge:
>>>
>>> drivers/isdn/hardware/mISDN/hfcmulti.c:5255:2: error: #error "not
>>> running on big endian machines now"
>>
>> is that only the HFC driver or the whole mISDN stack?
>>
>> I know that the two old ISDN stacks where really bad on big endian,
>> but my assumption was that we did sort this out in the end.
>
> One of the two mISDN drivers uses the deprecated virt_to_bus()
> interface for handling DMA addresses (that doesn't even work on many
> x86 systems these days) and the other mISDN driver gives the above
> big-endian compile time error.
>
> In short, this driver was not ready for merging at all.
I am not defending it and agree that this driver should have had at
least one test run in linux-next. However mISDN is a whole ISDN stack.
So does mISDN has an issue too or do we only have a really broken
driver. Karsten?
Regards
Marcel
^ permalink raw reply
* Re: gigantci pages patches
From: David Gibson @ 2008-07-28 1:07 UTC (permalink / raw)
To: Jon Tollefson; +Cc: npiggin, Stephen Rothwell, ppc-dev, paulus, Andrew Morton
In-Reply-To: <4886418C.9050704@linux.vnet.ibm.com>
On Tue, Jul 22, 2008 at 03:22:36PM -0500, Jon Tollefson wrote:
> David Gibson wrote:
> > On Fri, Jul 11, 2008 at 05:45:15PM +1000, Stephen Rothwell wrote:
> >
> >> Hi all,
> >>
> >> Could people take one last look at these patches and if there are no
> >> issues, please send Ack-bys to Andrew who will push them to Linus for
> >> 2.6.27.
> >>
> >> [PATCH 1/6 v2] allow arch specific function for allocating gigantic pages
> >> http://patchwork.ozlabs.org/linuxppc/patch?&id=18437
> >> Patch: [PATCH 2/6 v2] powerpc: function for allocating gigantic pages
> >> http://patchwork.ozlabs.org/linuxppc/patch?&id=18438
> >> Patch: [PATCH 3/6 v2] powerpc: scan device tree and save gigantic page locations
> >> http://patchwork.ozlabs.org/linuxppc/patch?&id=18439
> >> Patch: [PATCH 4/6 v2] powerpc: define page support for 16G pages
> >> http://patchwork.ozlabs.org/linuxppc/patch?&id=18440
> >> Patch: [PATCH 5/6 v2] check for overflow
> >> http://patchwork.ozlabs.org/linuxppc/patch?&id=18441
> >> Patch: [PATCH 6/6] powerpc: support multiple huge page sizes
> >> http://patchwork.ozlabs.org/linuxppc/patch?&id=18442
> >>
> >
> > Sorry, I should have looked at these properly when they went past in
> > May, but obviously I missed them.
> >
> > They mostly look ok. I'm a bit confused on 2/6 though - it seems the
> > new powerpc alloc_bootmem_huge_page() function is specific to the 16G
> > gigantic pages. But can't that function also get called for the
> > normal 16M hugepages depending on how the hugepage pool is
> > initialized.
> >
> > Or am I missing something (wouldn't surprise me given my brain's
> > sluggishness today)?
> >
> The alloc_bootmem_huge_page() function is only called for pages >=
> MAX_ORDER. The 16M pages are always allocated within the generic
> hugetlbfs code with alloc_pages_node().
Ah, ok. Well, in that case:
Acked-by: David Gibson <dwg@au1.ibm.com>
for the series.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: mISDN still breaking the allmodconfig build...
From: David Miller @ 2008-07-28 1:07 UTC (permalink / raw)
To: marcel; +Cc: sfr, kkeil, linux-kernel, linuxppc-dev, akpm, torvalds
In-Reply-To: <93120C4B-D2F4-4479-806B-2141AC3DC607@holtmann.org>
From: Marcel Holtmann <marcel@holtmann.org>
Date: Mon, 28 Jul 2008 03:03:04 +0200
> > More fallout from the premature mISDN driver merge:
> >
> > drivers/isdn/hardware/mISDN/hfcmulti.c:5255:2: error: #error "not
> > running on big endian machines now"
>
> is that only the HFC driver or the whole mISDN stack?
>
> I know that the two old ISDN stacks where really bad on big endian,
> but my assumption was that we did sort this out in the end.
One of the two mISDN drivers uses the deprecated virt_to_bus()
interface for handling DMA addresses (that doesn't even work on many
x86 systems these days) and the other mISDN driver gives the above
big-endian compile time error.
In short, this driver was not ready for merging at all.
^ permalink raw reply
* Re: mISDN still breaking the allmodconfig build...
From: Sean MacLennan @ 2008-07-28 0:48 UTC (permalink / raw)
To: benh; +Cc: sfr, kkeil, linux-kernel, linuxppc-dev, akpm, torvalds,
David Miller
In-Reply-To: <1217204022.11188.139.camel@pasglop>
On Mon, 28 Jul 2008 10:13:42 +1000
"Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote:
> On Sun, 2008-07-27 at 17:02 -0700, David Miller wrote:
> > More fallout from the premature mISDN driver merge:
> >
> > drivers/isdn/hardware/mISDN/hfcmulti.c:5255:2: error: #error "not
> > running on big endian machines now"
>
> Lovely...
mISDN is notoriously bad on big endian machines.
Cheers,
Sean
^ permalink raw reply
* Re: [PATCH] powerpc/ibmveth: fix multiple errors with dma_mapping_error conversion
From: Stephen Rothwell @ 2008-07-28 0:32 UTC (permalink / raw)
To: Andrew Morton; +Cc: Jeff Garzik, LKML, FUJITA Tomonori, ppc-dev, Paul Mackerras
In-Reply-To: <20080727122414.cac4e336.akpm@linux-foundation.org>
[-- Attachment #1: Type: text/plain, Size: 882 bytes --]
Hi Andrew,
On Sun, 27 Jul 2008 12:24:14 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 28 Jul 2008 02:14:24 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> > The addition of an argument to dma_mapping_error() in commit
> > 8d8bb39b9eba32dd70e87fd5ad5c5dd4ba118e06 "dma-mapping: add the device
> > argument to dma_mapping_error()" left a bit of fallout:
>
> Yeah, sorry, that patch was a horror - I fixed it perhaps ten times. I
> think people were madly adding new dma_mapping_error() calls while we
> were trying to maintain it as well.
>
> It should have been dome as a two-stage conversion.
Yes, indeed (my new favourite word :-))
I hope that we all can discuss procedures for API changes at the Kernel
Summit ...
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] isdn: mISDN HFC PCI support depends on virt_to_bus()
From: Stephen Rothwell @ 2008-07-28 0:23 UTC (permalink / raw)
To: Sean MacLennan; +Cc: ppc-dev, Andrew Morton, Linus, LKML, Karsten Keil
In-Reply-To: <20080727133918.72cbb127@lappy.seanm.ca>
[-- Attachment #1: Type: text/plain, Size: 894 bytes --]
Hi Sean,
On Sun, 27 Jul 2008 13:39:18 -0400 Sean MacLennan <smaclennan@pikatech.com> wrote:
>
> On Mon, 28 Jul 2008 02:37:32 +1000
> "Stephen Rothwell" <sfr@canb.auug.org.au> wrote:
>
> > On powerpc (allyesconfig build) we get this error:
> >
> > drivers/isdn/hardware/mISDN/hfcpci.c:1991: error: implicit
> > declaration of function 'virt_to_bus'
>
> When did mISDN make it into the kernel? And which kernel tree is it in?
> I don't see it in the Linux next tree....
Its not in linux-next (will be today). It went straight into Linus' tree
late last week.
> I am very interested since we are trying to get mISDN working on the
> Warp. There was no notice that they got 2.6.26 compiling.
It won't even build on powerpc64, powercp32 should at least build.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] isdn: mISDN HFC PCI support depends on virt_to_bus()
From: Stephen Rothwell @ 2008-07-28 0:20 UTC (permalink / raw)
To: David Miller
Cc: kkeil, rdreier, linux-kernel, linuxppc-dev, akpm, torvalds, alan
In-Reply-To: <20080727.161432.237385384.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 508 bytes --]
Hi Dave,
On Sun, 27 Jul 2008 16:14:32 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
>
> The driver went directly into Linus's tree.
>
> Stephen hit the build problem once he synced linux-next up with
> Linus's current tree.
It didn't even get as far as linux-next, I was just browsing the
bloodbath that is/was Linus' tree
(http://kisskb.ellerman.id.au/kisskb/branch/3/) :-(.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] isdn: mISDN HFC PCI support depends on virt_to_bus()
From: Stephen Rothwell @ 2008-07-28 0:17 UTC (permalink / raw)
To: Sam Ravnborg
Cc: kkeil, Roland Dreier, linux-kernel, linuxppc-dev, akpm, torvalds,
David Miller, alan
In-Reply-To: <20080727204355.GA14057@uranus.ravnborg.org>
[-- Attachment #1: Type: text/plain, Size: 723 bytes --]
Hi Sam,
On Sun, 27 Jul 2008 22:43:55 +0200 Sam Ravnborg <sam@ravnborg.org> wrote:
>
> On Sun, Jul 27, 2008 at 01:33:49PM -0700, Roland Dreier wrote:
> > > IMHO, this driver was really rushed in and that was a huge mistake.
> > > If it had gone through linux-next we could have tidied all of this
> > > stuff up in a less "rushed" manner.
> >
> > ?? This thread *is* about a build failure in linux-next.
> mISDN are in -linus which is part of -next and Stephens point is that if
> we have had mISDN in -next then this would have been fixed before hitting
> -linus.
Dave made that point, not me.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] isdn: mISDN HFC PCI support depends on virt_to_bus()
From: Stephen Rothwell @ 2008-07-28 0:16 UTC (permalink / raw)
To: Roland Dreier
Cc: kkeil, linux-kernel, linuxppc-dev, akpm, torvalds, David Miller,
alan
In-Reply-To: <adamyk35aoi.fsf@cisco.com>
[-- Attachment #1: Type: text/plain, Size: 524 bytes --]
Hi Roland,
On Sun, 27 Jul 2008 13:33:49 -0700 Roland Dreier <rdreier@cisco.com> wrote:
>
> > IMHO, this driver was really rushed in and that was a huge mistake.
> > If it had gone through linux-next we could have tidied all of this
> > stuff up in a less "rushed" manner.
>
> ?? This thread *is* about a build failure in linux-next.
I noticed this in Linus' tree ... I did not mention linux-next.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: [PATCH] isdn: mISDN HFC PCI support depends on virt_to_bus()
From: Stephen Rothwell @ 2008-07-28 0:15 UTC (permalink / raw)
To: Alan Cox; +Cc: ppc-dev, Andrew Morton, Linus, LKML, Karsten Keil
In-Reply-To: <20080727182709.1dac0556@lxorguk.ukuu.org.uk>
[-- Attachment #1: Type: text/plain, Size: 739 bytes --]
On Sun, 27 Jul 2008 18:27:09 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>
> On Mon, 28 Jul 2008 02:37:32 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> > On powerpc (allyesconfig build) we get this error:
> >
> > drivers/isdn/hardware/mISDN/hfcpci.c:1991: error: implicit declaration of function 'virt_to_bus'
> >
> > Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
>
> Can we please not merge new virt_to_bus users and instead fix the actual
> code to use the right functions?
Absolutely. But I currently just want my trees to build (I am a bear of
little brain with simple needs :-))
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: mISDN still breaking the allmodconfig build...
From: Benjamin Herrenschmidt @ 2008-07-28 0:13 UTC (permalink / raw)
To: David Miller; +Cc: sfr, kkeil, linux-kernel, linuxppc-dev, akpm, torvalds
In-Reply-To: <20080727.170235.97056809.davem@davemloft.net>
On Sun, 2008-07-27 at 17:02 -0700, David Miller wrote:
> More fallout from the premature mISDN driver merge:
>
> drivers/isdn/hardware/mISDN/hfcmulti.c:5255:2: error: #error "not running on big endian machines now"
Lovely...
Ben.
^ permalink raw reply
* mISDN still breaking the allmodconfig build...
From: David Miller @ 2008-07-28 0:02 UTC (permalink / raw)
To: linux-kernel; +Cc: sfr, akpm, torvalds, kkeil, linuxppc-dev
More fallout from the premature mISDN driver merge:
drivers/isdn/hardware/mISDN/hfcmulti.c:5255:2: error: #error "not running on big endian machines now"
^ permalink raw reply
* Re: [PATCH] isdn: mISDN HFC PCI support depends on virt_to_bus()
From: David Miller @ 2008-07-27 23:14 UTC (permalink / raw)
To: rdreier; +Cc: sfr, kkeil, linux-kernel, linuxppc-dev, akpm, torvalds, alan
In-Reply-To: <adamyk35aoi.fsf@cisco.com>
From: Roland Dreier <rdreier@cisco.com>
Date: Sun, 27 Jul 2008 13:33:49 -0700
> > IMHO, this driver was really rushed in and that was a huge mistake.
> > If it had gone through linux-next we could have tidied all of this
> > stuff up in a less "rushed" manner.
>
> ?? This thread *is* about a build failure in linux-next.
The driver went directly into Linus's tree.
Stephen hit the build problem once he synced linux-next up with
Linus's current tree.
^ permalink raw reply
* Re: [spi-devel-general] [PATCH 1/4] [SPI] [POWERPC] spi_mpc83xx: handles Freescale MPC8610 as well
From: David Brownell @ 2008-07-27 22:12 UTC (permalink / raw)
To: spi-devel-general
Cc: linux-kernel, linuxppc-dev, Scott Wood, Timur Tabi, Pierre Ossman
In-Reply-To: <20080516190438.GA2758@loki.buserror.net>
On Friday 16 May 2008, Scott Wood wrote:
> On Fri, May 16, 2008 at 08:50:52PM +0400, Anton Vorontsov wrote:
> > config SPI_MPC83xx
> > tristate "Freescale MPC83xx/QUICC Engine SPI controller"
> > - depends on SPI_MASTER && (PPC_83xx || QUICC_ENGINE) && EXPERIMENTAL
> > + depends on SPI_MASTER && (PPC_83xx || QUICC_ENGINE || PPC_86xx) && EXPERIMENTAL
>
> How about "depends on SPI_MASTER && FSL_SOC && EXPERIMENTAL"?
>
> Plus, we should change the option text to something like "MPC8xxx SPI
> controller".
Is there an approved-by-powerpc-folk version of this patch yet?
(Noting that SPI_MASTER is now implicit ...)
^ permalink raw reply
* Re: [PATCH] of: i2c: improve last resort compatible entry selection
From: Jon Smirl @ 2008-07-27 22:00 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev, devicetree-discuss
In-Reply-To: <77a246d166f3eafb4d6a5d899ff86945@kernel.crashing.org>
On 7/27/08, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> >
> > >
> > > > compatible = "atmel,24c32wp", "24c32", "eeprom";
> > > >
> > >
> >
>
>
> >
> > > I know this is just an example; but to keep thinks clear, the second
> > > and third values in this compatible property are completely bogus (for
> > > device trees). The manufacturer prefix needs to be present and
> > > 'eeprom' is far to vague.
> > >
> >
> > Isn't 24c32 a generic, cross manufacturer term used for these devices?
> >
>
> Sure it is. But "compatible" values are a global namespace so care
> needs to be taken not to cause collisions. One mechanism for that
> is to use vendor prefixes (and that just shifts the problem so it
> is less global); another is to choose good names that have a lower
> chance to collide with the name for another device. And the most
> important way to prevent collisions is to write up a binding, so
> everyone knows you have claimed that name. It still needs to be
> a good name, of course.
>
>
> > What if I have a socket and use a different vendor's chip each week?
> >
>
> You use sockets for your seeproms? Wow :-) But yes, it shouldn't
> be necessary to put the exact make of the device in the device
> tree, for such generic devices. It certainly doesn't hurt to do
> so though (if the exact model is known).
>
> A reasonable "compatible" value would be something like
> "serial-eeprom-24c32".
> You can go a little bit more generic than that, if you write up in
> your binding how the driver should figure out the device size and
> the protocol used.
Matching on "serial-eeprom-24c32" requires me to convince the at24
authors to add that string as an alias binding for their driver. How
about "serial-eeprom,24c32" or "generic,24x32"?
>
> > eeprom is the vague Linuxism that at24 is attempting to correct.
> > eeprom just goes and searches for anything resembling an eeprom. It
> > will trigger on chips that aren't eeproms.
> >
>
> Yeah. And no driver should need to probe _anything_ if it has a
> device tree node describing the device -- certainly it shouldn't
> probe for what kind of device it is!
>
>
> Segher
>
>
--
Jon Smirl
jonsmirl@gmail.com
^ permalink raw reply
* Re: [PATCH] of: i2c: improve last resort compatible entry selection
From: Segher Boessenkool @ 2008-07-27 21:52 UTC (permalink / raw)
To: Jon Smirl; +Cc: linuxppc-dev, devicetree-discuss
In-Reply-To: <9e4733910807270721h5777bb80u9f3d25413a62883c@mail.gmail.com>
>>> compatible = "atmel,24c32wp", "24c32", "eeprom";
>> I know this is just an example; but to keep thinks clear, the second
>> and third values in this compatible property are completely bogus
>> (for
>> device trees). The manufacturer prefix needs to be present and
>> 'eeprom' is far to vague.
>
> Isn't 24c32 a generic, cross manufacturer term used for these devices?
Sure it is. But "compatible" values are a global namespace so care
needs to be taken not to cause collisions. One mechanism for that
is to use vendor prefixes (and that just shifts the problem so it
is less global); another is to choose good names that have a lower
chance to collide with the name for another device. And the most
important way to prevent collisions is to write up a binding, so
everyone knows you have claimed that name. It still needs to be
a good name, of course.
> What if I have a socket and use a different vendor's chip each week?
You use sockets for your seeproms? Wow :-) But yes, it shouldn't
be necessary to put the exact make of the device in the device
tree, for such generic devices. It certainly doesn't hurt to do
so though (if the exact model is known).
A reasonable "compatible" value would be something like
"serial-eeprom-24c32".
You can go a little bit more generic than that, if you write up in
your binding how the driver should figure out the device size and
the protocol used.
> eeprom is the vague Linuxism that at24 is attempting to correct.
> eeprom just goes and searches for anything resembling an eeprom. It
> will trigger on chips that aren't eeproms.
Yeah. And no driver should need to probe _anything_ if it has a
device tree node describing the device -- certainly it shouldn't
probe for what kind of device it is!
Segher
^ permalink raw reply
* Re: [PATCH v3 0/4 REPOST] OF infrastructure for SPI devices
From: David Brownell @ 2008-07-27 21:49 UTC (permalink / raw)
To: spi-devel-general; +Cc: linuxppc-dev, akpm, linux-kernel
In-Reply-To: <20080725072549.8485.90723.stgit@trillian.secretlab.ca>
On Friday 25 July 2008, Grant Likely wrote:
> I don't know what to do with these patches. I'd really like to see them
> in .27, and now that akpm has cleared his queue, the prerequisite patch
> has been merged so they are ready to go in. However, even though there
> has been favourable reception on the SPI and linuxppc lists for these
> changes I don't have any acks from anybody yet.
>
> David, can you please reply on if you are okay with these patches
> getting merged? If so, do you want me to merge them via my tree, or
> do you want to pick them up?
OK ... to recap, #1 and #3 are OF-specific, I'll be agnostic.
If other OF folk think it's OK, great.
Some cleanup was needed for #2, but it was basically ok ...
I'd like to see the final version, and if it goes via an
OF push that's OK by me. (Though I'd like to see that change
make it into 2.6.27 regardless, so let me know.)
And #4 isn't quite cooked yet.
^ permalink raw reply
* Re: Please pull mpc52xx-next
From: Grant Likely @ 2008-07-27 21:48 UTC (permalink / raw)
To: Benjamin Herrenschmidt, linuxppc-dev, Paul Mackerras
In-Reply-To: <fa686aa40807252100q299f87d7xc2101869a45704bb@mail.gmail.com>
On Fri, Jul 25, 2008 at 10:00 PM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> Hey Ben, here are a few more patches for .27. I would have had this
> stuff in earlier, but they depended on another patch that I didn't
> feel like I should push that was in Andrew's queue.
>
> Grant Likely (4):
> of: adapt of_find_i2c_driver() to be usable by SPI also
> spi: split up spi_new_device() to allow two stage registration.
> spi: Add OF binding support for SPI busses
> powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver
Oops, I shouldn't have merged this last one. Please don't pull this
tree yet. I'll fix it up this evening.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH v3 4/4] powerpc/mpc5200: Add mpc5200-spi (non-PSC) device driver
From: David Brownell @ 2008-07-27 21:41 UTC (permalink / raw)
To: Grant Likely; +Cc: spi-devel-general, akpm, linux-kernel, linuxppc-dev
In-Reply-To: <20080725073326.8485.99210.stgit@trillian.secretlab.ca>
On Friday 25 July 2008, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
>
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
It'd be a bit more clear if you said dedicated SPI "controller";
"device" sounds like it's a "struct spi_device".
Capsule summary: fault handling needs work. (Details below.)
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>
> drivers/spi/Kconfig | 8 +
> drivers/spi/Makefile | 1
> drivers/spi/mpc52xx_spi.c | 595 +++++++++++++++++++++++++++++++++++++++
> include/linux/spi/mpc52xx_spi.h | 10 +
> 4 files changed, 614 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 2303521..68e4a4a 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -116,6 +116,14 @@ config SPI_LM70_LLP
> which interfaces to an LM70 temperature sensor using
> a parallel port.
>
> +config SPI_MPC52xx
> + tristate "Freescale MPC52xx SPI (non-PSC) controller support"
> + depends on PPC_MPC52xx && SPI
> + select SPI_MASTER_OF
> + help
> + This drivers supports the MPC52xx SPI controller in master SPI
> + mode.
> +
> config SPI_MPC52xx_PSC
> tristate "Freescale MPC52xx PSC SPI controller"
> depends on PPC_MPC52xx && EXPERIMENTAL
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 7fca043..340b878 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_SPI_PXA2XX) += pxa2xx_spi.o
> obj-$(CONFIG_SPI_OMAP_UWIRE) += omap_uwire.o
> obj-$(CONFIG_SPI_OMAP24XX) += omap2_mcspi.o
> obj-$(CONFIG_SPI_MPC52xx_PSC) += mpc52xx_psc_spi.o
> +obj-$(CONFIG_SPI_MPC52xx) += mpc52xx_spi.o
> obj-$(CONFIG_SPI_MPC83xx) += spi_mpc83xx.o
> obj-$(CONFIG_SPI_S3C24XX_GPIO) += spi_s3c24xx_gpio.o
> obj-$(CONFIG_SPI_S3C24XX) += spi_s3c24xx.o
> diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
> new file mode 100644
> index 0000000..453690f
> --- /dev/null
> +++ b/drivers/spi/mpc52xx_spi.c
> @@ -0,0 +1,595 @@
> +/*
> + * MPC52xx SPI master driver.
> + * Copyright (C) 2008 Secret Lab Technologies Ltd.
> + *
> + * This is the driver for the MPC5200's dedicated SPI device (not for a
> + * PSC in SPI mode)
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/of_platform.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/mpc52xx_spi.h>
> +#include <linux/of_spi.h>
> +#include <linux/io.h>
> +#include <asm/time.h>
> +#include <asm/mpc52xx.h>
> +
> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
> +MODULE_LICENSE("GPL");
> +
> +/* Register offsets */
> +#define SPI_CTRL1 0x00
> +#define SPI_CTRL1_SPIE (1 << 7)
> +#define SPI_CTRL1_SPE (1 << 6)
> +#define SPI_CTRL1_MSTR (1 << 4)
> +#define SPI_CTRL1_CPOL (1 << 3)
> +#define SPI_CTRL1_CPHA (1 << 2)
> +#define SPI_CTRL1_SSOE (1 << 1)
> +#define SPI_CTRL1_LSBFE (1 << 0)
> +
> +#define SPI_CTRL2 0x01
> +#define SPI_BRR 0x04
> +
> +#define SPI_STATUS 0x05
> +#define SPI_STATUS_SPIF (1 << 7)
> +#define SPI_STATUS_WCOL (1 << 6)
> +#define SPI_STATUS_MODF (1 << 4)
> +
> +#define SPI_DATA 0x09
> +#define SPI_PORTDATA 0x0d
> +#define SPI_DATADIR 0x10
> +
> +/* FSM state return values */
> +#define FSM_STOP 0
> +#define FSM_POLL 1
> +#define FSM_CONTINUE 2
> +
> +/* Driver internal data */
> +struct mpc52xx_spi {
> + struct spi_master *master;
> + u32 sysclk;
> + void __iomem *regs;
> + int irq0; /* MODF irq */
> + int irq1; /* SPIF irq */
> + int ipb_freq;
> +
> + /* Statistics */
> + int msg_count;
> + int wcol_count;
> + int wcol_ticks;
> + u32 wcol_tx_timestamp;
> + int modf_count;
> + int byte_count;
> +
> + /* Hooks for platform modification of behaviour */
> + void (*premessage)(struct spi_message *m, void *context);
> + void *premessage_context;
> +
> + struct list_head queue; /* queue of pending messages */
> + spinlock_t lock;
> + struct work_struct work;
> +
> +
> + /* Details of current transfer (length, and buffer pointers) */
> + struct spi_message *message; /* current message */
> + struct spi_transfer *transfer; /* current transfer */
> + int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
> + int len;
> + int timestamp;
> + u8 *rx_buf;
> + const u8 *tx_buf;
> + int cs_change;
> +};
> +
> +/*
> + * CS control function
> + */
> +static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
> +{
> + if (value)
> + writeb(0, ms->regs + SPI_PORTDATA); /* Assert SS pin */
> + else
> + writeb(0x08, ms->regs + SPI_PORTDATA); /* Deassert SS pin */
> +}
> +
> +/*
> + * Start a new transfer. This is called both by the idle state
> + * for the first transfer in a message, and by the wait state when the
> + * previous transfer in a message is complete.
> + */
> +static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
> +{
> + ms->rx_buf = ms->transfer->rx_buf;
> + ms->tx_buf = ms->transfer->tx_buf;
> + ms->len = ms->transfer->len;
> +
> + /* Activate the chip select */
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 1);
> + ms->cs_change = ms->transfer->cs_change;
> +
> + /* Write out the first byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + writeb(*ms->tx_buf++, ms->regs + SPI_DATA);
> + else
> + writeb(0, ms->regs + SPI_DATA);
> +}
> +
> +/* Forward declaration of state handlers */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data);
> +
> +/*
> + * IDLE state
> + *
> + * No transfers are in progress; if another transfer is pending then retrieve
> + * it and kick it off. Otherwise, stop processing the state machine
> + */
> +static int
> +mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + struct spi_message *m;
> + struct spi_device *spi;
> + int spr, sppr;
> + u8 ctrl1;
> +
> + if (status && (irq != NO_IRQ))
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + /* Check if there is another transfer waiting */
> + if (list_empty(&ms->queue))
> + return FSM_STOP;
As Daniel noted: the queue is protected by the spinlock,
so grab that first. And you said you'd fix that.
> +
> + /* Get the next message */
> + spin_lock(&ms->lock);
> +
> + /* Call the pre-message hook with a pointer to the next
> + * message. The pre-message hook may enqueue a new message for
> + * changing the chip select value to the head of the queue */
> + m = list_first_entry(&ms->queue, struct spi_message, queue);
I don't quite see the point of this pre-message extension;
the kerneldoc there is kind of opaque. How could it queue a
new message that would affect the head of the queue?? The
relevant data structures aren't even visible to that function!
That said:
> + if (ms->premessage)
> + ms->premessage(m, ms->premessage_context);
> +
> + /* reget the head of the queue (the premessage hook may have enqueued
> + * something before it.) and drop the spinlock */
> + ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
if (ms->premessage) {
hook();
ms->message = ...
} else
ms->message = m;
... would be more clear to me, at least if I could see a way
for that "premessage hook" to modify the queue in any way other
than calling spi_transfer() to *append* an entry.
Also, it'd be more clear to have this function use "m" for its
working state not "ms->message" ... and at least some versions
of GCC would generate much better code that way, since they
wouldn't need to reload the register.
> + list_del_init(&ms->message->queue);
> + spin_unlock(&ms->lock);
> +
> + /* Setup the controller parameters */
> + ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
> + spi = ms->message->spi;
> + if (spi->mode & SPI_CPHA)
> + ctrl1 |= SPI_CTRL1_CPHA;
> + if (spi->mode & SPI_CPOL)
> + ctrl1 |= SPI_CTRL1_CPOL;
> + if (spi->mode & SPI_LSB_FIRST)
> + ctrl1 |= SPI_CTRL1_LSBFE;
> + writeb(ctrl1, ms->regs + SPI_CTRL1);
> +
> + /* Setup the controller speed */
> + /* minimum divider is '2'. Also, add '1' to force rounding up. */
> + sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
> + spr = 0;
> + if (sppr < 1)
> + sppr = 1;
> + while (((sppr - 1) & ~0x7) != 0) {
> + sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
> + spr++;
> + }
> + sppr--; /* sppr quantity in register is offset by 1 */
> + if (spr > 7) {
> + /* Don't overrun limits of SPI baudrate register */
> + spr = 7;
> + sppr = 7;
So here you're setting the SPI clock rate faster than the spi_device
says it can handle? Bad idea! There should be an error report. In
this case, it's best done in the setup() callback -- it could just
compute and save the SPI_BRR value in chip-specific data -- so that
you never get errors at this point.
> + }
> + writeb(sppr << 4 | spr, ms->regs + SPI_BRR); /* Set speed */
It'd be better to have that "set speed" logic be a subroutine, so
that you can use it for both per-message setup and for per-transfer
overrides.
In a similar vein, you're ignoring spi->bits_per_word completely...
looks like you're assuming it's always eight.
> +
> + ms->cs_change = 1;
> + ms->transfer = container_of(ms->message->transfers.next,
> + struct spi_transfer, transfer_list);
> +
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> +
> +#if defined(VERBOSE_DEBUG)
> + dev_info(&ms->master->dev, "msg:%p, max_speed:%i, brr:%.2x\n",
So just make this dev_vdbg() ... not #ifdef + dev_info().
> + ms->message, ms->message->spi->max_speed_hz,
> + readb(ms->regs + SPI_BRR));
> +#endif
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * TRANSFER state
> + *
> + * In the middle of a transfer. If the SPI core has completed processing
> + * a byte, then read out the received data and write out the next byte
> + * (unless this transfer is finished; in which case go on to the wait
> + * state)
> + */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> + u8 status, u8 data)
> +{
> + if (!status)
> + return ms->irq0 == NO_IRQ ? FSM_POLL : FSM_STOP;
> +
> + if (status & SPI_STATUS_WCOL) {
> + /* The SPI device is stoopid. At slower speeds, it may raise
> + * the SPIF flag before the state machine is actually finished.
> + * which causes a collision (internal to the state machine
> + * only). The manual recommends inserting a delay between
> + * receving the interrupt and sending the next byte, but
> + * it can also be worked around simply by retrying the
> + * transfer which is what we do here. */
> + ms->wcol_count++;
> + ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
> + ms->wcol_tx_timestamp = get_tbl();
> + data = 0;
> + if (ms->tx_buf)
> + data = *(ms->tx_buf-1);
> + writeb(data, ms->regs + SPI_DATA); /* try again */
> + return FSM_CONTINUE;
> + } else if (status & SPI_STATUS_MODF) {
> + ms->modf_count++;
> + dev_err(&ms->master->dev, "mod fault\n");
Is this "mode fault"? A "mod fault" would be one of the
weak episodes of "The Mod Squad". ;)
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = -EIO;
> + if (ms->message->complete)
> + ms->message->complete(ms->message->context);
All messages must have completion functions; don't check.
And drop the spinlock before calling the completion, since
it's normal for such functions to resubmit ... and hence
re-enter this driver.
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* Read data out of the spi device */
> + ms->byte_count++;
> + if (ms->rx_buf)
> + *ms->rx_buf++ = data;
> +
> + /* Is the transfer complete? */
> + ms->len--;
> + if (ms->len == 0) {
> + ms->timestamp = get_tbl();
> + ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
> + ms->state = mpc52xx_spi_fsmstate_wait;
> + return FSM_CONTINUE;
> + }
> +
> + /* Write out the next byte */
> + ms->wcol_tx_timestamp = get_tbl();
> + if (ms->tx_buf)
> + writeb(*ms->tx_buf++, ms->regs + SPI_DATA);
> + else
> + writeb(0, ms->regs + SPI_DATA);
> +
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * WAIT state
> + *
> + * A transfer has completed; need to wait for the delay period to complete
> + * before starting the next transfer
> + */
> +static int
> +mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> + if (status && irq != NO_IRQ)
> + dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> + status);
> +
> + if (((int)get_tbl()) - ms->timestamp < 0)
> + return FSM_POLL;
> +
> + ms->message->actual_length += ms->transfer->len;
Subtract ms->len though, yes? And abort the transfer if ms->len is
nonzero (controller reported an error or whatever).
> +
> + /* Check if there is another transfer in this message. If there
> + * aren't then deactivate CS, notify sender, and drop back to idle
> + * to start the next message. */
> + if (ms->transfer->transfer_list.next == &ms->message->transfers) {
> + ms->msg_count++;
> + mpc52xx_spi_chipsel(ms, 0);
> + ms->message->status = 0;
> + if (ms->message->complete)
> + ms->message->complete(ms->message->context);
See above about calling completions.
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + return FSM_CONTINUE;
> + }
> +
> + /* There is another transfer; kick it off */
> +
> + if (ms->cs_change)
> + mpc52xx_spi_chipsel(ms, 0);
> +
> + ms->transfer = container_of(ms->transfer->transfer_list.next,
> + struct spi_transfer, transfer_list);
> + mpc52xx_spi_start_transfer(ms);
> + ms->state = mpc52xx_spi_fsmstate_transfer;
> + return FSM_CONTINUE;
> +}
> +
> +/*
> + * IRQ handler
> + */
> +static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
> +{
> + struct mpc52xx_spi *ms = _ms;
> + int rc = FSM_CONTINUE;
> + u8 status, data;
> +
> + while (rc == FSM_CONTINUE) {
> + /* Interrupt cleared by read of STATUS followed by
> + * read of DATA registers*/
> + status = readb(ms->regs + SPI_STATUS);
> + data = readb(ms->regs + SPI_DATA); /* clear status */
> + rc = ms->state(irq, ms, status, data);
> + }
> +
> + if (rc == FSM_POLL)
> + schedule_work(&ms->work);
When the POLL return is from mpc52xx_spi_fsmstate_wait() should
this maybe be a schedule_work_delayed() ... ?
> +
> + return IRQ_HANDLED;
> +}
> +
> +/*
> + * Workqueue method of running the state machine
> + */
> +static void mpc52xx_spi_wq(struct work_struct *work)
> +{
> + struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
> + mpc52xx_spi_irq(NO_IRQ, ms);
> +}
> +
> +/*
> + * spi_master callbacks
> + */
> +
> +static int mpc52xx_spi_setup(struct spi_device *spi)
> +{
> + return 0;
Very unhealthy. This is claiming success for *ALL* settings,
even invalid ones. You should validate things here:
- spi->max_speed_hz is within range of what this supports.
- (spi->mode & ~(SPI_CPOL|SPI_CPHA|SPI_LSB_FIRST)) == 0
... since those are the only mode bits you support
- spi->bits_per_word is valid ... I think that means 8
(or, synonymously, 0), but I can't tell.
- spi->chip_select is valid
> +}
> +
> +static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{
> + struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
> + unsigned long flags;
> +
> + m->actual_length = 0;
> + m->status = -EINPROGRESS;
Before adding this to the queue, I suggest you verify that you
can handle this. Your state machine assumes you did that, but
you aren't ...
Without changing the body of the code you've written already,
I suggest just scanning all the transfers in this message for
bits_per_word or max_speed_hz being nonzero, returning -EINVAL
if any transfer said to not use defaults for that spi_device.
And maybe verify that m->complete is non-null.
> +
> + spin_lock_irqsave(&ms->lock, flags);
> + list_add_tail(&m->queue, &ms->queue);
> + spin_unlock_irqrestore(&ms->lock, flags);
> + schedule_work(&ms->work);
That looks goofy. The workqueue just fakes out an IRQ;
but you don't check whether your state machine is active
before doing that, so a *real* IRQ could come in at the
same time and cause confusion.
Probably better to
if (ms->state == mpc52xx_spi_fsmstate_idle)
schedule_work()
spin_unlock_irqrestore(...)
maybe even just call mpc52xx_spi_fsmstate_idle() directly
instead of punting to a (possibly busy) workqueue.
> +
> + return 0;
> +}
> +
> +/*
> + * Hook to modify premessage hook
Opaque; why does this exist? If it's not well motivated I'd
rather not see it. And if it's well motivated I wonder why
it should be specific to this driver ...
> + */
> +void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
> + void (*hook)(struct spi_message *m,
> + void *context),
> + void *hook_context)
> +{
> + struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> + ms->premessage = hook;
> + ms->premessage_context = hook_context;
These ms->premessage values may be changed while something is
accessing them ... you should hold ms->lock to prevent that.
> +}
> +EXPORT_SYMBOL(mpc52xx_spi_set_premessage_hook);
EXPORT_SYMBOL_GPL?
> +
> +/*
> + * SysFS files
> + */
> +static int
> +*mpc52xx_spi_sysfs_get_counter(struct mpc52xx_spi *ms, const char *name)
> +{
> + if (strcmp(name, "msg_count") == 0)
> + return &ms->msg_count;
> + if (strcmp(name, "byte_count") == 0)
> + return &ms->byte_count;
> + if (strcmp(name, "wcol_count") == 0)
> + return &ms->wcol_count;
> + if (strcmp(name, "wcol_ticks") == 0)
> + return &ms->wcol_ticks;
> + if (strcmp(name, "modf_count") == 0)
> + return &ms->modf_count;
> + return NULL;
> +}
> +
> +static ssize_t mpc52xx_spi_show_count(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct spi_master *master = container_of(dev, struct spi_master, dev);
> + struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> + int *counter;
> +
> + counter = mpc52xx_spi_sysfs_get_counter(ms, attr->attr.name);
> + if (!counter)
> + return sprintf(buf, "error\n");
> + return sprintf(buf, "%d\n", *counter);
> +}
> +
> +static ssize_t mpc52xx_spi_set_count(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct spi_master *master = container_of(dev, struct spi_master, dev);
> + struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> + int *counter;
> + int value = simple_strtoul(buf, NULL, 0);
Checkpatch suggests strict_strtoul(), which would indeed be better...
> +
> + counter = mpc52xx_spi_sysfs_get_counter(ms, attr->attr.name);
> + if (counter)
> + *counter = value;
> + return count;
> +}
> +
> +DEVICE_ATTR(msg_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
> +DEVICE_ATTR(byte_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
> +DEVICE_ATTR(wcol_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
> +DEVICE_ATTR(wcol_ticks, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
> +DEVICE_ATTR(modf_count, 0644, mpc52xx_spi_show_count, mpc52xx_spi_set_count);
> +
> +/*
> + * OF Platform Bus Binding
> + */
> +static int __devinit mpc52xx_spi_of_probe(struct of_device *op,
> + const struct of_device_id *match)
> +{
> + struct spi_master *master;
> + struct mpc52xx_spi *ms;
> + void __iomem *regs;
> + const u32 *prop;
> + int rc, len;
> +
> + /* MMIO registers */
> + dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
> + regs = of_iomap(op->node, 0);
> + if (!regs)
> + return -ENODEV;
> +
> + /* initialize the device */
> + writeb(SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR, regs+SPI_CTRL1);
> + writeb(0x0, regs + SPI_CTRL2);
> + writeb(0xe, regs + SPI_DATADIR); /* Set output pins */
> + writeb(0x8, regs + SPI_PORTDATA); /* Deassert /SS signal */
> +
> + /* Clear the status register and re-read it to check for a MODF
> + * failure. This driver cannot currently handle multiple masters
> + * on the SPI bus. This fault will also occur if the SPI signals
> + * are not connected to any pins (port_config setting) */
> + readb(regs + SPI_STATUS);
> + readb(regs + SPI_DATA);
> + if (readb(regs + SPI_STATUS) & SPI_STATUS_MODF) {
> + dev_err(&op->dev, "mode fault; is port_config correct?\n");
> + return -EIO;
> + }
> +
> + dev_dbg(&op->dev, "allocating spi_master struct\n");
> + master = spi_alloc_master(&op->dev, sizeof *ms);
> + if (!master)
> + return -ENOMEM;
> + master->bus_num = -1;
> + master->num_chipselect = 1;
> + prop = of_get_property(op->node, "num-slaves", &len);
> + if (prop && len >= sizeof(*prop))
> + master->num_chipselect = *prop;
But you don't actually handle more than one chipselect, do you?
Either add full support for them, or rip this out and make sure
that mpc52xx_spi_setup only allows chipselect zero.
> +
> + master->setup = mpc52xx_spi_setup;
> + master->transfer = mpc52xx_spi_transfer;
> + dev_set_drvdata(&op->dev, master);
> +
> + ms = spi_master_get_devdata(master);
> + ms->master = master;
> + ms->regs = regs;
> + ms->irq0 = irq_of_parse_and_map(op->node, 0);
> + ms->irq1 = irq_of_parse_and_map(op->node, 1);
> + ms->state = mpc52xx_spi_fsmstate_idle;
> + ms->ipb_freq = mpc52xx_find_ipb_freq(op->node);
> + spin_lock_init(&ms->lock);
> + INIT_LIST_HEAD(&ms->queue);
> + INIT_WORK(&ms->work, mpc52xx_spi_wq);
> +
> + dev_dbg(&op->dev, "registering spi_master struct\n");
> + rc = spi_register_master(master);
> + if (rc < 0)
> + goto err_register;
> +
> + /* Decide if interrupts can be used */
> + if ((ms->irq0 != NO_IRQ) && (ms->irq1 != NO_IRQ)) {
I understand that NO_IRQ is supposed to vanish everywhere,
so these tests should become "if (ms->irq0 && ms->irq1)".
That said ... with two distinct interrupts, I'm thinking
there must be a better solution than having them do exactly
the same thing.
Plus, a more informative labeling policy would be passing
dev_name(&spi_master->dev) ...
> + rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-modf", ms);
> + rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> + "mpc5200-spi-spiF", ms);
I'm not a fan of that "rc |=" idiom, but I guess it works here.
As noted elsewhere: IRQF_DISABLED will probably be needed.
> + if (rc) {
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> + ms->irq0 = ms->irq1 = NO_IRQ;
> + dev_info(&op->dev, "using polled mode\n");
> + }
> + } else {
> + /* operate in polled mode */
> + ms->irq0 = ms->irq1 = NO_IRQ;
> + dev_info(&op->dev, "using polled mode\n");
irq0 = 0;
irq1 = 0;
> + }
> +
> + /* Create SysFS files */
> + rc = device_create_file(&ms->master->dev, &dev_attr_msg_count);
> + rc |= device_create_file(&ms->master->dev, &dev_attr_byte_count);
> + rc |= device_create_file(&ms->master->dev, &dev_attr_wcol_count);
> + rc |= device_create_file(&ms->master->dev, &dev_attr_wcol_ticks);
> + rc |= device_create_file(&ms->master->dev, &dev_attr_modf_count);
The minimalist in me wonders if those device files should exist,
or at least be moved to debugfs.
> + if (rc)
> + dev_info(&ms->master->dev, "error creating sysfs files\n");
The practical person in me notes that this continues after an otherwise
correct setup, but then returns a failure code from probe(). Which is
clearly a bug ...
> +
> + dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
> +
> + of_register_spi_devices(master, op->node);
> +
> + return rc;
> +
> + err_register:
> + dev_err(&ms->master->dev, "initialization failed\n");
> + spi_master_put(master);
> + return rc;
> +}
> +
> +static void __devexit mpc52xx_spi_of_remove(struct of_device *op)
> +{
> + struct spi_master *master = dev_get_drvdata(&op->dev);
> + struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +
> + device_remove_file(&ms->master->dev, &dev_attr_msg_count);
> + device_remove_file(&ms->master->dev, &dev_attr_byte_count);
> + device_remove_file(&ms->master->dev, &dev_attr_wcol_count);
> + device_remove_file(&ms->master->dev, &dev_attr_wcol_ticks);
> + device_remove_file(&ms->master->dev, &dev_attr_modf_count);
> +
> + free_irq(ms->irq0, ms);
> + free_irq(ms->irq1, ms);
> +
> + spi_unregister_master(master);
> + spi_master_put(master);
> + iounmap(ms->regs);
> +}
> +
> +static struct of_device_id mpc52xx_spi_of_match[] __devinitdata = {
> + { .compatible = "fsl,mpc5200-spi", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, mpc52xx_psc_spi_of_match);
> +
> +static struct of_platform_driver mpc52xx_spi_of_driver = {
> + .owner = THIS_MODULE,
> + .name = "mpc52xx-spi",
> + .match_table = mpc52xx_spi_of_match,
> + .probe = mpc52xx_spi_of_probe,
> + .remove = __exit_p(mpc52xx_spi_of_remove),
> +};
> +
> +static int __init mpc52xx_spi_init(void)
> +{
> + return of_register_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_init(mpc52xx_spi_init);
> +
> +static void __exit mpc52xx_spi_exit(void)
> +{
> + of_unregister_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_exit(mpc52xx_spi_exit);
> +
> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
> new file mode 100644
> index 0000000..d1004cf
> --- /dev/null
> +++ b/include/linux/spi/mpc52xx_spi.h
> @@ -0,0 +1,10 @@
> +
> +#ifndef INCLUDE_MPC5200_SPI_H
> +#define INCLUDE_MPC5200_SPI_H
> +
> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
> + void (*hook)(struct spi_message *m,
> + void *context),
> + void *hook_context);
> +
> +#endif
>
^ permalink raw reply
* Re: [PATCH] powerpc/mpc5200: Fix locking on mpc52xx_spi driver
From: David Brownell @ 2008-07-27 20:45 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev, dwalker, spi-devel-general
In-Reply-To: <20080726062226.10506.65539.stgit@trillian.secretlab.ca>
Applying patch mpc52xx-spi-fix0.patch
patching file drivers/spi/mpc52xx_spi.c
Hunk #1 FAILED at 149.
Hunk #2 succeeded at 154 (offset -7 lines).
Hunk #3 succeeded at 311 (offset -7 lines).
1 out of 3 hunks FAILED -- rejects in file drivers/spi/mpc52xx_spi.c
Patch mpc52xx-spi-fix0.patch does not apply (enforce with -f)
... looks like chunk #1 was against something other than
what you posted, and was partly reversed ..
> +static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
> +{
> + struct mpc52xx_spi *ms = _ms;
> + spin_lock(&ms->lock);
Blank line after variable declaration blocks please ... and
also, since you're not using IRQF_DISABLED when you request
this IRQ, you've got locking trouble here. Either specify
IRQF_DISABLED (my preference) or use spin_lock_irqsave().
> + mpc52xx_spi_fsm_process(irq, ms);
> + spin_unlock(&ms->lock);
> return IRQ_HANDLED;
> }
^ permalink raw reply
* Re: [PATCH] isdn: mISDN HFC PCI support depends on virt_to_bus()
From: Sam Ravnborg @ 2008-07-27 20:43 UTC (permalink / raw)
To: Roland Dreier
Cc: sfr, kkeil, linux-kernel, linuxppc-dev, akpm, torvalds,
David Miller, alan
In-Reply-To: <adamyk35aoi.fsf@cisco.com>
On Sun, Jul 27, 2008 at 01:33:49PM -0700, Roland Dreier wrote:
> > IMHO, this driver was really rushed in and that was a huge mistake.
> > If it had gone through linux-next we could have tidied all of this
> > stuff up in a less "rushed" manner.
>
> ?? This thread *is* about a build failure in linux-next.
mISDN are in -linus which is part of -next and Stephens point is that if
we have had mISDN in -next then this would have been fixed before hitting
-linus. Maybe Karsten should use davem as gateway as he has anyway been
so for other isdn patches lately?
Sam
^ permalink raw reply
* Re: [PATCH] isdn: mISDN HFC PCI support depends on virt_to_bus()
From: Roland Dreier @ 2008-07-27 20:33 UTC (permalink / raw)
To: David Miller; +Cc: sfr, kkeil, linux-kernel, linuxppc-dev, akpm, torvalds, alan
In-Reply-To: <20080727.125604.73857337.davem@davemloft.net>
> IMHO, this driver was really rushed in and that was a huge mistake.
> If it had gone through linux-next we could have tidied all of this
> stuff up in a less "rushed" manner.
?? This thread *is* about a build failure in linux-next.
- R.
^ permalink raw reply
* Re: [PATCH] isdn: mISDN HFC PCI support depends on virt_to_bus()
From: David Miller @ 2008-07-27 19:56 UTC (permalink / raw)
To: alan; +Cc: sfr, kkeil, linux-kernel, linuxppc-dev, akpm, torvalds
In-Reply-To: <20080727182709.1dac0556@lxorguk.ukuu.org.uk>
From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Sun, 27 Jul 2008 18:27:09 +0100
> On Mon, 28 Jul 2008 02:37:32 +1000
> Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> > On powerpc (allyesconfig build) we get this error:
> >
> > drivers/isdn/hardware/mISDN/hfcpci.c:1991: error: implicit declaration of function 'virt_to_bus'
> >
> > Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
>
> Can we please not merge new virt_to_bus users and instead fix the actual
> code to use the right functions?
Yes, please.
IMHO, this driver was really rushed in and that was a huge mistake.
If it had gone through linux-next we could have tidied all of this
stuff up in a less "rushed" manner.
^ permalink raw reply
* Re: [PATCH] powerpc/ibmveth: fix multiple errors with dma_mapping_error conversion
From: Andrew Morton @ 2008-07-27 19:24 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Jeff Garzik, LKML, FUJITA Tomonori, ppc-dev, Paul Mackerras
In-Reply-To: <20080728021424.40cf66bc.sfr@canb.auug.org.au>
On Mon, 28 Jul 2008 02:14:24 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> The addition of an argument to dma_mapping_error() in commit
> 8d8bb39b9eba32dd70e87fd5ad5c5dd4ba118e06 "dma-mapping: add the device
> argument to dma_mapping_error()" left a bit of fallout:
Yeah, sorry, that patch was a horror - I fixed it perhaps ten times. I
think people were madly adding new dma_mapping_error() calls while we
were trying to maintain it as well.
It should have been dome as a two-stage conversion.
^ permalink raw reply
* Re: [PATCH] isdn: mISDN HFC PCI support depends on virt_to_bus()
From: Alan Cox @ 2008-07-27 17:27 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: ppc-dev, Andrew Morton, Linus, LKML, Karsten Keil
In-Reply-To: <20080728023732.090aff83.sfr@canb.auug.org.au>
On Mon, 28 Jul 2008 02:37:32 +1000
Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> On powerpc (allyesconfig build) we get this error:
>
> drivers/isdn/hardware/mISDN/hfcpci.c:1991: error: implicit declaration of function 'virt_to_bus'
>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
Can we please not merge new virt_to_bus users and instead fix the actual
code to use the right functions?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox