linux-mips
[Top] [All Lists]

Re: [PATCH] TXx9 SPI controller driver (take 2)

To: david-b@pacbell.net
Subject: Re: [PATCH] TXx9 SPI controller driver (take 2)
From: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Date: Mon, 02 Jul 2007 23:00:50 +0900 (JST)
Cc: linux-mips@linux-mips.org, ralf@linux-mips.org, sshtylyov@ru.mvista.com, mlachwani@mvista.com, spi-devel-general@lists.sourceforge.net
In-reply-to: <200706300953.20156.david-b@pacbell.net>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <20070627.222458.27955527.anemo@mba.ocn.ne.jp> <200706300953.20156.david-b@pacbell.net>
Sender: linux-mips-bounce@linux-mips.org
On Sat, 30 Jun 2007 09:53:19 -0700, David Brownell <david-b@pacbell.net> wrote:
> > +   txx9spi_cs_func(spi, c, 0, 1000000000 / 2 / spi->max_speed_hz);
> 
> You still use this confusing A/2/B syntax.  Please
> rewrite that using one "/" and one "*".  (And there
> is similar usage elsewhere.)

Replaced with "(NSEC_PER_SEC / 2) / spi->max_speed_hz".

> These checks here should be in txx9_spi_transfer(), where
> returning EINVAL will do some good.  The single caller to
> this routine doesn't even look at its return value ... and
> returning without issuing the message's completion callback
> is just a bug.

Oh it was really a bug.  Fixed.

> That speed check is wrong.  There's no reason two transfers
> shouldn't have different speeds ... e.g. flash chips often
> have speed limits in certain bulk reads, which don't apply
> to other operations.

Done.

> Also, you can't replace per-transfer speed checks with one
> for the overall message... each transfer could have a
> very different speed.

Done.  Now per-transfer speed_hz and bits_per_word are really supported.

> Here's where the (corrected) checks for each spi_transfer in the
> message belong:  if the message is invalid, don't even queue it,
> just return -EINVAL.

Done.

> > +   if (clk_enable(c->clk)) {
> 
> Minor comment:  if power management is a concern, you might
> consider leaving the clock disabled except when transfers
> are active or you're accessing controller registers.  On
> most chips, leaving a clock enabled all the time (like this)
> means power is needlessly consumed.  (This isn't wrong, just
> sub-optimal in terms of power reduction.)

Well, I did not change this since we can not really stop the
"spi-baseclk" anyway.  But thank you for comment.

> > +   master->num_chipselect = 0;     /* unlimited: any GPIO numbers */
> 
> No, actually it means "no chipselects" as I said before;
> the fact that this works right now is a bug that will be
> fixed at some point.  INT_MAX would allow any GPIO.

Done.  I chose "(u16)INT_MAX" to silence a sparse warning.

> ... almost mergeable!

Let's go take3 !

---
Atsushi Nemoto

<Prev in Thread] Current Thread [Next in Thread>