| Message ID | 200906231538.40125.david-b@pacbell.net |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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.
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
--- 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;