| Message ID | 1376216117-5864-1-git-send-email-wangyuhang2014@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Aug 11, 2013 at 06:15:17PM +0800, wangyuhang wrote: Applied, this looks good thanks. A few coding style issues which I'll fix up myself but one thing: > fix the previous patch some mistake below: > 1. DT in slave node, use "spi-tx-nbits = <1/2/4>" in place of using > "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the > previous way to get the property in @of_register_spi_devices(). This new DT stuff needs to be reviewed on the devicetree list and documented in the generic SPI bindings. Can you please send a patch doing that? Please also note that the SPI list moved to vger.
Hi Mark, Wang, On Sunday 11 August 2013 03:45 PM, wangyuhang wrote: > fix the previous patch some mistake below: > 1. DT in slave node, use "spi-tx-nbits =<1/2/4>" in place of using > "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the > previous way to get the property in @of_register_spi_devices(). > 2. Change the value of transfer bit macro(SPI_NBITS_SINGLE, SPI_NBITS_DUAL > SPI_NBITS_QUAD) to 0x01, 0x02 and 0x04 to match the actual wires. > 3. Add the following check > (1)keep the tx_nbits and rx_nbits in spi_transfer is not beyond the > single, dual and quad. > (2)keep tx_nbits and rx_nbits are contained by @spi_device->mode > example: if @spi_device->mode = DUAL, then tx/rx_nbits can not be set > to QUAD(SPI_NBITS_QUAD) > (3)if "@spi_device->mode& SPI_3WIRE", then tx/rx_nbits should be in > single(SPI_NBITS_SINGLE) > > Signed-off-by: wangyuhang<wangyuhang2014@gmail.com> > --- > drivers/spi/spi.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/spi/spi.h | 22 ++++++++++- > 2 files changed, 116 insertions(+), 2 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 004b10f..78f257a 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -869,6 +869,51 @@ static void of_register_spi_devices(struct spi_master *master) > if (of_find_property(nc, "spi-3wire", NULL)) > spi->mode |= SPI_3WIRE; > > + /* Device DUAL/QUAD mode */ > + prop = of_get_property(nc, "spi-tx-nbits",&len); > + if (!prop || len< sizeof(*prop)) { > + dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", > + nc->full_name); > + spi_dev_put(spi); > + continue; > + } > + switch(be32_to_cpup(prop)) { > + case SPI_NBITS_SINGLE: > + break; > + case SPI_NBITS_DUAL: > + spi->mode |= SPI_TX_DUAL; > + break; > + case SPI_NBITS_QUAD: > + spi->mode |= SPI_TX_QUAD; > + break; > + default: > + dev_err(&master->dev, "spi-tx-nbits value is not supported\n"); > + spi_dev_put(spi); > + continue; > + } > + > + prop = of_get_property(nc, "spi-rx-nbits",&len); > + if (!prop || len< sizeof(*prop)) { > + dev_err(&master->dev, "%s has no 'spi-rx-nbits' property\n", > + nc->full_name); > + spi_dev_put(spi); > + continue; > + } > + switch(be32_to_cpup(prop)) { > + case SPI_NBITS_SINGLE: > + break; > + case SPI_NBITS_DUAL: > + spi->mode |= SPI_RX_DUAL; > + break; > + case SPI_NBITS_QUAD: > + spi->mode |= SPI_RX_QUAD; > + break; > + default: > + dev_err(&master->dev, "spi-rx-nbits value is not supported\n"); > + spi_dev_put(spi); > + continue; > + } > + > /* Device speed */ > prop = of_get_property(nc, "spi-max-frequency",&len); > if (!prop || len< sizeof(*prop)) { > @@ -1313,6 +1358,19 @@ int spi_setup(struct spi_device *spi) > unsigned bad_bits; > int status = 0; > > + /* check mode to prevent that DUAL and QUAD set at the same time > + */ > + if (((spi->mode& SPI_TX_DUAL)&& (spi->mode& SPI_TX_QUAD)) || > + ((spi->mode& SPI_RX_DUAL)&& (spi->mode& SPI_RX_QUAD))) { > + dev_err(&spi->dev, > + "setup: can not select dual and quad at the same time\n"); > + return -EINVAL; > + } > + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden > + */ > + if ((spi->mode& SPI_3WIRE)&& (spi->mode& > + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD))) > + return -EINVAL; > /* help drivers fail *cleanly* when they need options > * that aren't supported with their current master > */ > @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) > /** > * Set transfer bits_per_word and max speed as spi device default if > * it is not set for this transfer. > + * Set transfer tx_nbits and rx_nbits as single transfer default > + * (SPI_NBITS_SINGLE) if it is not set for this transfer. > */ > list_for_each_entry(xfer,&message->transfers, transfer_list) { > if (!xfer->bits_per_word) > xfer->bits_per_word = spi->bits_per_word; > if (!xfer->speed_hz) > xfer->speed_hz = spi->max_speed_hz; > + if (xfer->tx_buf&& !xfer->tx_nbits) > + xfer->tx_nbits = SPI_NBITS_SINGLE; > + if (xfer->rx_buf&& !xfer->rx_nbits) > + xfer->rx_nbits = SPI_NBITS_SINGLE; > + /* check transfer tx/rx_nbits: > + * 1. keep the value is not out of single, dual and quad > + * 2. keep tx/rx_nbits is contained by mode in spi_device > + * 3. if SPI_3WIRE, tx/rx_nbits should be in single > + */ Sorry for pitchin in a bit late on this. I was trying to use this for my use case today. I realise this check should be under... > + if (xfer->tx_nbits != SPI_NBITS_SINGLE&& > + xfer->tx_nbits != SPI_NBITS_DUAL&& > + xfer->tx_nbits != SPI_NBITS_QUAD) > + return -EINVAL; > + if ((xfer->tx_nbits == SPI_NBITS_DUAL)&& > + !(spi->mode& (SPI_TX_DUAL | SPI_TX_QUAD))) > + return -EINVAL; > + if ((xfer->tx_nbits == SPI_NBITS_QUAD)&& > + !(spi->mode& SPI_TX_QUAD)) > + return -EINVAL; > + if ((spi->mode& SPI_3WIRE)&& > + (xfer->tx_nbits != SPI_NBITS_SINGLE)) > + return -EINVAL; if (xfer->tx_buf) { ...... } and > + /* check transfer rx_nbits */ > + if (xfer->rx_nbits != SPI_NBITS_SINGLE&& > + xfer->rx_nbits != SPI_NBITS_DUAL&& > + xfer->rx_nbits != SPI_NBITS_QUAD) > + return -EINVAL; > + if ((xfer->rx_nbits == SPI_NBITS_DUAL)&& > + !(spi->mode& (SPI_RX_DUAL | SPI_RX_QUAD))) > + return -EINVAL; > + if ((xfer->rx_nbits == SPI_NBITS_QUAD)&& > + !(spi->mode& SPI_RX_QUAD)) > + return -EINVAL; > + if ((spi->mode& SPI_3WIRE)&& > + (xfer->rx_nbits != SPI_NBITS_SINGLE)) > + return -EINVAL; this should be under if (xfer->rx_buf) { ..... } > } > > message->spi = spi; > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 38c2b92..3dc0a86 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; > #define SPI_CPHA 0x01 /* clock phase */ > #define SPI_CPOL 0x02 /* clock polarity */ > #define SPI_MODE_0 (0|0) /* (original MicroWire) */ > @@ -87,6 +87,10 @@ struct spi_device { > #define SPI_LOOP 0x20 /* loopback mode */ > #define SPI_NO_CS 0x40 /* 1 dev/bus, no chipselect */ > #define SPI_READY 0x80 /* slave pulls low to pause */ > +#define SPI_TX_DUAL 0x100 /* transmit with 2 wires */ > +#define SPI_TX_QUAD 0x200 /* transmit with 4 wires */ > +#define SPI_RX_DUAL 0x400 /* receive with 2 wires */ > +#define SPI_RX_QUAD 0x800 /* receive with 4 wires */ > u8 bits_per_word; > int irq; > void *controller_state; > @@ -437,6 +441,10 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum); > * @rx_buf: data to be read (dma-safe memory), or NULL > * @tx_dma: DMA address of tx_buf, if @spi_message.is_dma_mapped > * @rx_dma: DMA address of rx_buf, if @spi_message.is_dma_mapped > + * @tx_nbits: number of bits used for writting. If 0 the default > + * (SPI_NBITS_SINGLE) is used. > + * @rx_nbits: number of bits used for reading. If 0 the default > + * (SPI_NBITS_SINGLE) is used. > * @len: size of rx and tx buffers (in bytes) > * @speed_hz: Select a speed other than the device default for this > * transfer. If 0 the default (from @spi_device) is used. > @@ -491,6 +499,11 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum); > * by the results of previous messages and where the whole transaction > * ends when the chipselect goes intactive. > * > + * When SPI can transfer in 1x,2x or 4x. It can get this tranfer information > + * from device through @tx_nbits and @rx_nbits. In Bi-direction, these > + * two should both be set. User can set transfer mode with SPI_NBITS_SINGLE(1x) > + * SPI_NBITS_DUAL(2x) and SPI_NBITS_QUAD(4x) to support these three transfer. > + * > * The code that submits an spi_message (and its spi_transfers) > * to the lower layers is responsible for managing its memory. > * Zero-initialize every field you don't set up explicitly, to > @@ -511,6 +524,11 @@ struct spi_transfer { > dma_addr_t rx_dma; > > unsigned cs_change:1; > + u8 tx_nbits; > + u8 rx_nbits; > +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ > +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ > +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ > u8 bits_per_word; > u16 delay_usecs; > u32 speed_hz; > @@ -858,7 +876,7 @@ struct spi_board_info { > /* mode becomes spi_device.mode, and is essential for chips > * where the default of SPI_CS_HIGH = 0 is wrong. > */ > - u8 mode; > + u16 mode; > > /* ... may need additional spi_device chip config data here. > * avoid stuff protocol drivers can set; but include stuff
On Sun, Aug 11, 2013 at 06:15:17PM +0800, wangyuhang wrote:
> fix the previous patch some mistake below:
One other thing: this wasn't sent against current code, it seems to have
been generated against an old kernel version. Please make sure when you
submit that you submit against either Linus' tree or ideally the latest
development code for the subsystem.
On Thu, Aug 22, 2013 at 06:05:11PM +0530, Sourav Poddar wrote: > On Sunday 11 August 2013 03:45 PM, wangyuhang wrote: > >+ /* check transfer tx/rx_nbits: > >+ * 1. keep the value is not out of single, dual and quad > >+ * 2. keep tx/rx_nbits is contained by mode in spi_device > >+ * 3. if SPI_3WIRE, tx/rx_nbits should be in single > >+ */ > Sorry for pitchin in a bit late on this. > I was trying to use this for my use case today. I realise this check > should be under... Can you send a patch for this please?
On Thursday 22 August 2013 06:51 PM, Mark Brown wrote: > On Thu, Aug 22, 2013 at 06:05:11PM +0530, Sourav Poddar wrote: >> On Sunday 11 August 2013 03:45 PM, wangyuhang wrote: >>> + /* check transfer tx/rx_nbits: >>> + * 1. keep the value is not out of single, dual and quad >>> + * 2. keep tx/rx_nbits is contained by mode in spi_device >>> + * 3. if SPI_3WIRE, tx/rx_nbits should be in single >>> + */ >> Sorry for pitchin in a bit late on this. >> I was trying to use this for my use case today. I realise this check >> should be under... > Can you send a patch for this please? Yes, I will send it.
On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014@gmail.com> wrote: > fix the previous patch some mistake below: This isn't a very good commit message. "the previous patch" will have no meaning in the kernel git repo. The rest of the message only describes changes since a previous version of the patch and not the actual patch in full. > > + /* Device DUAL/QUAD mode */ > + prop = of_get_property(nc, "spi-tx-nbits", &len); Why not use of_property_read_u32() here? > + if (!prop || len < sizeof(*prop)) { > + dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", > + nc->full_name); > + spi_dev_put(spi); > + continue; So if no spi-tx-nbits property is supplied, the device is rejected and the loop continues to the next device entry? This means ALL EXISTING DEVICE TREES with SPI devices will be rejected, since none of them have this new property! Was this patch tested at all with any system before being accepted? > + /* check mode to prevent that DUAL and QUAD set at the same time > + */ > + if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) || > + ((spi->mode & SPI_RX_DUAL) && (spi->mode & SPI_RX_QUAD))) { > + dev_err(&spi->dev, > + "setup: can not select dual and quad at the same time\n"); > + return -EINVAL; This test can be done with fewer operations as: if ((spi->mode & (SPI_TX_DUAL|SPI_TX_QUAD)) == (SPI_TX_DUAL|SPI_TX_QUAD) || (spi->mode & (SPI_RX_DUAL|SPI_RX_QUAD)) == (SPI_RX_DUAL|SPI_RX_QUAD)) { > + } > + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden > + */ > + if ((spi->mode & SPI_3WIRE) && (spi->mode & > + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD))) > + return -EINVAL; No dev_err message for this possibility? What's different than the previous check that does produce a message? > /* help drivers fail *cleanly* when they need options > * that aren't supported with their current master > */ Following this comment is the code: bad_bits = spi->mode & ~spi->master->mode_bits; if (bad_bits) { dev_err(&spi->dev, "setup: unsupported mode bits %x\n", bad_bits); return -EINVAL; } Won't this always trigger for anything that sets one of the dual or quad bits? > @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) > /** > * Set transfer bits_per_word and max speed as spi device default if > * it is not set for this transfer. > + * Set transfer tx_nbits and rx_nbits as single transfer default > + * (SPI_NBITS_SINGLE) if it is not set for this transfer. > */ > list_for_each_entry(xfer, &message->transfers, transfer_list) { > if (!xfer->bits_per_word) > xfer->bits_per_word = spi->bits_per_word; > if (!xfer->speed_hz) > xfer->speed_hz = spi->max_speed_hz; > + if (xfer->tx_buf && !xfer->tx_nbits) > + xfer->tx_nbits = SPI_NBITS_SINGLE; > + if (xfer->rx_buf && !xfer->rx_nbits) > + xfer->rx_nbits = SPI_NBITS_SINGLE; > + /* check transfer tx/rx_nbits: > + * 1. keep the value is not out of single, dual and quad > + * 2. keep tx/rx_nbits is contained by mode in spi_device > + * 3. if SPI_3WIRE, tx/rx_nbits should be in single > + */ > + if (xfer->tx_nbits != SPI_NBITS_SINGLE && > + xfer->tx_nbits != SPI_NBITS_DUAL && > + xfer->tx_nbits != SPI_NBITS_QUAD) > + return -EINVAL; > + if ((xfer->tx_nbits == SPI_NBITS_DUAL) && > + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD))) > + return -EINVAL; > + if ((xfer->tx_nbits == SPI_NBITS_QUAD) && > + !(spi->mode & SPI_TX_QUAD)) > + return -EINVAL; > + if ((spi->mode & SPI_3WIRE) && > + (xfer->tx_nbits != SPI_NBITS_SINGLE)) > + return -EINVAL; > + /* check transfer rx_nbits */ > + if (xfer->rx_nbits != SPI_NBITS_SINGLE && > + xfer->rx_nbits != SPI_NBITS_DUAL && > + xfer->rx_nbits != SPI_NBITS_QUAD) > + return -EINVAL; > + if ((xfer->rx_nbits == SPI_NBITS_DUAL) && > + !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD))) > + return -EINVAL; > + if ((xfer->rx_nbits == SPI_NBITS_QUAD) && > + !(spi->mode & SPI_RX_QUAD)) > + return -EINVAL; > + if ((spi->mode & SPI_3WIRE) && > + (xfer->rx_nbits != SPI_NBITS_SINGLE)) > + return -EINVAL; > } What a lot of code to check the transfer bits. It really needs this much? > @@ -511,6 +524,11 @@ struct spi_transfer { > dma_addr_t rx_dma; > > unsigned cs_change:1; > + u8 tx_nbits; > + u8 rx_nbits; > +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ > +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ > +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ These fields increase the size of a spi_transfer by 4 bytes. If you used bitfields instead it wouldn't increase the size at all since there are still 7 bits left after cs_change.
2013/9/4 Trent Piepho <tpiepho@gmail.com>: > On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014@gmail.com> wrote: >> fix the previous patch some mistake below: > > This isn't a very good commit message. "the previous patch" will have > no meaning in the kernel git repo. The rest of the message only > describes changes since a previous version of the patch and not the > actual patch in full. > >> >> + /* Device DUAL/QUAD mode */ >> + prop = of_get_property(nc, "spi-tx-nbits", &len); > > Why not use of_property_read_u32() here? > >> + if (!prop || len < sizeof(*prop)) { >> + dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", >> + nc->full_name); >> + spi_dev_put(spi); >> + continue; > > So if no spi-tx-nbits property is supplied, the device is rejected and > the loop continues to the next device entry? This means ALL EXISTING > DEVICE TREES with SPI devices will be rejected, since none of them > have this new property! Was this patch tested at all with any system > before being accepted? > This part has been fixed in patch[commit id:a822e99c70f448c4068ea85bb195dac0b2eb3afe] >> + /* check mode to prevent that DUAL and QUAD set at the same time >> + */ >> + if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) || >> + ((spi->mode & SPI_RX_DUAL) && (spi->mode & SPI_RX_QUAD))) { >> + dev_err(&spi->dev, >> + "setup: can not select dual and quad at the same time\n"); >> + return -EINVAL; > > This test can be done with fewer operations as: > if ((spi->mode & (SPI_TX_DUAL|SPI_TX_QUAD)) == (SPI_TX_DUAL|SPI_TX_QUAD) || > (spi->mode & (SPI_RX_DUAL|SPI_RX_QUAD)) == (SPI_RX_DUAL|SPI_RX_QUAD)) { > > >> + } >> + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden >> + */ >> + if ((spi->mode & SPI_3WIRE) && (spi->mode & >> + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD))) >> + return -EINVAL; > > No dev_err message for this possibility? What's different than the > previous check that does produce a message? > OK, should be added. Thanks. >> /* help drivers fail *cleanly* when they need options >> * that aren't supported with their current master >> */ > Following this comment is the code: > bad_bits = spi->mode & ~spi->master->mode_bits; > if (bad_bits) { > dev_err(&spi->dev, "setup: unsupported mode bits %x\n", > bad_bits); > return -EINVAL; > } > > Won't this always trigger for anything that sets one of the dual or quad bits? > I can't get your idea clearly. Please provide more infomation. >> @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) >> /** >> * Set transfer bits_per_word and max speed as spi device default if >> * it is not set for this transfer. >> + * Set transfer tx_nbits and rx_nbits as single transfer default >> + * (SPI_NBITS_SINGLE) if it is not set for this transfer. >> */ >> list_for_each_entry(xfer, &message->transfers, transfer_list) { >> if (!xfer->bits_per_word) >> xfer->bits_per_word = spi->bits_per_word; >> if (!xfer->speed_hz) >> xfer->speed_hz = spi->max_speed_hz; >> + if (xfer->tx_buf && !xfer->tx_nbits) >> + xfer->tx_nbits = SPI_NBITS_SINGLE; >> + if (xfer->rx_buf && !xfer->rx_nbits) >> + xfer->rx_nbits = SPI_NBITS_SINGLE; >> + /* check transfer tx/rx_nbits: >> + * 1. keep the value is not out of single, dual and quad >> + * 2. keep tx/rx_nbits is contained by mode in spi_device >> + * 3. if SPI_3WIRE, tx/rx_nbits should be in single >> + */ >> + if (xfer->tx_nbits != SPI_NBITS_SINGLE && >> + xfer->tx_nbits != SPI_NBITS_DUAL && >> + xfer->tx_nbits != SPI_NBITS_QUAD) >> + return -EINVAL; >> + if ((xfer->tx_nbits == SPI_NBITS_DUAL) && >> + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD))) >> + return -EINVAL; >> + if ((xfer->tx_nbits == SPI_NBITS_QUAD) && >> + !(spi->mode & SPI_TX_QUAD)) >> + return -EINVAL; >> + if ((spi->mode & SPI_3WIRE) && >> + (xfer->tx_nbits != SPI_NBITS_SINGLE)) >> + return -EINVAL; >> + /* check transfer rx_nbits */ >> + if (xfer->rx_nbits != SPI_NBITS_SINGLE && >> + xfer->rx_nbits != SPI_NBITS_DUAL && >> + xfer->rx_nbits != SPI_NBITS_QUAD) >> + return -EINVAL; >> + if ((xfer->rx_nbits == SPI_NBITS_DUAL) && >> + !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD))) >> + return -EINVAL; >> + if ((xfer->rx_nbits == SPI_NBITS_QUAD) && >> + !(spi->mode & SPI_RX_QUAD)) >> + return -EINVAL; >> + if ((spi->mode & SPI_3WIRE) && >> + (xfer->rx_nbits != SPI_NBITS_SINGLE)) >> + return -EINVAL; >> } > > What a lot of code to check the transfer bits. It really needs this much? > This part is also corrected in patch [commit id:db90a44177ac39fc22b2da5235b231fccdd4c673]. >> @@ -511,6 +524,11 @@ struct spi_transfer { >> dma_addr_t rx_dma; >> >> unsigned cs_change:1; >> + u8 tx_nbits; >> + u8 rx_nbits; >> +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ >> +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ >> +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ > > These fields increase the size of a spi_transfer by 4 bytes. If you > used bitfields instead it wouldn't increase the size at all since > there are still 7 bits left after cs_change. Yes, it will increase the size by 4 bytes, but using bitfields here will make it logically weird. Bitfields may mean that really have the restriction of number of bits. If just for saving memory, then what about the other members such as bits_per_world, delay_usecs.
On Tue, Sep 03, 2013 at 08:24:08PM -0700, Trent Piepho wrote: > So if no spi-tx-nbits property is supplied, the device is rejected and > the loop continues to the next device entry? This means ALL EXISTING > DEVICE TREES with SPI devices will be rejected, since none of them > have this new property! Was this patch tested at all with any system > before being accepted? This particular issue is already fixed properly anyway.
On Wed, Sep 4, 2013 at 12:07 AM, yuhang wang <wangyuhang2014@gmail.com> wrote: >>> dma_addr_t rx_dma; >>> >>> unsigned cs_change:1; >>> + u8 tx_nbits; >>> + u8 rx_nbits; >>> +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ >>> +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ >>> +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ >> >> These fields increase the size of a spi_transfer by 4 bytes. If you >> used bitfields instead it wouldn't increase the size at all since >> there are still 7 bits left after cs_change. > Yes, it will increase the size by 4 bytes, but using bitfields here > will make it logically weird. Bitfields may mean that really have the > restriction of number of bits. If just for saving memory, then what > about the other members such as bits_per_world, delay_usecs. Rather than just talk about it, I'll make patches to do it. Then it will be clear. On Wed, Sep 4, 2013 at 12:07 AM, yuhang wang <wangyuhang2014@gmail.com> wrote: > 2013/9/4 Trent Piepho <tpiepho@gmail.com>: >> On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2014@gmail.com> wrote: >>> fix the previous patch some mistake below: >> >> This isn't a very good commit message. "the previous patch" will have >> no meaning in the kernel git repo. The rest of the message only >> describes changes since a previous version of the patch and not the >> actual patch in full. >> >>> >>> + /* Device DUAL/QUAD mode */ >>> + prop = of_get_property(nc, "spi-tx-nbits", &len); >> >> Why not use of_property_read_u32() here? >> >>> + if (!prop || len < sizeof(*prop)) { >>> + dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", >>> + nc->full_name); >>> + spi_dev_put(spi); >>> + continue; >> >> So if no spi-tx-nbits property is supplied, the device is rejected and >> the loop continues to the next device entry? This means ALL EXISTING >> DEVICE TREES with SPI devices will be rejected, since none of them >> have this new property! Was this patch tested at all with any system >> before being accepted? >> > This part has been fixed in patch[commit > id:a822e99c70f448c4068ea85bb195dac0b2eb3afe] > >>> + /* check mode to prevent that DUAL and QUAD set at the same time >>> + */ >>> + if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) || >>> + ((spi->mode & SPI_RX_DUAL) && (spi->mode & SPI_RX_QUAD))) { >>> + dev_err(&spi->dev, >>> + "setup: can not select dual and quad at the same time\n"); >>> + return -EINVAL; >> >> This test can be done with fewer operations as: >> if ((spi->mode & (SPI_TX_DUAL|SPI_TX_QUAD)) == (SPI_TX_DUAL|SPI_TX_QUAD) || >> (spi->mode & (SPI_RX_DUAL|SPI_RX_QUAD)) == (SPI_RX_DUAL|SPI_RX_QUAD)) { >> >> >>> + } >>> + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden >>> + */ >>> + if ((spi->mode & SPI_3WIRE) && (spi->mode & >>> + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD))) >>> + return -EINVAL; >> >> No dev_err message for this possibility? What's different than the >> previous check that does produce a message? >> > OK, should be added. Thanks. > >>> /* help drivers fail *cleanly* when they need options >>> * that aren't supported with their current master >>> */ >> Following this comment is the code: >> bad_bits = spi->mode & ~spi->master->mode_bits; >> if (bad_bits) { >> dev_err(&spi->dev, "setup: unsupported mode bits %x\n", >> bad_bits); >> return -EINVAL; >> } >> >> Won't this always trigger for anything that sets one of the dual or quad bits? >> > I can't get your idea clearly. Please provide more infomation. > >>> @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) >>> /** >>> * Set transfer bits_per_word and max speed as spi device default if >>> * it is not set for this transfer. >>> + * Set transfer tx_nbits and rx_nbits as single transfer default >>> + * (SPI_NBITS_SINGLE) if it is not set for this transfer. >>> */ >>> list_for_each_entry(xfer, &message->transfers, transfer_list) { >>> if (!xfer->bits_per_word) >>> xfer->bits_per_word = spi->bits_per_word; >>> if (!xfer->speed_hz) >>> xfer->speed_hz = spi->max_speed_hz; >>> + if (xfer->tx_buf && !xfer->tx_nbits) >>> + xfer->tx_nbits = SPI_NBITS_SINGLE; >>> + if (xfer->rx_buf && !xfer->rx_nbits) >>> + xfer->rx_nbits = SPI_NBITS_SINGLE; >>> + /* check transfer tx/rx_nbits: >>> + * 1. keep the value is not out of single, dual and quad >>> + * 2. keep tx/rx_nbits is contained by mode in spi_device >>> + * 3. if SPI_3WIRE, tx/rx_nbits should be in single >>> + */ >>> + if (xfer->tx_nbits != SPI_NBITS_SINGLE && >>> + xfer->tx_nbits != SPI_NBITS_DUAL && >>> + xfer->tx_nbits != SPI_NBITS_QUAD) >>> + return -EINVAL; >>> + if ((xfer->tx_nbits == SPI_NBITS_DUAL) && >>> + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD))) >>> + return -EINVAL; >>> + if ((xfer->tx_nbits == SPI_NBITS_QUAD) && >>> + !(spi->mode & SPI_TX_QUAD)) >>> + return -EINVAL; >>> + if ((spi->mode & SPI_3WIRE) && >>> + (xfer->tx_nbits != SPI_NBITS_SINGLE)) >>> + return -EINVAL; >>> + /* check transfer rx_nbits */ >>> + if (xfer->rx_nbits != SPI_NBITS_SINGLE && >>> + xfer->rx_nbits != SPI_NBITS_DUAL && >>> + xfer->rx_nbits != SPI_NBITS_QUAD) >>> + return -EINVAL; >>> + if ((xfer->rx_nbits == SPI_NBITS_DUAL) && >>> + !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD))) >>> + return -EINVAL; >>> + if ((xfer->rx_nbits == SPI_NBITS_QUAD) && >>> + !(spi->mode & SPI_RX_QUAD)) >>> + return -EINVAL; >>> + if ((spi->mode & SPI_3WIRE) && >>> + (xfer->rx_nbits != SPI_NBITS_SINGLE)) >>> + return -EINVAL; >>> } >> >> What a lot of code to check the transfer bits. It really needs this much? >> > This part is also corrected in patch [commit > id:db90a44177ac39fc22b2da5235b231fccdd4c673]. > >>> @@ -511,6 +524,11 @@ struct spi_transfer { >>> dma_addr_t rx_dma; >>> >>> unsigned cs_change:1; >>> + u8 tx_nbits; >>> + u8 rx_nbits; >>> +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ >>> +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ >>> +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ >> >> These fields increase the size of a spi_transfer by 4 bytes. If you >> used bitfields instead it wouldn't increase the size at all since >> there are still 7 bits left after cs_change. > Yes, it will increase the size by 4 bytes, but using bitfields here > will make it logically weird. Bitfields may mean that really have the > restriction of number of bits. If just for saving memory, then what > about the other members such as bits_per_world, delay_usecs.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 004b10f..78f257a 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -869,6 +869,51 @@ static void of_register_spi_devices(struct spi_master *master) if (of_find_property(nc, "spi-3wire", NULL)) spi->mode |= SPI_3WIRE; + /* Device DUAL/QUAD mode */ + prop = of_get_property(nc, "spi-tx-nbits", &len); + if (!prop || len < sizeof(*prop)) { + dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", + nc->full_name); + spi_dev_put(spi); + continue; + } + switch(be32_to_cpup(prop)) { + case SPI_NBITS_SINGLE: + break; + case SPI_NBITS_DUAL: + spi->mode |= SPI_TX_DUAL; + break; + case SPI_NBITS_QUAD: + spi->mode |= SPI_TX_QUAD; + break; + default: + dev_err(&master->dev, "spi-tx-nbits value is not supported\n"); + spi_dev_put(spi); + continue; + } + + prop = of_get_property(nc, "spi-rx-nbits", &len); + if (!prop || len < sizeof(*prop)) { + dev_err(&master->dev, "%s has no 'spi-rx-nbits' property\n", + nc->full_name); + spi_dev_put(spi); + continue; + } + switch(be32_to_cpup(prop)) { + case SPI_NBITS_SINGLE: + break; + case SPI_NBITS_DUAL: + spi->mode |= SPI_RX_DUAL; + break; + case SPI_NBITS_QUAD: + spi->mode |= SPI_RX_QUAD; + break; + default: + dev_err(&master->dev, "spi-rx-nbits value is not supported\n"); + spi_dev_put(spi); + continue; + } + /* Device speed */ prop = of_get_property(nc, "spi-max-frequency", &len); if (!prop || len < sizeof(*prop)) { @@ -1313,6 +1358,19 @@ int spi_setup(struct spi_device *spi) unsigned bad_bits; int status = 0; + /* check mode to prevent that DUAL and QUAD set at the same time + */ + if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) || + ((spi->mode & SPI_RX_DUAL) && (spi->mode & SPI_RX_QUAD))) { + dev_err(&spi->dev, + "setup: can not select dual and quad at the same time\n"); + return -EINVAL; + } + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden + */ + if ((spi->mode & SPI_3WIRE) && (spi->mode & + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD))) + return -EINVAL; /* help drivers fail *cleanly* when they need options * that aren't supported with their current master */ @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi, struct spi_message *message) /** * Set transfer bits_per_word and max speed as spi device default if * it is not set for this transfer. + * Set transfer tx_nbits and rx_nbits as single transfer default + * (SPI_NBITS_SINGLE) if it is not set for this transfer. */ list_for_each_entry(xfer, &message->transfers, transfer_list) { if (!xfer->bits_per_word) xfer->bits_per_word = spi->bits_per_word; if (!xfer->speed_hz) xfer->speed_hz = spi->max_speed_hz; + if (xfer->tx_buf && !xfer->tx_nbits) + xfer->tx_nbits = SPI_NBITS_SINGLE; + if (xfer->rx_buf && !xfer->rx_nbits) + xfer->rx_nbits = SPI_NBITS_SINGLE; + /* check transfer tx/rx_nbits: + * 1. keep the value is not out of single, dual and quad + * 2. keep tx/rx_nbits is contained by mode in spi_device + * 3. if SPI_3WIRE, tx/rx_nbits should be in single + */ + if (xfer->tx_nbits != SPI_NBITS_SINGLE && + xfer->tx_nbits != SPI_NBITS_DUAL && + xfer->tx_nbits != SPI_NBITS_QUAD) + return -EINVAL; + if ((xfer->tx_nbits == SPI_NBITS_DUAL) && + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD))) + return -EINVAL; + if ((xfer->tx_nbits == SPI_NBITS_QUAD) && + !(spi->mode & SPI_TX_QUAD)) + return -EINVAL; + if ((spi->mode & SPI_3WIRE) && + (xfer->tx_nbits != SPI_NBITS_SINGLE)) + return -EINVAL; + /* check transfer rx_nbits */ + if (xfer->rx_nbits != SPI_NBITS_SINGLE && + xfer->rx_nbits != SPI_NBITS_DUAL && + xfer->rx_nbits != SPI_NBITS_QUAD) + return -EINVAL; + if ((xfer->rx_nbits == SPI_NBITS_DUAL) && + !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD))) + return -EINVAL; + if ((xfer->rx_nbits == SPI_NBITS_QUAD) && + !(spi->mode & SPI_RX_QUAD)) + return -EINVAL; + if ((spi->mode & SPI_3WIRE) && + (xfer->rx_nbits != SPI_NBITS_SINGLE)) + return -EINVAL; } message->spi = spi; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 38c2b92..3dc0a86 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; #define SPI_CPHA 0x01 /* clock phase */ #define SPI_CPOL 0x02 /* clock polarity */ #define SPI_MODE_0 (0|0) /* (original MicroWire) */ @@ -87,6 +87,10 @@ struct spi_device { #define SPI_LOOP 0x20 /* loopback mode */ #define SPI_NO_CS 0x40 /* 1 dev/bus, no chipselect */ #define SPI_READY 0x80 /* slave pulls low to pause */ +#define SPI_TX_DUAL 0x100 /* transmit with 2 wires */ +#define SPI_TX_QUAD 0x200 /* transmit with 4 wires */ +#define SPI_RX_DUAL 0x400 /* receive with 2 wires */ +#define SPI_RX_QUAD 0x800 /* receive with 4 wires */ u8 bits_per_word; int irq; void *controller_state; @@ -437,6 +441,10 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum); * @rx_buf: data to be read (dma-safe memory), or NULL * @tx_dma: DMA address of tx_buf, if @spi_message.is_dma_mapped * @rx_dma: DMA address of rx_buf, if @spi_message.is_dma_mapped + * @tx_nbits: number of bits used for writting. If 0 the default + * (SPI_NBITS_SINGLE) is used. + * @rx_nbits: number of bits used for reading. If 0 the default + * (SPI_NBITS_SINGLE) is used. * @len: size of rx and tx buffers (in bytes) * @speed_hz: Select a speed other than the device default for this * transfer. If 0 the default (from @spi_device) is used. @@ -491,6 +499,11 @@ extern struct spi_master *spi_busnum_to_master(u16 busnum); * by the results of previous messages and where the whole transaction * ends when the chipselect goes intactive. * + * When SPI can transfer in 1x,2x or 4x. It can get this tranfer information + * from device through @tx_nbits and @rx_nbits. In Bi-direction, these + * two should both be set. User can set transfer mode with SPI_NBITS_SINGLE(1x) + * SPI_NBITS_DUAL(2x) and SPI_NBITS_QUAD(4x) to support these three transfer. + * * The code that submits an spi_message (and its spi_transfers) * to the lower layers is responsible for managing its memory. * Zero-initialize every field you don't set up explicitly, to @@ -511,6 +524,11 @@ struct spi_transfer { dma_addr_t rx_dma; unsigned cs_change:1; + u8 tx_nbits; + u8 rx_nbits; +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */ +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */ +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */ u8 bits_per_word; u16 delay_usecs; u32 speed_hz; @@ -858,7 +876,7 @@ struct spi_board_info { /* mode becomes spi_device.mode, and is essential for chips * where the default of SPI_CS_HIGH = 0 is wrong. */ - u8 mode; + u16 mode; /* ... may need additional spi_device chip config data here. * avoid stuff protocol drivers can set; but include stuff
fix the previous patch some mistake below: 1. DT in slave node, use "spi-tx-nbits = <1/2/4>" in place of using "spi-tx-dual, spi-tx-quad" directly, same to rx. So correct the previous way to get the property in @of_register_spi_devices(). 2. Change the value of transfer bit macro(SPI_NBITS_SINGLE, SPI_NBITS_DUAL SPI_NBITS_QUAD) to 0x01, 0x02 and 0x04 to match the actual wires. 3. Add the following check (1)keep the tx_nbits and rx_nbits in spi_transfer is not beyond the single, dual and quad. (2)keep tx_nbits and rx_nbits are contained by @spi_device->mode example: if @spi_device->mode = DUAL, then tx/rx_nbits can not be set to QUAD(SPI_NBITS_QUAD) (3)if "@spi_device->mode & SPI_3WIRE", then tx/rx_nbits should be in single(SPI_NBITS_SINGLE) Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> --- drivers/spi/spi.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/spi/spi.h | 22 ++++++++++- 2 files changed, 116 insertions(+), 2 deletions(-)