Re: [PATCH] spi: Added spi master driver for Freescale MPC83xx SPI controller

[David Brownell <david-b@pacbell.net>]

> This driver supports the SPI controller on the MPC83xx SoC devices from Freescale.
> Note, this driver supports only the simple shift register SPI controller and not
> the descriptor based CPM or QUICCEngine SPI controller.

Or the QSPI on Coldfire; there's a driver for that floating around, but for an
older version of this framework (sans lists).


> --- /dev/null
> +++ b/drivers/spi/spi_mpc83xx.c
> @@ -0,0 +1,349 @@
> +/*
> + * MPC83xx SPI controller driver.
> + *
> + * Maintainer: Kumar Gala

Needs a "Copyright (C) 2006 <NAME>" for the GPL to be valid; it's
the copyright holder who licences the code.


> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> ...
> +
> +/* Default for SPI Mode, slowest speed, MSB, inactive high, 8-bit char */
> +#define	SPMODE_INIT_VAL (SPMODE_CI_INACTIVEHIGH | SPMODE_CP_RISE_EDGECLK | \
> +			 SPMODE_DIV16 | SPMODE_REV | SPMODE_MS | \
> +			 SPMODE_LEN(7) | SPMODE_PM(0xf))

Hmm, it will of course be overridden as soon as needed, but shouldn't
that default be "inactive low" clock?  SPI mode 0 that is.  That stands
out mostly because you were interpreting CPOL=0 as inactive high, and
that's not my understanding of how that signal works...


> +struct mpc83xx_spi {
> +	/* bitbang has to be first */
> +	struct spi_bitbang bitbang;
> +	struct completion tx_done, rx_ready;
> +
> +	u32 __iomem *base;

Erm, OK, but fwiw my preference is to have pointer-to-struct and let
the compiler calculate the offsets (and tell you when you pass the wrong
kind of pointer).  Otherwise such pointers should use "void __iomem *"
(or maybe in your case "__be32 *"?) for explicit {base,offset} addressing.


> +static inline void mpc83xx_spi_write_reg(__be32 * base, u32 reg, u32 val)
> +{
> +	out_be32(base + (reg >> 2), val);
> +}
> +
> +static inline u32 mpc83xx_spi_read_reg(__be32 * base, u32 reg)
> +{
> +	return in_be32(base + (reg >> 2));
> +}

... here you use "__be32" not "u32", and no "__iomem" annotation.  So
this is inconsistent with the declaration above.  Note that if you
just made this "&bank->regname" you'd be having the compiler do any
offset calculation magic, and the source code will be more obvious.


> +static
> +int mpc83xx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
> +{
> +	struct mpc83xx_spi *mpc83xx_spi;
> +	u32 regval;
> +	u32 len = t->bits_per_word - 1;
> +
> +	if (len == 32)
> +		len = 0;

So the hardware handles 1-33 bit words?  It'd be good to filter
the spi_setup() path directly then, returning EINVAL for illegal
word lengths (and clock speeds).



> +static u32
> +mpc83xx_spi_txrx(struct spi_device *spi, unsigned nsecs, u32 word, u8 bits)
> +{
> +	struct mpc83xx_spi *mpc83xx_spi;
> +	mpc83xx_spi = spi_master_get_devdata(spi->master);
> +
> +	INIT_COMPLETION(mpc83xx_spi->tx_done);
> +	INIT_COMPLETION(mpc83xx_spi->rx_ready);
> +
> +	/* enable tx/rx ints */
> +	mpc83xx_spi_write_reg(mpc83xx_spi->base, SPIM_REG, SPIM_NF | SPIM_NE);
> +
> +	/* transmit word */
> +	mpc83xx_spi_write_reg(mpc83xx_spi->base, SPITD_REG, word);
> +
> +	/* wait for both a tx & rx interrupt */
> +	wait_for_completion(&mpc83xx_spi->tx_done);
> +	wait_for_completion(&mpc83xx_spi->rx_ready);

I guess I'm surprised you're not using txrx_buffers() and having
that whole thing be IRQ driven, so the per-word cost eliminates
the task scheduling.  You already paid for IRQ handling ... why
not have it store the rx byte into the buffer, and write the tx
byte froom the other buffer?  That'd be cheaper than what you're
doing now ... in both time and code.  Only wake up a task at
the end of a given spi_transfer().

Plus, your IRQ handler should _not_ always return IRQ_HANDLED.
Only return it if you actually do enter one of those branches...


> +	mpc83xx_spi->sysclk = pdata->sysclk;

When MPC/PPC starts to support <linux/clk.h> would seem to be
the right sort of time to

	mpc83xx-spi->clk = clk_get(&pdev->dev, "spi_clk");

or whatever.


> +MODULE_DESCRIPTION("Simple Platform SPI Driver");

How about "Simple MPC83xx SPI driver"?

- Dave
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/