Messages in this thread | | | Date | Thu, 18 Jun 2009 16:26:10 +0200 | From | Wolfram Sang <> | Subject | Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver |
| |
Hi Grant,
> The double spaces at the end of sentences are intentional. It is > perhaps a bit quaint and old fashioned, but it is my writing style.
Ah, okay.
> >> + > >> + /* Statistics */ > >> + int msg_count; > >> + int wcol_count; > >> + int wcol_ticks; > >> + u32 wcol_tx_timestamp; > >> + int modf_count; > >> + int byte_count; > > > > Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around > > them will be ugly. Well, I can't make up if this is just overhead or useful > > debugging aid, so no real objection :) > > There used to be a sysfs interface for dumping these, but it was an > ugly misuse. I'd like to leave these in. I still have the sysfs bits > in a private patch and I'm going to rework them for debugfs.
Okay. Maybe a comment stating the future use will be nice.
> > > But I wonder more about the usage of the SS pin and if this chipsel is needed > > at all (sadly I cannot test as I don't have any board with SPI connected to > > that device). You define the SS-pin as output, but do not set the SSOE-bit. > > More, you use the MODF-feature, so the SS-pin should be defined as input? > > According to Table 17.3 in the PM, you have that pin defined as generic purpose > > output. > > That's right. The SS handling by the SPI device is completely > useless, so this driver uses it as a GPIO and asserts it manually.
That definately needs a comment :D (perhaps with some more details if you know them).
> The MODF irq is probably irrelevant, but I'd like to leave it in for > completeness.
But it won't work if the pin is set to output, no? Are you sure there are no side-effects?
> >> + /* initialize the device */ > >> + out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR); > > > > spaces around operator. > > Intentional to keep from spilling past column 80; but I'll move the > missing spaces to around the | operators. I think it is easier to > read that way.
Ah, I remember, we had this topic a while ago :D
Regards,
Wolfram
-- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |