RE: support DUAL and QUAD[patch v1]

["Gupta, Pekon" <pekon-l0cyMroinI0@public.gmane.org>]

> 
> On Wed, Jul 17, 2013 at 6:58 PM, yuhang wang
> <wangyuhang2014-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > >> > >
> > >> > > > [Pekon]: Instead of adding new fields you can use existing 'mode'
> > >> > > > field
> > >> to
> > >> > > > pass on the platform specific configurations. And if 'u8 mode'
> > >> > > > does not
> > >> > > > suffice you can increase it to 'u32'.
> 
> But what about spidev?
> 
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 004b10f..36d2451 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -868,6 +868,14 @@ static void of_register_spi_devices(struct
> > spi_master *master)
> >   spi->mode |= SPI_CS_HIGH;
> >   if (of_find_property(nc, "spi-3wire", NULL))
> >   spi->mode |= SPI_3WIRE;
> > + if (of_find_property(nc, "spi-tx-dual", NULL))
> > + spi->mode |= SPI_TX_DUAL;
> > + if (of_find_property(nc, "spi-tx-quad", NULL))
> > + spi->mode |= SPI_TX_QUAD;
> > + if (of_find_property(nc, "spi-rx-dual", NULL))
> > + spi->mode |= SPI_RX_DUAL;
> > + if (of_find_property(nc, "spi-rx-quad", NULL))
> > + spi->mode |= SPI_RX_QUAD;
> >
> >   /* Device speed */
> >   prop = of_get_property(nc, "spi-max-frequency", &len);
> > @@ -1316,6 +1324,12 @@ int spi_setup(struct spi_device *spi)
> >   /* help drivers fail *cleanly* when they need options
> >   * that aren't supported with their current master
> >   */
> > + if (((spi->mode >> 8) & 0x03) == 0x03 ||
> > + ((spi->mode >> 10) & 0x03) == 0x03) {
> > + dev_err(&spi->dev,
> > + "setup: can not select dual and quad at the same time\n");
> > + return -EINVAL;
> > + }
> 
> Maybe it would make more sense to have a spi-rx-width and spi-tx-width
> than can be set to 1, 2, 4, etc. bits instead of a bunch of properties
> that are mutually exclusive?  Sooner or later someone is going to want
> 8 bits.
> 
Then it would not remain a serial interface (Serial Peripheral Interface),
rather it would be more of parallel memory like interface.
And you would not like to use SPI controller for it, instead you would use
On-chip General Purpose External Interface controllers (like GPMC)
because it gives you more options to tweak and configure.

In general, If the place-holders are already there (like 'mode' field in 
struct spi_device), I would prefer to re-use them.
But if you are adding new ones just ensure its scalability and usage.
Names don't matter much there then, except for good readability.

> > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> > index 38c2b92..e9ba1cf 100644
> > --- a/include/linux/spi/spi.h
> > +++ b/include/linux/spi/spi.h
> > @@ -74,7 +74,7 @@ struct spi_device {
> >   struct spi_master *master;
> >   u32 max_speed_hz;
> >   u8 chip_select;
> > - u8 mode;
> > + u16 mode;
> 
> What about this, from include/uapi/linux/spi/spidev.h:
> 
> /* Read / Write of SPI mode (SPI_MODE_0..SPI_MODE_3) */
> #define SPI_IOC_RD_MODE                 _IOR(SPI_IOC_MAGIC, 1, __u8)
> #define SPI_IOC_WR_MODE                 _IOW(SPI_IOC_MAGIC, 1, __u8)
> 
> Note that you can't just change the type in the ioctl define, as it
> won't be backward compatible.
> 
Thanks, for pointing this out. Yes this is a constrain, if we change the
existing type. But I don't think u8 -> u32 is really required, it was just
a thought, not deep-dived.


with regards, pekon

------------------------------------------------------------------------------
See everything from the browser to the database with AppDynamics
Get end-to-end visibility with application monitoring from AppDynamics
Isolate bottlenecks and diagnose root cause in seconds.
Start your free trial of AppDynamics Pro today!
http://pubads.g.doubleclick.net/gampad/clk?id=48808831&iu=/4140/ostg.clktrk