From: Wolfram Sang <firstname.lastname@example.org>
To: Grant Likely <email@example.com>
David Brownell <firstname.lastname@example.org>,
Subject: Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
Date: Thu, 18 Jun 2009 16:26:10 +0200 [thread overview]
Message-ID: <20090618142610.GC10629@pengutronix.de> (raw)
[-- Attachment #1.1: Type: text/plain, Size: 2272 bytes --]
> 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.
> >> +
> >> + /* 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
But it won't work if the pin is set to output, no? Are you sure there are no
> >> + /* 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
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]
[-- Attachment #2: Type: text/plain, Size: 150 bytes --]
Linuxppc-dev mailing list
next prev parent reply other threads:[~2009-06-18 14:26 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-06-18 2:55 [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver Grant Likely
2009-06-18 6:58 ` [spi-devel-general] " Wolfram Sang
2009-06-18 13:35 ` Grant Likely
2009-06-18 14:26 ` Wolfram Sang [this message]
2009-07-03 7:01 ` Grant Likely
2009-10-21 13:17 ` Wolfram Sang
2009-10-21 14:45 ` Grant Likely
2009-10-21 17:06 ` Wolfram Sang
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).