Patchwork [Question] m25p80 driver versus spi clock rate

login
register
mail settings
Submitter David Brownell
Date June 23, 2009, 10:38 p.m.
Message ID <200906231538.40125.david-b@pacbell.net>
Download mbox | patch
Permalink /patch/29098/
State New
Headers show

Comments

David Brownell - June 23, 2009, 10:38 p.m.
On Tuesday 23 June 2009, Steven A. Falco wrote:
> m25p80 spi0.0: invalid bits-per-word (0)
> 
> This message comes from spi_ppc4xx_setupxfer.  I believe your patch
> is doing what you intended (i.e. forcing an initial call to
> spi_ppc4xx_setupxfer), but it exposes an OF / SPI linkage problem.
> 
> Namely, of_register_spi_devices does not support a bits-per-word
> property, so bits-per-word is zero.

Bits-per-word == 0 must be interpreted as == 8.

Simple bug in the ppc4xx code.  It currently rejects
values other than 8.

Speaking of spi_ppc4xx issues ... I still have an oldish
copy in my review queue, it needs something like the
appended patch.  (Plus something to accept bpw == 0.)
Is there a newer version?

- Dave
Steven A. Falco - June 24, 2009, 2:25 p.m.
David Brownell wrote:
> On Tuesday 23 June 2009, Steven A. Falco wrote:
>> m25p80 spi0.0: invalid bits-per-word (0)
>>
>> This message comes from spi_ppc4xx_setupxfer.  I believe your patch
>> is doing what you intended (i.e. forcing an initial call to
>> spi_ppc4xx_setupxfer), but it exposes an OF / SPI linkage problem.
>>
>> Namely, of_register_spi_devices does not support a bits-per-word
>> property, so bits-per-word is zero.
> 
> Bits-per-word == 0 must be interpreted as == 8.
> 
> Simple bug in the ppc4xx code.  It currently rejects
> values other than 8.

Ok - I'll post a patch for that.  Your changes to bitbang_work look
good.  I'm not clear on why you first set do_setup = -1 but later
use do_setup = 1.  Perhaps they should both be "1".  Other than that,

Acked-by: Steven A. Falco <sfalco@harris.com>

> 
> Speaking of spi_ppc4xx issues ... I still have an oldish
> copy in my review queue, it needs something like the
> appended patch.  (Plus something to accept bpw == 0.)
> Is there a newer version?

That is a question for Stefan.  Perhaps when I post my patch
to the PPC list, we can move this further along...

	Steve
Stefan Roese - June 24, 2009, 2:33 p.m.
On Wednesday 24 June 2009 16:25:20 Steven A. Falco wrote:
> > Speaking of spi_ppc4xx issues ... I still have an oldish
> > copy in my review queue, it needs something like the
> > appended patch.  (Plus something to accept bpw == 0.)
> > Is there a newer version?
>
> That is a question for Stefan.  Perhaps when I post my patch
> to the PPC list, we can move this further along...

I have to admit that I didn't find the time to rework the driver after David's 
latest review. Frankly, this could take some time since I'm currently busy 
with other tasks. So it would be great if someone else (Steven?) might pick up 
here and resubmit this driver so that we can get it finally included.

Thanks.

Best regards,
Stefan
Steven A. Falco - June 24, 2009, 2:36 p.m.
Stefan Roese wrote:
> On Wednesday 24 June 2009 16:25:20 Steven A. Falco wrote:
>>> Speaking of spi_ppc4xx issues ... I still have an oldish
>>> copy in my review queue, it needs something like the
>>> appended patch.  (Plus something to accept bpw == 0.)
>>> Is there a newer version?
>> That is a question for Stefan.  Perhaps when I post my patch
>> to the PPC list, we can move this further along...
> 
> I have to admit that I didn't find the time to rework the driver after David's 
> latest review. Frankly, this could take some time since I'm currently busy 
> with other tasks. So it would be great if someone else (Steven?) might pick up 
> here and resubmit this driver so that we can get it finally included.
> 
> Thanks.
> 
> Best regards,
> Stefan

Ok - I'll take that on.

Please, both David and Stefan send me the latest versions
and/or patches you have, and I'll integrate them and post
to the PPC list.

	Steve
