Message ID | 200911181720.06390.to-fleischer@t-online.de (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Grant Likely |
Headers | show |
On Wed, Nov 18, 2009 at 05:20:06PM +0100, Torsten Fleischer wrote: [...] > > 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. > 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. > 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. > 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!
On Wed, Nov 18, 2009 at 4:29 PM, Anton Vorontsov <avorontsov@ru.mvista.com> wrote: > On Wed, Nov 18, 2009 at 05:20:06PM +0100, Torsten Fleischer wrote: > [...] >> > 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. Hey Torsten, do you have an updated version of this change to address the comments? I'm collecting the last few things for some linux-next exposure now. g.
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; 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); + + 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)); + + gpio_direction_output(gpio, on); +} + static int of_mpc8xxx_spi_get_chipselects(struct device *dev) { struct device_node *np = dev_archdata_get_node(&dev->archdata); @@ -720,14 +743,6 @@ pinfo->gpios[i] = gpio; pinfo->alow_flags[i] = flags & OF_GPIO_ACTIVE_LOW; - - ret = gpio_direction_output(pinfo->gpios[i], - pinfo->alow_flags[i]); - if (ret) { - dev_err(dev, "can't set output direction for gpio " - "#%d: %d\n", i, ret); - goto err_loop; - } } pdata->max_chipselect = ngpios;