Re: spi_mpc8xxx.c: chip select polarity problem
From: Anton Vorontsov <hidden>
Date: 2009-11-18 23:29:25
On Wed, Nov 18, 2009 at 05:20:06PM +0100, Torsten Fleischer wrote: [...]
quoted
Oh. On the other hand, we can postpone the gpio_direction_output() call, and still require that the platform code (or firmware) should be responsible for setting a sane default values on the chip selects.How about that?
Looks great, thanks! Few minor issues below.
quoted hunk ↗ jump to hunk
diff -u -r -N linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c--- linux-2.6.31.6_orig//drivers/spi/spi_mpc8xxx.c 2009-11-10 01:32:31.000000000 +0100 +++ linux-2.6.31.6/drivers/spi/spi_mpc8xxx.c 2009-11-18 10:47:06.000000000 +0100@@ -114,6 +114,7 @@ u32 rx_shift; /* RX data reg shift when in qe mode */ u32 tx_shift; /* TX data reg shift when in qe mode */ u32 hw_mode; /* Holds HW mode register settings */ + int initialized; }; static inline void mpc8xxx_spi_write_reg(__be32 __iomem *reg, u32 val)@@ -437,6 +438,7 @@ cs = kzalloc(sizeof *cs, GFP_KERNEL); if (!cs) return -ENOMEM; + cs->initialized = 0;
No need for this, we allocated cs with kzalloc, which zeroes the memory anyway.
quoted hunk ↗ jump to hunk
spi->controller_state = cs; } mpc8xxx_spi = spi_master_get_devdata(spi->master);@@ -503,15 +505,25 @@ return ret; } + +static void mpc8xxx_spi_cs_init(struct spi_device *spi); + +
Whenever possible, please avoid forward declarations.
quoted hunk ↗ jump to hunk
static int mpc8xxx_spi_transfer(struct spi_device *spi, struct spi_message *m) { struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi->master); + struct spi_mpc8xxx_cs *cs = spi->controller_state; unsigned long flags; m->actual_length = 0; m->status = -EINPROGRESS; + if (cs && !cs->initialized) { + mpc8xxx_spi_cs_init(spi); + cs->initialized = 1; + } + spin_lock_irqsave(&mpc8xxx_spi->lock, flags); list_add_tail(&m->queue, &mpc8xxx_spi->queue); queue_work(mpc8xxx_spi->workqueue, &mpc8xxx_spi->work);@@ -671,6 +683,17 @@ gpio_set_value(gpio, on ^ alow); } +static void mpc8xxx_spi_cs_init(struct spi_device *spi) +{ + struct device *dev = spi->dev.parent; + struct mpc8xxx_spi_probe_info *pinfo = to_of_pinfo(dev->platform_data); + u16 cs = spi->chip_select; + int gpio = pinfo->gpios[cs]; + bool on = (pinfo->alow_flags[cs] ^ !(spi->mode & SPI_CS_HIGH));
The outermost parenthesis aren't needed.
+ + gpio_direction_output(gpio, on);
gpio_direction_output() may fail, so spi_cs_init too.
I'd write it as 'return gpio_direction_outpit(..)', and in
mpc8xxx_spi_transfer something like this:
if (cs && !cs->initialized) {
int ret;
ret = mpc8xxx_spi_cs_init(spi);
if (ret) {
dev_dbg(&spi->dev, "cs_init failed: %d\n", ret);
return ret;
}
cs->initialized = 1;
}
Thanks!
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2