Stefan Roese - June 24, 2009, 2:50 p.m.
On Wednesday 24 June 2009 16:36:58 Steven A. Falco wrote:
> > I have to admit that I didn't find the time to rework the driver after
> > David's latest review. Frankly, this could take some time since I'm
> > currently busy with other tasks. So it would be great if someone else
> > (Steven?) might pick up here and resubmit this driver so that we can get
> > it finally included.
> >
> > Thanks.
> >
> > Best regards,
> > Stefan
>
> Ok - I'll take that on.

Great, thanks.

> Please, both David and Stefan send me the latest versions
> and/or patches you have, and I'll integrate them and post
> to the PPC list.

I just sent you my latest driver version (v6). Here a link to David's latest 
review:

http://article.gmane.org/gmane.linux.ports.ppc64.devel/55229

Best regards,
Stefan
David Brownell - June 24, 2009, 3:13 p.m.
On Wednesday 24 June 2009, Steven A. Falco wrote:
> Your changes to bitbang_work look good.

You tested?


> I'm not clear on why you first set do_setup = -1 but later 
> use do_setup = 1.  Perhaps they should both be "1".  Other than that,
> 
> Acked-by: Steven A. Falco <sfalco@harris.com>

The "-1" is for the init path, "1" for per-transfer overrides;
this way it avoids some extra calls to set up the bits/speed.
Steven A. Falco - June 24, 2009, 4:14 p.m.
David Brownell wrote:
> On Wednesday 24 June 2009, Steven A. Falco wrote:
>> Your changes to bitbang_work look good.
> 
> You tested?
> 

Yes - I built a kernel with your patch, combined with the changes I
just posted to linuxppc-dev@ozlabs.org as:

"[PATCH v1] Make spi_ppc4xx.c tolerate 0 bits-per-word and 0 speed_hz"

I was successful in operating both the m25p16 at 16 MHz and the AVR
at 240 KHz, as verified by oscilloscope.  So my "ack" includes testing.

> 
>> I'm not clear on why you first set do_setup = -1 but later 
>> use do_setup = 1.  Perhaps they should both be "1".  Other than that,
>>
>> Acked-by: Steven A. Falco <sfalco@harris.com>
> 
> The "-1" is for the init path, "1" for per-transfer overrides;
> this way it avoids some extra calls to set up the bits/speed.

Got it.  No further comments.  My "ack" stands.

I'll start looking at a revised version of the spi_ppc4xx.c driver,
integrating your comments.

	Steve

Patch

--- a/drivers/spi/spi_ppc4xx.c
+++ b/drivers/spi/spi_ppc4xx.c
@@ -61,9 +61,6 @@ 
 /* RxD ready */
 #define SPI_PPC4XX_SR_RBR	(0x80 >> 7)
 
-/* the spi->mode bits understood by this driver: */
-#define MODEBITS	(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST)
-
 /* clock settings (SCP and CI) for various SPI modes */
 #define SPI_CLK_MODE0	SPI_PPC4XX_MODE_SCP
 #define SPI_CLK_MODE1	0
@@ -198,9 +195,6 @@  static int spi_ppc4xx_setup(struct spi_d
 	struct spi_ppc4xx_cs *cs = spi->controller_state;
 	int init = 0;
 
-	if (!spi->bits_per_word)
-		spi->bits_per_word = 8;
-
 	if (spi->bits_per_word != 8) {
 		dev_err(&spi->dev, "invalid bits-per-word (%d)\n",
 			spi->bits_per_word);
@@ -212,12 +206,6 @@  static int spi_ppc4xx_setup(struct spi_d
 		return -EINVAL;
 	}
 
-	if (spi->mode & ~MODEBITS) {
-		dev_dbg(&spi->dev, "setup: unsupported mode bits %x\n",
-			spi->mode & ~MODEBITS);
-		return -EINVAL;
-	}
-
 	if (cs == NULL) {
 		cs = kzalloc(sizeof *cs, GFP_KERNEL);
 		if (!cs)
@@ -268,10 +256,6 @@  static int spi_ppc4xx_setup(struct spi_d
 		}
 	}
 
-	dev_dbg(&spi->dev, "%s: mode %d, %u bpw, %d hz\n",
-		__func__, spi->mode, spi->bits_per_word,
-		spi->max_speed_hz);
-
 	return 0;
 }
 
@@ -442,6 +426,9 @@  static int __init spi_ppc4xx_of_probe(st
 		}
 	}
 
+	/* the spi->mode bits understood by this driver: */
+	master->modebits = SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST;
+
 	/* Setup the state for the bitbang driver */
 	bbp = &hw->bitbang;
 	bbp->master = hw->master;