Re: [PATCH v3] spi: Add PPC4xx SPI driver

[Josh Boyer <jwboyer@linux.vnet.ibm.com>]

On Fri, 31 Oct 2008 09:31:19 +0100
Stefan Roese <sr@denx.de> wrote:

> On Wednesday 29 October 2008, Josh Boyer wrote:
> > Looks pretty good.  Just a few minor comments/questions below.
> 
> Thanks. I also added some comments below.
> 
> > Also, do you have a patch for a DTS file that gives an example of how to
> > instantiate the SPI stuff in the device tree?
> 
> OK, I'll add a DTS patch to the next version.

Ok thanks.

> > >+	/* send the first byte */
> > >+	data = hw->tx ? hw->tx[0] : 0;
> > >+	out_8(&hw->regs->txd, data);
> > >+	out_8(&hw->regs->cr, SPI_PPC4XX_CR_STR);
> >
> > Maybe iowrite8?  Same comment elsewhere.
> 
> Why? We use the in_/out_xxx() accessor function for all other 4xx driver as 
> well.

Yeah.  We used to have all of 4xx in arch/ppc too.  ;)

Seriously though, it doesn't really bother me too much.  The
io{read,write} functions do have a bit better clarity as to endian-ness
though.

> > >+	wait_for_completion(&hw->done);
> > >+
> > >+	return hw->count;
> > >+}
> > >+
> >
> > <snip>
> >
> > >+static struct of_device_id spi_ppc4xx_of_match[] = {
> > >+	{ .compatible = "ibm,spi", },
> > >+	{},
> > >+};
> >
> > I'm wondering if that is too generic of a match.  In theory,
> > IBM could have another SPI controller that isn't for 4xx.
> > Maybe "ibm,spi-4xx" ?
> 
> Right. I was doing it the same way as already done before, e.g. "ibm,iic". For 
> the gpio driver we already switched to "ibm,ppc4xx-gpio". So how 
> about "ibm,ppc4xx-spi"?

Sounds fine to me.

josh