SPI: DUAL and QUAD support

Hi,

modify two things.
1:
>> @@ -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;
>> +       }
>> +               return -EINVAL;
>> +       }

>This code won't work if the constants you added for mode flags change
>value.  More importantly, anyone searching the code for SPI_TX_DUAL,
>etc. won't find this code.

so use the certain macro to replace.

2:
>You need to make sure that existing userspace binaries can run
>unmodified, changing the types will change the layout of the ioctl
>arguments.  This may mean that you need to add new ioctls if you need to
>change types.

I recovered the SPI_IOC_RD_LSB_FIRST ioctl.
Add new ioctl to fit u16 mode.
In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way.

Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
---
 drivers/spi/spi.c               |   14 ++++++++++++++
 drivers/spi/spidev.c            |   29 ++++++++++++++++++++++++++++-
 include/linux/spi/spi.h         |   20 ++++++++++++++++++--
 include/uapi/linux/spi/spidev.h |   17 ++++++++++++++++-
 4 files changed, 76 insertions(+), 4 deletions(-)

Hello,

> 
> Hi,
> 
> modify two things.
Hope you are tracking the Patch versions ?  Good to include it in mail-subject.

> 1:
> >> @@ -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;
> >> +       }
> >> +               return -EINVAL;
> >> +       }
> 
> >This code won't work if the constants you added for mode flags change
> >value.  More importantly, anyone searching the code for SPI_TX_DUAL,
> >etc. won't find this code.
> 
> so use the certain macro to replace.
> 
> 2:
> >You need to make sure that existing userspace binaries can run
> >unmodified, changing the types will change the layout of the ioctl
> >arguments.  This may mean that you need to add new ioctls if you need to
> >change types.
> 
> I recovered the SPI_IOC_RD_LSB_FIRST ioctl.
> Add new ioctl to fit u16 mode.
> In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way.
> 
> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
> ---
>  drivers/spi/spi.c               |   14 ++++++++++++++
>  drivers/spi/spidev.c            |   29 ++++++++++++++++++++++++++++-
>  include/linux/spi/spi.h         |   20 ++++++++++++++++++--
>  include/uapi/linux/spi/spidev.h |   17 ++++++++++++++++-
>  4 files changed, 76 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 004b10f..3f6dc93 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;
> 
 [Pekon] I think its cleaner to use similar parameters for channel-width
 info everywhere. Like you have added separate fields  'tx_nbits' and
 'rx_nbits' in spi_transfer, Why not do similar here for 'spi_master' ?
And also in 'spi_device'. This can be like below:

- SPI_3WIRE : tx_nbit = 0, rx_nbit = 0 (single wire for MISO & MOSI)
- SPI_4WIRE :  tx_nbit = 1, rx_nbit = 1 (separate wire for MISO & MOSI)
- DUAL_SPI :   tx_nbit = 2, rx_nbit = 2
- QUAD_SPI : tx_nbit = 2 or 4, rx_nbit = 4
This way 'mode' field can be explicitly left for other purposes like CPHA, CPOL.
OR custom modes (like modified SPI mode in Freescale controllers).

But if you are touching any existing configuration (like SPI_3WIRE as
proposed  above), Then please send it as independent patch/proposal,
which can be Acked by individual SPI driver maintainers.

[Pekon]: I think it is correct to assume all these SPI modes as mutually
exclusive from of_property() point of view? 
Instead of defining separate of_properties(), you can define a single
of_property() , something like 'max-tx-nbits' and 'max-rx-nbits'
which can represent various modes like (DUAL, QUAD, 3WIRE, 4WIRE).

>  		/* 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 & 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;
> +	}
[Pekon]: this check would be not be required if you have single
 of_property() for all modes.


>  	bad_bits = spi->mode & ~spi->master->mode_bits;
>  	if (bad_bits) {
>  		dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index 2e0655d..0b776de 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -75,6 +75,9 @@ static DECLARE_BITMAP(minors, N_SPI_MINORS);
>  				| SPI_LSB_FIRST | SPI_3WIRE | SPI_LOOP \
>  				| SPI_NO_CS | SPI_READY)
> 
> +#define SPI_EXTMODE_MASK	(SPI_MODE_MASK | SPI_TX_DUAL \
> +				| SPI_TX_QUAD | SPI_RX_DUAL |
> SPI_RX_QUAD)
> +
>  struct spidev_data {
>  	dev_t			devt;
>  	spinlock_t		spi_lock;
> @@ -268,6 +271,8 @@ static int spidev_message(struct spidev_data *spidev,
>  		k_tmp->bits_per_word = u_tmp->bits_per_word;
>  		k_tmp->delay_usecs = u_tmp->delay_usecs;
>  		k_tmp->speed_hz = u_tmp->speed_hz;
> +		k_tmp->tx_nbits = u_tmp->tx_nbits;
> +		k_tmp->rx_nbits = u_tmp->rx_nbits;
>  #ifdef VERBOSE
>  		dev_dbg(&spidev->spi->dev,
>  			"  xfer len %zd %s%s%s%dbits %u usec %uHz\n",
> @@ -369,7 +374,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg)
>  	case SPI_IOC_RD_MAX_SPEED_HZ:
>  		retval = __put_user(spi->max_speed_hz, (__u32 __user
> *)arg);
>  		break;
> -
> +	case SPI_IOC_EXTRD_MODE:
> +		retval = __put_user(spi->mode & SPI_EXTMODE_MASK,
> +					(__u16 __user *)arg);
> +		break;
>  	/* write requests */
>  	case SPI_IOC_WR_MODE:
>  		retval = __get_user(tmp, (u8 __user *)arg);
> @@ -433,6 +441,25 @@ spidev_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg)
>  				dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
>  		}
>  		break;
> +	case SPI_IOC_EXTWR_MODE:
> +		retval = __get_user(tmp, (u16 __user *)arg);
> +		if (retval == 0) {
> +			u16	save = spi->mode;
> +
> +			if (tmp & ~SPI_EXTMODE_MASK) {
> +				retval = -EINVAL;
> +				break;
> +			}
> +
> +			tmp |= spi->mode & ~SPI_EXTMODE_MASK;
> +			spi->mode = (u16)tmp;
> +			retval = spi_setup(spi);
> +			if (retval < 0)
> +				spi->mode = save;
> +			else
> +				dev_dbg(&spi->dev, "spi mode %02x\n",
> tmp);
> +		}
> +		break;
> 
>  	default:
>  		/* segmented and/or full-duplex I/O request */
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 38c2b92..222e49e 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,8 @@ 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
> + * @rx_nbits: number of bits used for reading
>   * @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 +497,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 +522,11 @@ struct spi_transfer {
>  	dma_addr_t	rx_dma;
> 
>  	unsigned	cs_change:1;
> +	u8		tx_nbits;
> +	u8		rx_nbits;
> +#define	SPI_NBITS_SINGLE	0x0; /* 1bit transfer */
> +#define	SPI_NBITS_DUAL		0x01; /* 2bits transfer */
> +#define	SPI_NBITS_QUAD		0x02; /* 4bits transfer */

[Pekon]: I don't think it's a good place for #defines.  Plz keep it out of struct body.

>  	u8		bits_per_word;
>  	u16		delay_usecs;
>  	u32		speed_hz;
> @@ -858,7 +874,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


With regards, pekon

Le Mon, 29 Jul 2013 11:53:14 +0100,
wangyuhang <wangyuhang2014@gmail.com> a écrit :

> Hi,
> 
> modify two things.
> 1:
> >> @@ -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;
> >> +       }
> >> +               return -EINVAL;
> >> +       }
> 
> >This code won't work if the constants you added for mode flags change
> >value.  More importantly, anyone searching the code for SPI_TX_DUAL,
> >etc. won't find this code.
> 
> so use the certain macro to replace.
> 
> 2:
> >You need to make sure that existing userspace binaries can run
> >unmodified, changing the types will change the layout of the ioctl
> >arguments.  This may mean that you need to add new ioctls if you
> >need to change types.
> 
> I recovered the SPI_IOC_RD_LSB_FIRST ioctl.
> Add new ioctl to fit u16 mode.
> In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way.
> 
> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
> ---

>  /* IOCTL commands */
> @@ -54,6 +61,8 @@
>   * @tx_buf: Holds pointer to userspace buffer with transmit data, or
> null.
>   *     If no data is provided, zeroes are shifted out.
>   * @rx_buf: Holds pointer to userspace buffer for receive data, or
> null.
> + * @tx_nbits: number of bits used for writting.
> + * @rx_nbits: number of bits used for reading.
>   * @len: Length of tx and rx buffers, in bytes.
>   * @speed_hz: Temporary override of the device's bitrate.
>   * @bits_per_word: Temporary override of the device's wordsize.
> @@ -85,6 +94,8 @@
>  struct spi_ioc_transfer {
>         __u64           tx_buf;
>         __u64           rx_buf;
> +       __u8            tx_nbits;
> +       __u8            rx_nbits;
> 
>         __u32           len;
>         __u32           speed_hz;
You still break old userspace ABI.

You need to create a new structure, if you need to add fields.


Matthieu

Hi, Pekon

2013/7/29 Gupta, Pekon <pekon@ti.com>:
> Hello,
>
>>
>> Hi,
>>
>> modify two things.
> Hope you are tracking the Patch versions ?  Good to include it in mail-subject.
>
Ok, got it.

>> 1:
>> >> @@ -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;
>> >> +       }
>> >> +               return -EINVAL;
>> >> +       }
>>
>> >This code won't work if the constants you added for mode flags change
>> >value.  More importantly, anyone searching the code for SPI_TX_DUAL,
>> >etc. won't find this code.
>>
>> so use the certain macro to replace.
>>
>> 2:
>> >You need to make sure that existing userspace binaries can run
>> >unmodified, changing the types will change the layout of the ioctl
>> >arguments.  This may mean that you need to add new ioctls if you need to
>> >change types.
>>
>> I recovered the SPI_IOC_RD_LSB_FIRST ioctl.
>> Add new ioctl to fit u16 mode.
>> In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way.
>>
>> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
>> ---
>>  drivers/spi/spi.c               |   14 ++++++++++++++
>>  drivers/spi/spidev.c            |   29 ++++++++++++++++++++++++++++-
>>  include/linux/spi/spi.h         |   20 ++++++++++++++++++--
>>  include/uapi/linux/spi/spidev.h |   17 ++++++++++++++++-
>>  4 files changed, 76 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> index 004b10f..3f6dc93 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;
>>
>  [Pekon] I think its cleaner to use similar parameters for channel-width
>  info everywhere. Like you have added separate fields  'tx_nbits' and
>  'rx_nbits' in spi_transfer, Why not do similar here for 'spi_master' ?
> And also in 'spi_device'. This can be like below:
>
> - SPI_3WIRE : tx_nbit = 0, rx_nbit = 0 (single wire for MISO & MOSI)
> - SPI_4WIRE :  tx_nbit = 1, rx_nbit = 1 (separate wire for MISO & MOSI)
> - DUAL_SPI :   tx_nbit = 2, rx_nbit = 2
> - QUAD_SPI : tx_nbit = 2 or 4, rx_nbit = 4
> This way 'mode' field can be explicitly left for other purposes like CPHA, CPOL.
> OR custom modes (like modified SPI mode in Freescale controllers).
>
> But if you are touching any existing configuration (like SPI_3WIRE as
> proposed  above), Then please send it as independent patch/proposal,
> which can be Acked by individual SPI driver maintainers.
>
Adding the similar members like 'tx_nbits' and 'rx_nbits' is my
initial thought. But in different struct, that members meaning may be
a little different.
In spi_master, the member is what transfer mode master can support.
In spi_device, the member is what mode the slave will use.
In spi_transfer, the member is the transfer wires needed.
1: adding the similar members everywhere may looks a little redundant.
What is more, in spi_master and spi_device, it is more like a mode and
in spi_device it is a transfer function due to the mode.
example:
DUAL mode in spi_device, in spi_transfer tx_nbits or rx_nbits can be
set to 1 or 2.
2: the member in spi_master has to describe all the transfer
mode(single/dual/quad), because the member means the mode that master
support, so the member has to be seperated into several bits to cover
all the single dual and quad and then check it to spi_device. This
also seems similar to what the existed spi_master->mode_bit and
spi_device->mode do.
so added in mode may looks better.

> [Pekon]: I think it is correct to assume all these SPI modes as mutually
> exclusive from of_property() point of view?
> Instead of defining separate of_properties(), you can define a single
> of_property() , something like 'max-tx-nbits' and 'max-rx-nbits'
> which can represent various modes like (DUAL, QUAD, 3WIRE, 4WIRE).
>
Yes, seems better. I will correct it.

>>               /* 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 & 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;
>> +     }
> [Pekon]: this check would be not be required if you have single
>  of_property() for all modes.
>
>
>>       bad_bits = spi->mode & ~spi->master->mode_bits;
>>       if (bad_bits) {
>>               dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
>> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
>> index 2e0655d..0b776de 100644
>> --- a/drivers/spi/spidev.c
>> +++ b/drivers/spi/spidev.c
>> @@ -75,6 +75,9 @@ static DECLARE_BITMAP(minors, N_SPI_MINORS);
>>                               | SPI_LSB_FIRST | SPI_3WIRE | SPI_LOOP \
>>                               | SPI_NO_CS | SPI_READY)
>>
>> +#define SPI_EXTMODE_MASK     (SPI_MODE_MASK | SPI_TX_DUAL \
>> +                             | SPI_TX_QUAD | SPI_RX_DUAL |
>> SPI_RX_QUAD)
>> +
>>  struct spidev_data {
>>       dev_t                   devt;
>>       spinlock_t              spi_lock;
>> @@ -268,6 +271,8 @@ static int spidev_message(struct spidev_data *spidev,
>>               k_tmp->bits_per_word = u_tmp->bits_per_word;
>>               k_tmp->delay_usecs = u_tmp->delay_usecs;
>>               k_tmp->speed_hz = u_tmp->speed_hz;
>> +             k_tmp->tx_nbits = u_tmp->tx_nbits;
>> +             k_tmp->rx_nbits = u_tmp->rx_nbits;
>>  #ifdef VERBOSE
>>               dev_dbg(&spidev->spi->dev,
>>                       "  xfer len %zd %s%s%s%dbits %u usec %uHz\n",
>> @@ -369,7 +374,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>>       case SPI_IOC_RD_MAX_SPEED_HZ:
>>               retval = __put_user(spi->max_speed_hz, (__u32 __user
>> *)arg);
>>               break;
>> -
>> +     case SPI_IOC_EXTRD_MODE:
>> +             retval = __put_user(spi->mode & SPI_EXTMODE_MASK,
>> +                                     (__u16 __user *)arg);
>> +             break;
>>       /* write requests */
>>       case SPI_IOC_WR_MODE:
>>               retval = __get_user(tmp, (u8 __user *)arg);
>> @@ -433,6 +441,25 @@ spidev_ioctl(struct file *filp, unsigned int cmd,
>> unsigned long arg)
>>                               dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
>>               }
>>               break;
>> +     case SPI_IOC_EXTWR_MODE:
>> +             retval = __get_user(tmp, (u16 __user *)arg);
>> +             if (retval == 0) {
>> +                     u16     save = spi->mode;
>> +
>> +                     if (tmp & ~SPI_EXTMODE_MASK) {
>> +                             retval = -EINVAL;
>> +                             break;
>> +                     }
>> +
>> +                     tmp |= spi->mode & ~SPI_EXTMODE_MASK;
>> +                     spi->mode = (u16)tmp;
>> +                     retval = spi_setup(spi);
>> +                     if (retval < 0)
>> +                             spi->mode = save;
>> +                     else
>> +                             dev_dbg(&spi->dev, "spi mode %02x\n",
>> tmp);
>> +             }
>> +             break;
>>
>>       default:
>>               /* segmented and/or full-duplex I/O request */
>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
>> index 38c2b92..222e49e 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,8 @@ 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
>> + * @rx_nbits: number of bits used for reading
>>   * @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 +497,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 +522,11 @@ struct spi_transfer {
>>       dma_addr_t      rx_dma;
>>
>>       unsigned        cs_change:1;
>> +     u8              tx_nbits;
>> +     u8              rx_nbits;
>> +#define      SPI_NBITS_SINGLE        0x0; /* 1bit transfer */
>> +#define      SPI_NBITS_DUAL          0x01; /* 2bits transfer */
>> +#define      SPI_NBITS_QUAD          0x02; /* 4bits transfer */
>
> [Pekon]: I don't think it's a good place for #defines.  Plz keep it out of struct body.
>
>>       u8              bits_per_word;
>>       u16             delay_usecs;
>>       u32             speed_hz;
>> @@ -858,7 +874,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
>
>
> With regards, pekon

Hi Yuhang,

> >>
> >> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> >> index 004b10f..3f6dc93 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;
> >>
> >  [Pekon] I think its cleaner to use similar parameters for channel-width
> >  info everywhere. Like you have added separate fields  'tx_nbits' and
> >  'rx_nbits' in spi_transfer, Why not do similar here for 'spi_master' ?
> > And also in 'spi_device'. This can be like below:
> >
> > - SPI_3WIRE : tx_nbit = 0, rx_nbit = 0 (single wire for MISO & MOSI)
> > - SPI_4WIRE :  tx_nbit = 1, rx_nbit = 1 (separate wire for MISO & MOSI)
> > - DUAL_SPI :   tx_nbit = 2, rx_nbit = 2
> > - QUAD_SPI : tx_nbit = 2 or 4, rx_nbit = 4
> > This way 'mode' field can be explicitly left for other purposes like CPHA,
> CPOL.
> > OR custom modes (like modified SPI mode in Freescale controllers).
> >
> > But if you are touching any existing configuration (like SPI_3WIRE as
> > proposed  above), Then please send it as independent patch/proposal,
> > which can be Acked by individual SPI driver maintainers.
> >
> Adding the similar members like 'tx_nbits' and 'rx_nbits' is my
> initial thought. But in different struct, that members meaning may be
> a little different.
> In spi_master, the member is what transfer mode master can support.
> In spi_device, the member is what mode the slave will use.
> In spi_transfer, the member is the transfer wires needed.
> 1: adding the similar members everywhere may looks a little redundant.
> What is more, in spi_master and spi_device, it is more like a mode and
> in spi_device it is a transfer function due to the mode.
> example:
> DUAL mode in spi_device, in spi_transfer tx_nbits or rx_nbits can be
> set to 1 or 2.
> 2: the member in spi_master has to describe all the transfer
> mode(single/dual/quad), because the member means the mode that master
> support, so the member has to be seperated into several bits to cover
> all the single dual and quad and then check it to spi_device. This
> also seems similar to what the existed spi_master->mode_bit and
> spi_device->mode do.
> so added in mode may looks better.
> 
[Pekon]: Just review following scenarios .. 
_Case-1_: I have a controller which supports
- Quad & Dual modes only for reads,
- Only Single SPI for writes.
 This means though bit (spi_master.mode & QUAD_MODE)
is set, but still if MTD layer does a SPI WRITE transfer using tx_nbits=4, 
I should get error..

_Case-2_: Some flash devices also have similar constrains.
Example: check Spansion flash S25FL128S datasheet,
 "section 10.1 command summary". This device supports
- Single/Dual/Quad READS,
- but only SINGLE & QUAD page-programs (WRITES).

So, if you have only one field ('mode') for storing capability of device or
master, you cannot resolve above scenarios. This is where I realized that
 we need to have separate tx_nbits and rx_nbits in spi_device and
 spi_master also. (This is contrary to my own earliest posts where I 
proposed reusing mode bits for keeping channel-width info).



> > [Pekon]: I think it is correct to assume all these SPI modes as mutually
> > exclusive from of_property() point of view?
> > Instead of defining separate of_properties(), you can define a single
> > of_property() , something like 'max-tx-nbits' and 'max-rx-nbits'
> > which can represent various modes like (DUAL, QUAD, 3WIRE, 4WIRE).
> >
> Yes, seems better. I will correct it.
> 
> >>               /* 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 & 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;
> >> +     }
> > [Pekon]: this check would be not be required if you have single
> >  of_property() for all modes.
> >
> >
> >>       bad_bits = spi->mode & ~spi->master->mode_bits;
> >>       if (bad_bits) {
> >>               dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
> >> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> >> index 2e0655d..0b776de 100644
> >> --- a/drivers/spi/spidev.c
> >> +++ b/drivers/spi/spidev.c
> >> @@ -75,6 +75,9 @@ static DECLARE_BITMAP(minors, N_SPI_MINORS);
> >>                               | SPI_LSB_FIRST | SPI_3WIRE | SPI_LOOP \
> >>                               | SPI_NO_CS | SPI_READY)
> >>
> >> +#define SPI_EXTMODE_MASK     (SPI_MODE_MASK | SPI_TX_DUAL \
> >> +                             | SPI_TX_QUAD | SPI_RX_DUAL |
> >> SPI_RX_QUAD)
> >> +
> >>  struct spidev_data {
> >>       dev_t                   devt;
> >>       spinlock_t              spi_lock;
> >> @@ -268,6 +271,8 @@ static int spidev_message(struct spidev_data
> *spidev,
> >>               k_tmp->bits_per_word = u_tmp->bits_per_word;
> >>               k_tmp->delay_usecs = u_tmp->delay_usecs;
> >>               k_tmp->speed_hz = u_tmp->speed_hz;
> >> +             k_tmp->tx_nbits = u_tmp->tx_nbits;
> >> +             k_tmp->rx_nbits = u_tmp->rx_nbits;
> >>  #ifdef VERBOSE
> >>               dev_dbg(&spidev->spi->dev,
> >>                       "  xfer len %zd %s%s%s%dbits %u usec %uHz\n",
> >> @@ -369,7 +374,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd,
> >> unsigned long arg)
> >>       case SPI_IOC_RD_MAX_SPEED_HZ:
> >>               retval = __put_user(spi->max_speed_hz, (__u32 __user
> >> *)arg);
> >>               break;
> >> -
> >> +     case SPI_IOC_EXTRD_MODE:
> >> +             retval = __put_user(spi->mode & SPI_EXTMODE_MASK,
> >> +                                     (__u16 __user *)arg);
> >> +             break;
> >>       /* write requests */
> >>       case SPI_IOC_WR_MODE:
> >>               retval = __get_user(tmp, (u8 __user *)arg);
> >> @@ -433,6 +441,25 @@ spidev_ioctl(struct file *filp, unsigned int cmd,
> >> unsigned long arg)
> >>                               dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
> >>               }
> >>               break;
> >> +     case SPI_IOC_EXTWR_MODE:
> >> +             retval = __get_user(tmp, (u16 __user *)arg);
> >> +             if (retval == 0) {
> >> +                     u16     save = spi->mode;
> >> +
> >> +                     if (tmp & ~SPI_EXTMODE_MASK) {
> >> +                             retval = -EINVAL;
> >> +                             break;
> >> +                     }
> >> +
> >> +                     tmp |= spi->mode & ~SPI_EXTMODE_MASK;
> >> +                     spi->mode = (u16)tmp;
> >> +                     retval = spi_setup(spi);
> >> +                     if (retval < 0)
> >> +                             spi->mode = save;
> >> +                     else
> >> +                             dev_dbg(&spi->dev, "spi mode %02x\n",
> >> tmp);
> >> +             }
> >> +             break;
> >>
> >>       default:
> >>               /* segmented and/or full-duplex I/O request */
> >> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> >> index 38c2b92..222e49e 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,8 @@ 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
> >> + * @rx_nbits: number of bits used for reading
> >>   * @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 +497,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 +522,11 @@ struct spi_transfer {
> >>       dma_addr_t      rx_dma;
> >>
> >>       unsigned        cs_change:1;
> >> +     u8              tx_nbits;
> >> +     u8              rx_nbits;
> >> +#define      SPI_NBITS_SINGLE        0x0; /* 1bit transfer */
> >> +#define      SPI_NBITS_DUAL          0x01; /* 2bits transfer */
> >> +#define      SPI_NBITS_QUAD          0x02; /* 4bits transfer */
> >
> > [Pekon]: I don't think it's a good place for #defines.  Plz keep it out of struct
> body.
> >
> >>       u8              bits_per_word;
> >>       u16             delay_usecs;
> >>       u32             speed_hz;
> >> @@ -858,7 +874,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
> >
> >
with regards, pekon

Hi,

2013/7/29 Matthieu CASTET <matthieu.castet@parrot.com>:
> Le Mon, 29 Jul 2013 11:53:14 +0100,
> wangyuhang <wangyuhang2014@gmail.com> a écrit :
>
>> Hi,
>>
>> modify two things.
>> 1:
>> >> @@ -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;
>> >> +       }
>> >> +               return -EINVAL;
>> >> +       }
>>
>> >This code won't work if the constants you added for mode flags change
>> >value.  More importantly, anyone searching the code for SPI_TX_DUAL,
>> >etc. won't find this code.
>>
>> so use the certain macro to replace.
>>
>> 2:
>> >You need to make sure that existing userspace binaries can run
>> >unmodified, changing the types will change the layout of the ioctl
>> >arguments.  This may mean that you need to add new ioctls if you
>> >need to change types.
>>
>> I recovered the SPI_IOC_RD_LSB_FIRST ioctl.
>> Add new ioctl to fit u16 mode.
>> In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way.
>>
>> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
>> ---
>
>>  /* IOCTL commands */
>> @@ -54,6 +61,8 @@
>>   * @tx_buf: Holds pointer to userspace buffer with transmit data, or
>> null.
>>   *     If no data is provided, zeroes are shifted out.
>>   * @rx_buf: Holds pointer to userspace buffer for receive data, or
>> null.
>> + * @tx_nbits: number of bits used for writting.
>> + * @rx_nbits: number of bits used for reading.
>>   * @len: Length of tx and rx buffers, in bytes.
>>   * @speed_hz: Temporary override of the device's bitrate.
>>   * @bits_per_word: Temporary override of the device's wordsize.
>> @@ -85,6 +94,8 @@
>>  struct spi_ioc_transfer {
>>         __u64           tx_buf;
>>         __u64           rx_buf;
>> +       __u8            tx_nbits;
>> +       __u8            rx_nbits;
>>
>>         __u32           len;
>>         __u32           speed_hz;
> You still break old userspace ABI.
>
> You need to create a new structure, if you need to add fields.
>
I dont think adding the members will cause the program in userspace
any errors. User do not have to set these members.
Thanks

Hi, Pekon

2013/7/29 Gupta, Pekon <pekon@ti.com>:
> Hi Yuhang,
>
>> >>
>> >> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
>> >> index 004b10f..3f6dc93 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;
>> >>
>> >  [Pekon] I think its cleaner to use similar parameters for channel-width
>> >  info everywhere. Like you have added separate fields  'tx_nbits' and
>> >  'rx_nbits' in spi_transfer, Why not do similar here for 'spi_master' ?
>> > And also in 'spi_device'. This can be like below:
>> >
>> > - SPI_3WIRE : tx_nbit = 0, rx_nbit = 0 (single wire for MISO & MOSI)
>> > - SPI_4WIRE :  tx_nbit = 1, rx_nbit = 1 (separate wire for MISO & MOSI)
>> > - DUAL_SPI :   tx_nbit = 2, rx_nbit = 2
>> > - QUAD_SPI : tx_nbit = 2 or 4, rx_nbit = 4
>> > This way 'mode' field can be explicitly left for other purposes like CPHA,
>> CPOL.
>> > OR custom modes (like modified SPI mode in Freescale controllers).
>> >
>> > But if you are touching any existing configuration (like SPI_3WIRE as
>> > proposed  above), Then please send it as independent patch/proposal,
>> > which can be Acked by individual SPI driver maintainers.
>> >
>> Adding the similar members like 'tx_nbits' and 'rx_nbits' is my
>> initial thought. But in different struct, that members meaning may be
>> a little different.
>> In spi_master, the member is what transfer mode master can support.
>> In spi_device, the member is what mode the slave will use.
>> In spi_transfer, the member is the transfer wires needed.
>> 1: adding the similar members everywhere may looks a little redundant.
>> What is more, in spi_master and spi_device, it is more like a mode and
>> in spi_device it is a transfer function due to the mode.
>> example:
>> DUAL mode in spi_device, in spi_transfer tx_nbits or rx_nbits can be
>> set to 1 or 2.
>> 2: the member in spi_master has to describe all the transfer
>> mode(single/dual/quad), because the member means the mode that master
>> support, so the member has to be seperated into several bits to cover
>> all the single dual and quad and then check it to spi_device. This
>> also seems similar to what the existed spi_master->mode_bit and
>> spi_device->mode do.
>> so added in mode may looks better.
>>
> [Pekon]: Just review following scenarios ..
> _Case-1_: I have a controller which supports
> - Quad & Dual modes only for reads,
> - Only Single SPI for writes.
>  This means though bit (spi_master.mode & QUAD_MODE)
> is set, but still if MTD layer does a SPI WRITE transfer using tx_nbits=4,
> I should get error..
>
> _Case-2_: Some flash devices also have similar constrains.
> Example: check Spansion flash S25FL128S datasheet,
>  "section 10.1 command summary". This device supports
> - Single/Dual/Quad READS,
> - but only SINGLE & QUAD page-programs (WRITES).
>
> So, if you have only one field ('mode') for storing capability of device or
> master, you cannot resolve above scenarios. This is where I realized that
>  we need to have separate tx_nbits and rx_nbits in spi_device and
>  spi_master also. (This is contrary to my own earliest posts where I
> proposed reusing mode bits for keeping channel-width info).
>
>
>
To the above two scenarios, it is necessary to modify the slave driver(m25p80.c)
what about this:
1,set spi_master->mode | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD;
2,set spi_device->mode | SPI_TX_QUAD | SPI_RX_QUAD(if use 4x read and
4x write) through DT.
3,add your own config and code in slave driver, example m25p80 read and write.
  if (spi->mode | QUAD)
    cmd and addr: tx_nbits=1;
    data: tx_nbits = 4;
  because no standard for dual and quad to serial flash, so no general
code for all flash.
  I just think to provide a spi interface to let flash can set its own
transfer bits.
  But now we have to correct the slave driver based on ourselves.
So if you modify your slave driver correctly, case1 and 2 won't happen.
If you use tx_nbits or something similar in spi_master and spi_device,
But the procedure may be same to above.

Best regards.

>> > [Pekon]: I think it is correct to assume all these SPI modes as mutually
>> > exclusive from of_property() point of view?
>> > Instead of defining separate of_properties(), you can define a single
>> > of_property() , something like 'max-tx-nbits' and 'max-rx-nbits'
>> > which can represent various modes like (DUAL, QUAD, 3WIRE, 4WIRE).
>> >
>> Yes, seems better. I will correct it.
>>
>> >>               /* 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 & 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;
>> >> +     }
>> > [Pekon]: this check would be not be required if you have single
>> >  of_property() for all modes.
>> >
>> >
>> >>       bad_bits = spi->mode & ~spi->master->mode_bits;
>> >>       if (bad_bits) {
>> >>               dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
>> >> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
>> >> index 2e0655d..0b776de 100644
>> >> --- a/drivers/spi/spidev.c
>> >> +++ b/drivers/spi/spidev.c
>> >> @@ -75,6 +75,9 @@ static DECLARE_BITMAP(minors, N_SPI_MINORS);
>> >>                               | SPI_LSB_FIRST | SPI_3WIRE | SPI_LOOP \
>> >>                               | SPI_NO_CS | SPI_READY)
>> >>
>> >> +#define SPI_EXTMODE_MASK     (SPI_MODE_MASK | SPI_TX_DUAL \
>> >> +                             | SPI_TX_QUAD | SPI_RX_DUAL |
>> >> SPI_RX_QUAD)
>> >> +
>> >>  struct spidev_data {
>> >>       dev_t                   devt;
>> >>       spinlock_t              spi_lock;
>> >> @@ -268,6 +271,8 @@ static int spidev_message(struct spidev_data
>> *spidev,
>> >>               k_tmp->bits_per_word = u_tmp->bits_per_word;
>> >>               k_tmp->delay_usecs = u_tmp->delay_usecs;
>> >>               k_tmp->speed_hz = u_tmp->speed_hz;
>> >> +             k_tmp->tx_nbits = u_tmp->tx_nbits;
>> >> +             k_tmp->rx_nbits = u_tmp->rx_nbits;
>> >>  #ifdef VERBOSE
>> >>               dev_dbg(&spidev->spi->dev,
>> >>                       "  xfer len %zd %s%s%s%dbits %u usec %uHz\n",
>> >> @@ -369,7 +374,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd,
>> >> unsigned long arg)
>> >>       case SPI_IOC_RD_MAX_SPEED_HZ:
>> >>               retval = __put_user(spi->max_speed_hz, (__u32 __user
>> >> *)arg);
>> >>               break;
>> >> -
>> >> +     case SPI_IOC_EXTRD_MODE:
>> >> +             retval = __put_user(spi->mode & SPI_EXTMODE_MASK,
>> >> +                                     (__u16 __user *)arg);
>> >> +             break;
>> >>       /* write requests */
>> >>       case SPI_IOC_WR_MODE:
>> >>               retval = __get_user(tmp, (u8 __user *)arg);
>> >> @@ -433,6 +441,25 @@ spidev_ioctl(struct file *filp, unsigned int cmd,
>> >> unsigned long arg)
>> >>                               dev_dbg(&spi->dev, "%d Hz (max)\n", tmp);
>> >>               }
>> >>               break;
>> >> +     case SPI_IOC_EXTWR_MODE:
>> >> +             retval = __get_user(tmp, (u16 __user *)arg);
>> >> +             if (retval == 0) {
>> >> +                     u16     save = spi->mode;
>> >> +
>> >> +                     if (tmp & ~SPI_EXTMODE_MASK) {
>> >> +                             retval = -EINVAL;
>> >> +                             break;
>> >> +                     }
>> >> +
>> >> +                     tmp |= spi->mode & ~SPI_EXTMODE_MASK;
>> >> +                     spi->mode = (u16)tmp;
>> >> +                     retval = spi_setup(spi);
>> >> +                     if (retval < 0)
>> >> +                             spi->mode = save;
>> >> +                     else
>> >> +                             dev_dbg(&spi->dev, "spi mode %02x\n",
>> >> tmp);
>> >> +             }
>> >> +             break;
>> >>
>> >>       default:
>> >>               /* segmented and/or full-duplex I/O request */
>> >> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
>> >> index 38c2b92..222e49e 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,8 @@ 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
>> >> + * @rx_nbits: number of bits used for reading
>> >>   * @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 +497,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 +522,11 @@ struct spi_transfer {
>> >>       dma_addr_t      rx_dma;
>> >>
>> >>       unsigned        cs_change:1;
>> >> +     u8              tx_nbits;
>> >> +     u8              rx_nbits;
>> >> +#define      SPI_NBITS_SINGLE        0x0; /* 1bit transfer */
>> >> +#define      SPI_NBITS_DUAL          0x01; /* 2bits transfer */
>> >> +#define      SPI_NBITS_QUAD          0x02; /* 4bits transfer */
>> >
>> > [Pekon]: I don't think it's a good place for #defines.  Plz keep it out of struct
>> body.
>> >
>> >>       u8              bits_per_word;
>> >>       u16             delay_usecs;
>> >>       u32             speed_hz;
>> >> @@ -858,7 +874,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
>> >
>> >
> with regards, pekon

Le Mon, 29 Jul 2013 15:25:51 +0100,
yuhang wang <wangyuhang2014@gmail.com> a écrit :

> Hi,
> 
> 2013/7/29 Matthieu CASTET <matthieu.castet@parrot.com>:
> > Le Mon, 29 Jul 2013 11:53:14 +0100,
> > wangyuhang <wangyuhang2014@gmail.com> a écrit :
> >
> >> Hi,
> >>
> >> modify two things.
> >> 1:
> >> >> @@ -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;
> >> >> +       }
> >> >> +               return -EINVAL;
> >> >> +       }
> >>
> >> >This code won't work if the constants you added for mode flags
> >> >change value.  More importantly, anyone searching the code for
> >> >SPI_TX_DUAL, etc. won't find this code.
> >>
> >> so use the certain macro to replace.
> >>
> >> 2:
> >> >You need to make sure that existing userspace binaries can run
> >> >unmodified, changing the types will change the layout of the ioctl
> >> >arguments.  This may mean that you need to add new ioctls if you
> >> >need to change types.
> >>
> >> I recovered the SPI_IOC_RD_LSB_FIRST ioctl.
> >> Add new ioctl to fit u16 mode.
> >> In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way.
> >>
> >> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
> >> ---
> >
> >>  /* IOCTL commands */
> >> @@ -54,6 +61,8 @@
> >>   * @tx_buf: Holds pointer to userspace buffer with transmit data,
> >> or null.
> >>   *     If no data is provided, zeroes are shifted out.
> >>   * @rx_buf: Holds pointer to userspace buffer for receive data, or
> >> null.
> >> + * @tx_nbits: number of bits used for writting.
> >> + * @rx_nbits: number of bits used for reading.
> >>   * @len: Length of tx and rx buffers, in bytes.
> >>   * @speed_hz: Temporary override of the device's bitrate.
> >>   * @bits_per_word: Temporary override of the device's wordsize.
> >> @@ -85,6 +94,8 @@
> >>  struct spi_ioc_transfer {
> >>         __u64           tx_buf;
> >>         __u64           rx_buf;
> >> +       __u8            tx_nbits;
> >> +       __u8            rx_nbits;
> >>
> >>         __u32           len;
> >>         __u32           speed_hz;
> > You still break old userspace ABI.
> >
> > You need to create a new structure, if you need to add fields.
> >
> I dont think adding the members will cause the program in userspace
> any errors. User do not have to set these members.
Did you try to run on your patch kernel a program build without the
patch ?


if I build my program with :

> >>  struct spi_ioc_transfer {
> >>         __u64           tx_buf;
> >>         __u64           rx_buf;
> >>
> >>         __u32           len;
> >>         __u32           speed_hz;

Now I run it on a new kernel.
The kernel won't parse correctly my structure : len will go in
tx_nbits, rx_nbits and len.

Keep in mind that it is binary interface, not source interface. You
should not have to rebuild your binary.

Matthieu

On Mon, Jul 29, 2013 at 7:21 AM, Gupta, Pekon <pekon@ti.com> wrote:
>> >>                       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;
>> >>
>> >  [Pekon] I think its cleaner to use similar parameters for channel-width
>> >  info everywhere. Like you have added separate fields  'tx_nbits' and
>> >  'rx_nbits' in spi_transfer, Why not do similar here for 'spi_master' ?

There are I think a total of five places that channel width can be
used in the Linux SPI stack.

a) In the master driver (spi_master)
b) In a slave driver (spi_device)
c) In a spi_message
d) In a spi_transfer
e) In the OF device tree, for DT nodes of a and/or b, as they appear in the dt.

For a, all the widths supported would be specified.  In b, c, and d,
only the width to be used would be specified.  So, there is some
difference here, as we need a method that allows multiple modes to be
specified for a but not for the others.

Since some slaves will want to send a single spi message with multiple
transfers of different widths (e.g., single bit read command followed
by quad bit data xfer) it's necessary to specify the width in d.  With
the widths specified at the lowest level, it is not actually necessary
to supply the width for C or B.

While it's not necessary to specify the width for B, doing so would be
consistent with other fields that can be specified in the spi_transfer
and in the spi_device, like bits_per_word and speed_hz.  It's worth
pointing out that none of the mode bits from the spi_device can be
specified in in a spi_transfer.  Currently, if you want to change
anything in mode you must call spi_setup() on the device.  Since width
can be changed on a transfer by transfer basis, maybe that is a good
reason to say it doesn't belong in mode.

If the default width is in the spi_device, then a spi_transfer needs a
width state to mean "unspecified".  I.e., the read width can be one,
two, four, (eight), or unspecified bits.  Unspecified gets changed to
the default from the spi_device, whereas one bit wide means one bit
wide.

Allowing the spi_device to have a width means the width can be checked
against the master's supported widths at spi_setup() time, like the
other mode bits.  But, since width can be set on a transfer by
transfer basis and each transfer is not checked in the spi core, this
doesn't really do the master driver much good.  The master driver
still needs to check each transfer to see if the width is allowed.

I don't see the point of putting the width in C.  Current a
spi_message can't change any of the settings from the spi_device or
supply a default for the the settings in a spi_transfer, and I don't
see why width is any different here.

For the OF device tree, I don't think the master's node should have
anything about the widths supported.  There is nothing about any of
the other spi master capabilities in the DT.  The master driver
supplies this information.

For the OF slave device node, it's not clear it needs to be there
either.  The current existing use cases are for memories that only use
dual and quad for certain parts of certain commands.  Setting the
width for the slave overall doesn't make sense.

But, the memory might have four data lines connected on the board or
it might just have one.  That does seem like information about board
layout that belongs in the device tree.  So there does need to be done
"enable quad mode" property.  However, is this a SPI property or a
property of the memory device's driver?  Turning on quad mode support
isn't the same that as setting the clock polarity.  Clock polarity it
a SPI property supplied to spi_setup.  Quad mode means the driver
should use the quad read command and the appropriate spi_transfers for
using it.

Hi,

Yes, you are right. I will correct it.

Thanks

2013/7/29 Matthieu CASTET <matthieu.castet@parrot.com>:
> Le Mon, 29 Jul 2013 15:25:51 +0100,
> yuhang wang <wangyuhang2014@gmail.com> a écrit :
>
>> Hi,
>>
>> 2013/7/29 Matthieu CASTET <matthieu.castet@parrot.com>:
>> > Le Mon, 29 Jul 2013 11:53:14 +0100,
>> > wangyuhang <wangyuhang2014@gmail.com> a écrit :
>> >
>> >> Hi,
>> >>
>> >> modify two things.
>> >> 1:
>> >> >> @@ -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;
>> >> >> +       }
>> >> >> +               return -EINVAL;
>> >> >> +       }
>> >>
>> >> >This code won't work if the constants you added for mode flags
>> >> >change value.  More importantly, anyone searching the code for
>> >> >SPI_TX_DUAL, etc. won't find this code.
>> >>
>> >> so use the certain macro to replace.
>> >>
>> >> 2:
>> >> >You need to make sure that existing userspace binaries can run
>> >> >unmodified, changing the types will change the layout of the ioctl
>> >> >arguments.  This may mean that you need to add new ioctls if you
>> >> >need to change types.
>> >>
>> >> I recovered the SPI_IOC_RD_LSB_FIRST ioctl.
>> >> Add new ioctl to fit u16 mode.
>> >> In addition,may SPI_IOC_RD_LSB_FIRST should be del in a better way.
>> >>
>> >> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com>
>> >> ---
>> >
>> >>  /* IOCTL commands */
>> >> @@ -54,6 +61,8 @@
>> >>   * @tx_buf: Holds pointer to userspace buffer with transmit data,
>> >> or null.
>> >>   *     If no data is provided, zeroes are shifted out.
>> >>   * @rx_buf: Holds pointer to userspace buffer for receive data, or
>> >> null.
>> >> + * @tx_nbits: number of bits used for writting.
>> >> + * @rx_nbits: number of bits used for reading.
>> >>   * @len: Length of tx and rx buffers, in bytes.
>> >>   * @speed_hz: Temporary override of the device's bitrate.
>> >>   * @bits_per_word: Temporary override of the device's wordsize.
>> >> @@ -85,6 +94,8 @@
>> >>  struct spi_ioc_transfer {
>> >>         __u64           tx_buf;
>> >>         __u64           rx_buf;
>> >> +       __u8            tx_nbits;
>> >> +       __u8            rx_nbits;
>> >>
>> >>         __u32           len;
>> >>         __u32           speed_hz;
>> > You still break old userspace ABI.
>> >
>> > You need to create a new structure, if you need to add fields.
>> >
>> I dont think adding the members will cause the program in userspace
>> any errors. User do not have to set these members.
> Did you try to run on your patch kernel a program build without the
> patch ?
>
>
> if I build my program with :
>
>> >>  struct spi_ioc_transfer {
>> >>         __u64           tx_buf;
>> >>         __u64           rx_buf;
>> >>
>> >>         __u32           len;
>> >>         __u32           speed_hz;
>
> Now I run it on a new kernel.
> The kernel won't parse correctly my structure : len will go in
> tx_nbits, rx_nbits and len.
>
> Keep in mind that it is binary interface, not source interface. You
> should not have to rebuild your binary.
>
> Matthieu

Hi,Trent

Thanks for so many details.
2013/7/30 Trent Piepho <tpiepho@gmail.com>:
> On Mon, Jul 29, 2013 at 7:21 AM, Gupta, Pekon <pekon@ti.com> wrote:
>>> >>                       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;
>>> >>
>>> >  [Pekon] I think its cleaner to use similar parameters for channel-width
>>> >  info everywhere. Like you have added separate fields  'tx_nbits' and
>>> >  'rx_nbits' in spi_transfer, Why not do similar here for 'spi_master' ?
>
> There are I think a total of five places that channel width can be
> used in the Linux SPI stack.
>
> a) In the master driver (spi_master)
> b) In a slave driver (spi_device)
> c) In a spi_message
> d) In a spi_transfer
> e) In the OF device tree, for DT nodes of a and/or b, as they appear in the dt.
>
> For a, all the widths supported would be specified.  In b, c, and d,
> only the width to be used would be specified.  So, there is some
> difference here, as we need a method that allows multiple modes to be
> specified for a but not for the others.
>
> Since some slaves will want to send a single spi message with multiple
> transfers of different widths (e.g., single bit read command followed
> by quad bit data xfer) it's necessary to specify the width in d.  With
> the widths specified at the lowest level, it is not actually necessary
> to supply the width for C or B.
>
> While it's not necessary to specify the width for B, doing so would be
> consistent with other fields that can be specified in the spi_transfer
> and in the spi_device, like bits_per_word and speed_hz.  It's worth
> pointing out that none of the mode bits from the spi_device can be
> specified in in a spi_transfer.  Currently, if you want to change
> anything in mode you must call spi_setup() on the device.  Since width
> can be changed on a transfer by transfer basis, maybe that is a good
> reason to say it doesn't belong in mode.
>
what you said is ok. But b) and d) are not the same. d) will be set
based on b), but not equal to d)=b)(bits_per_word and speed_hz). So
don't you think that b) is more like a transfer mode and d) is the
detailed functions related to the mode. Also b) should be set in
spi_setup(), only d) width can be changed by transfer, but b) should
not be modifyed. But none of the mode bits from the spi_device can be
specified in in a spi_transfer, that is a point. Is that necessary to
add members like "transfer_mode" in spi_master and spi_device which do
the similar operation to "mode"?

> If the default width is in the spi_device, then a spi_transfer needs a
> width state to mean "unspecified".  I.e., the read width can be one,
> two, four, (eight), or unspecified bits.  Unspecified gets changed to
> the default from the spi_device, whereas one bit wide means one bit
> wide.
>
what do you mean by default, is that without init value? Do you want
to use bit by bit in spi_transfer?

> Allowing the spi_device to have a width means the width can be checked
> against the master's supported widths at spi_setup() time, like the
> other mode bits.  But, since width can be set on a transfer by
> transfer basis and each transfer is not checked in the spi core, this
> doesn't really do the master driver much good.  The master driver
> still needs to check each transfer to see if the width is allowed.
>
agree. so do it in __spi_async() and to make sure that spi_transfer <=
spi_device.

> I don't see the point of putting the width in C.  Current a
> spi_message can't change any of the settings from the spi_device or
> supply a default for the the settings in a spi_transfer, and I don't
> see why width is any different here.
>
> For the OF device tree, I don't think the master's node should have
> anything about the widths supported.  There is nothing about any of
> the other spi master capabilities in the DT.  The master driver
> supplies this information.
>
> For the OF slave device node, it's not clear it needs to be there
> either.  The current existing use cases are for memories that only use
> dual and quad for certain parts of certain commands.  Setting the
> width for the slave overall doesn't make sense.
>
> But, the memory might have four data lines connected on the board or
> it might just have one.  That does seem like information about board
> layout that belongs in the device tree.  So there does need to be done
> "enable quad mode" property.  However, is this a SPI property or a
> property of the memory device's driver?  Turning on quad mode support
> isn't the same that as setting the clock polarity.  Clock polarity it
> a SPI property supplied to spi_setup.  Quad mode means the driver
> should use the quad read command and the appropriate spi_transfers for
> using it.
How to set DT depend on whether to seperate the dual and quad from mode.

Best regards.

> 
> Hello,
> > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> > index 38c2b92..222e49e 100644
> > --- a/include/linux/spi/spi.h
> > +++ b/include/linux/spi/spi.h
> > @@ -437,6 +441,8 @@ 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
> > + * @rx_nbits: number of bits used for reading
> >   * @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 +497,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 +522,11 @@ struct spi_transfer {
> >  	dma_addr_t	rx_dma;
> >
> >  	unsigned	cs_change:1;
> > +	u8		tx_nbits;
> > +	u8		rx_nbits;
> > +#define	SPI_NBITS_SINGLE	0x0; /* 1bit transfer */
> > +#define	SPI_NBITS_DUAL		0x01; /* 2bits transfer */
> > +#define	SPI_NBITS_QUAD		0x02; /* 4bits transfer */
> 
> [Pekon]: I don't think it's a good place for #defines.  Plz keep it out of struct
> body.
One more feedback, Its better you define SPI_NBITS_xx matching
the actual number of wires used. This would leave places for some
micro-wire and 3-wire SPI, and would be more readable.
+#define	SPI_NBITS_SINGLE_HALF_DUPLEX	0x0; /* 1bit transfer  microwire*/
+#define	SPI_NBITS_SINGLE_FULL_DUPLEX	0x01; /* 1bit transfer  normal SPI*/
+#define	SPI_NBITS_DUAL	2; /* 2bits transfer */
+#define	SPI_NBITS_QUAD	4; /* 4bits transfer */

with regards, pekon

> 
> Hi,Trent
> 
> Thanks for so many details.
> 2013/7/30 Trent Piepho <tpiepho@gmail.com>:
> > On Mon, Jul 29, 2013 at 7:21 AM, Gupta, Pekon <pekon@ti.com> wrote:
> >>> >>                       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;
> >>> >>
> >>> >  [Pekon] I think its cleaner to use similar parameters for channel-width
> >>> >  info everywhere. Like you have added separate fields  'tx_nbits' and
> >>> >  'rx_nbits' in spi_transfer, Why not do similar here for 'spi_master' ?
> >
> > There are I think a total of five places that channel width can be
> > used in the Linux SPI stack.
> >
> > a) In the master driver (spi_master)
> > b) In a slave driver (spi_device)
> > c) In a spi_message
> > d) In a spi_transfer
> > e) In the OF device tree, for DT nodes of a and/or b, as they appear in the
> dt.
> >
[Pekon]: Just to be aligned here.. 
(a) master_driver (spi_master) == like spi/spi-omap2-mcspi.c .. correct ?
(b) slave_driver (spi_device) ==  like mtd/devices/m25p80.c .. correct ?

> > For a, all the widths supported would be specified.  In b, c, and d,
> > only the width to be used would be specified.  So, there is some
> > difference here, as we need a method that allows multiple modes to be
> > specified for a but not for the others.
> >
> > Since some slaves will want to send a single spi message with multiple
> > transfers of different widths (e.g., single bit read command followed
> > by quad bit data xfer) it's necessary to specify the width in d.  With
> > the widths specified at the lowest level, it is not actually necessary
> > to supply the width for C or B.
> >
[Pekon]: Agreed.. This proposal is better of having..
(b) spi_device->mode = SPI_RX_QUAD | SPI_TX_SINGLE;
(c) spi_transfer.tx_nbits = 4; spi_transfer.rx_nbits = 4;

> > While it's not necessary to specify the width for B, doing so would be
> > consistent with other fields that can be specified in the spi_transfer
> > and in the spi_device, like bits_per_word and speed_hz.  It's worth
> > pointing out that none of the mode bits from the spi_device can be
> > specified in in a spi_transfer.  Currently, if you want to change
> > anything in mode you must call spi_setup() on the device.  Since width
> > can be changed on a transfer by transfer basis, maybe that is a good
> > reason to say it doesn't belong in mode.
> >
> what you said is ok. But b) and d) are not the same. d) will be set
> based on b), but not equal to d)=b)(bits_per_word and speed_hz). So
> don't you think that b) is more like a transfer mode and d) is the
> detailed functions related to the mode. Also b) should be set in
> spi_setup(), only d) width can be changed by transfer, but b) should
> not be modifyed. But none of the mode bits from the spi_device can be
> specified in in a spi_transfer, that is a point. Is that necessary to
> add members like "transfer_mode" in spi_master and spi_device which do
> the similar operation to "mode"?
> 
[Pekon]: I did not fully understand your comments, But following
 is my understanding of DT parsing.
 There should two level of DT binding
(a) For SPI controller: 
- describes the capabilities of H/W engine on SoC
- goes into spi_master->mode
(b) For SPI devices:
- describes capabilities of each device on board (includes board
   wire connections. (single controller can be connected to many SPI devices)
- goes into spi_device->mode, by matching common compatibilities
 between spi_master and spi_device_n

This is important for where multiple SPI devices are connected to 
one controller which supports all modes (SINGLE, DUAL, QUAD) But;
-  /dev/spi0 can have all 4 data-outputs connected on board 
   so can support QUAD mode
- /dev/spi1 has only 2 data-outputs connected on board
   so can support only SINGLE | DUAL

So, you have defined DT bindings for (b) only.
You need to add DT binding for (a) also.

> > If the default width is in the spi_device, then a spi_transfer needs a
> > width state to mean "unspecified".  I.e., the read width can be one,
> > two, four, (eight), or unspecified bits.  Unspecified gets changed to
> > the default from the spi_device, whereas one bit wide means one bit
> > wide.
> >
> what do you mean by default, is that without init value? Do you want
> to use bit by bit in spi_transfer?
>
[Pekon]: So default here means common basic capability which all 
devices are expected to meet at-least.
And I think this is SPI_SINGLE_FULL_DUPLEX mode.
 
> > Allowing the spi_device to have a width means the width can be checked
> > against the master's supported widths at spi_setup() time, like the
> > other mode bits.  But, since width can be set on a transfer by
> > transfer basis and each transfer is not checked in the spi core, this
> > doesn't really do the master driver much good.  The master driver
> > still needs to check each transfer to see if the width is allowed.
> >
> agree. so do it in __spi_async() and to make sure that spi_transfer <=
> spi_device.
> 
> > I don't see the point of putting the width in C.  Current a
> > spi_message can't change any of the settings from the spi_device or
> > supply a default for the the settings in a spi_transfer, and I don't
> > see why width is any different here.
> >
[Pekon]: Agreed.. if you are already adding tx_nbits & rx_nbits in 
spi_transfer, then you don't need to add same in spi_message.
(at-least not for now).

> > For the OF device tree, I don't think the master's node should have
> > anything about the widths supported.  There is nothing about any of
> > the other spi master capabilities in the DT.  The master driver
> > supplies this information.
> >
[Pekon]: Nope not correct. See example above of having multiple
SPI devices connected to same controller.

> > For the OF slave device node, it's not clear it needs to be there
> > either.  The current existing use cases are for memories that only use
> > dual and quad for certain parts of certain commands.  Setting the
> > width for the slave overall doesn't make sense.
> >
> > But, the memory might have four data lines connected on the board or
> > it might just have one.  That does seem like information about board
> > layout that belongs in the device tree.  So there does need to be done
> > "enable quad mode" property.  However, is this a SPI property or a
> > property of the memory device's driver?  Turning on quad mode support
> > isn't the same that as setting the clock polarity.  Clock polarity it
> > a SPI property supplied to spi_setup.  Quad mode means the driver
> > should use the quad read command and the appropriate spi_transfers for
> > using it.
> How to set DT depend on whether to seperate the dual and quad from
> mode.
> 
[Pekon] you need separate DT binding for controller and child devices.
And then separate for Tx and Rx in each of them.
Also, I suggest you add devicetree-discuss maillist when you update the 
Device-tree documentation for same.

I havn't seen any traffic on next version of this patch?
 Did I miss, or you are working on it?


with regards, pekon

Hi, Pekon

2013/8/7 Gupta, Pekon <pekon@ti.com>:
>>
>> Hello,
>> > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
>> > index 38c2b92..222e49e 100644
>> > --- a/include/linux/spi/spi.h
>> > +++ b/include/linux/spi/spi.h
>> > @@ -437,6 +441,8 @@ 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
>> > + * @rx_nbits: number of bits used for reading
>> >   * @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 +497,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 +522,11 @@ struct spi_transfer {
>> >     dma_addr_t      rx_dma;
>> >
>> >     unsigned        cs_change:1;
>> > +   u8              tx_nbits;
>> > +   u8              rx_nbits;
>> > +#define    SPI_NBITS_SINGLE        0x0; /* 1bit transfer */
>> > +#define    SPI_NBITS_DUAL          0x01; /* 2bits transfer */
>> > +#define    SPI_NBITS_QUAD          0x02; /* 4bits transfer */
>>
>> [Pekon]: I don't think it's a good place for #defines.  Plz keep it out of struct
>> body.
> One more feedback, Its better you define SPI_NBITS_xx matching
> the actual number of wires used. This would leave places for some
> micro-wire and 3-wire SPI, and would be more readable.
> +#define        SPI_NBITS_SINGLE_HALF_DUPLEX    0x0; /* 1bit transfer  microwire*/
> +#define        SPI_NBITS_SINGLE_FULL_DUPLEX    0x01; /* 1bit transfer  normal SPI*/
> +#define        SPI_NBITS_DUAL  2; /* 2bits transfer */
> +#define        SPI_NBITS_QUAD  4; /* 4bits transfer */
>
Totally three points.
1. There are still other positions which have macro defined in struct
in spi.h. That means the macro is belonged to the member above. So to
make it consistent, define it in @spi_transfer seems OK.
2. I don't think that  SPI_NBITS_SINGLE_HALF_DUPLEX is necessary.
tx_nbits and rx_nbits in @spi_transfer should be passed to spi master
driver and the master driver will set the certain mode depend on the
info in @spi_transfer. So there is no need to let the master driver
know SPI_NBITS_SINGLE_HALF_DUPLEX, also master can get the 3-wire SPI
info from spi_device->mode.
3. I agree to match the actual number of wires used. To make it 0x01,
0x02 and 0x04. Thanks.

Best regards.

> 2. I don't think that  SPI_NBITS_SINGLE_HALF_DUPLEX is necessary.
> tx_nbits and rx_nbits in @spi_transfer should be passed to spi master
> driver and the master driver will set the certain mode depend on the
> info in @spi_transfer. So there is no need to let the master driver
> know SPI_NBITS_SINGLE_HALF_DUPLEX, also master can get the 3-wire SPI
> info from spi_device->mode.
I should have been more explicit here.. it would be good it you replace
'spi_device->mode & SPI_3_WIRE' with your above mode & tx_nbit, rx_nbits
so that the channel width information becomes consistent for all modes.
All mode bit flags in same octet look better.

with regards, pekon

Hi, Pekon

2013/8/8 Gupta, Pekon <pekon@ti.com>:
>>
>> Hi,Trent
>>
>> Thanks for so many details.
>> 2013/7/30 Trent Piepho <tpiepho@gmail.com>:
>> > On Mon, Jul 29, 2013 at 7:21 AM, Gupta, Pekon <pekon@ti.com> wrote:
>> >>> >>                       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;
>> >>> >>
>> >>> >  [Pekon] I think its cleaner to use similar parameters for channel-width
>> >>> >  info everywhere. Like you have added separate fields  'tx_nbits' and
>> >>> >  'rx_nbits' in spi_transfer, Why not do similar here for 'spi_master' ?
>> >
>> > There are I think a total of five places that channel width can be
>> > used in the Linux SPI stack.
>> >
>> > a) In the master driver (spi_master)
>> > b) In a slave driver (spi_device)
>> > c) In a spi_message
>> > d) In a spi_transfer
>> > e) In the OF device tree, for DT nodes of a and/or b, as they appear in the
>> dt.
>> >
> [Pekon]: Just to be aligned here..
> (a) master_driver (spi_master) == like spi/spi-omap2-mcspi.c .. correct ?
> (b) slave_driver (spi_device) ==  like mtd/devices/m25p80.c .. correct ?
>
Yes, that's it.

>> > For a, all the widths supported would be specified.  In b, c, and d,
>> > only the width to be used would be specified.  So, there is some
>> > difference here, as we need a method that allows multiple modes to be
>> > specified for a but not for the others.
>> >
>> > Since some slaves will want to send a single spi message with multiple
>> > transfers of different widths (e.g., single bit read command followed
>> > by quad bit data xfer) it's necessary to specify the width in d.  With
>> > the widths specified at the lowest level, it is not actually necessary
>> > to supply the width for C or B.
>> >
> [Pekon]: Agreed.. This proposal is better of having..
> (b) spi_device->mode = SPI_RX_QUAD | SPI_TX_SINGLE;
> (c) spi_transfer.tx_nbits = 4; spi_transfer.rx_nbits = 4;
>
>> > While it's not necessary to specify the width for B, doing so would be
>> > consistent with other fields that can be specified in the spi_transfer
>> > and in the spi_device, like bits_per_word and speed_hz.  It's worth
>> > pointing out that none of the mode bits from the spi_device can be
>> > specified in in a spi_transfer.  Currently, if you want to change
>> > anything in mode you must call spi_setup() on the device.  Since width
>> > can be changed on a transfer by transfer basis, maybe that is a good
>> > reason to say it doesn't belong in mode.
>> >
>> what you said is ok. But b) and d) are not the same. d) will be set
>> based on b), but not equal to d)=b)(bits_per_word and speed_hz). So
>> don't you think that b) is more like a transfer mode and d) is the
>> detailed functions related to the mode. Also b) should be set in
>> spi_setup(), only d) width can be changed by transfer, but b) should
>> not be modifyed. But none of the mode bits from the spi_device can be
>> specified in in a spi_transfer, that is a point. Is that necessary to
>> add members like "transfer_mode" in spi_master and spi_device which do
>> the similar operation to "mode"?
>>
> [Pekon]: I did not fully understand your comments, But following
>  is my understanding of DT parsing.
>  There should two level of DT binding
> (a) For SPI controller:
> - describes the capabilities of H/W engine on SoC
> - goes into spi_master->mode
> (b) For SPI devices:
> - describes capabilities of each device on board (includes board
>    wire connections. (single controller can be connected to many SPI devices)
> - goes into spi_device->mode, by matching common compatibilities
>  between spi_master and spi_device_n
>
> This is important for where multiple SPI devices are connected to
> one controller which supports all modes (SINGLE, DUAL, QUAD) But;
> -  /dev/spi0 can have all 4 data-outputs connected on board
>    so can support QUAD mode
> - /dev/spi1 has only 2 data-outputs connected on board
>    so can support only SINGLE | DUAL
>
> So, you have defined DT bindings for (b) only.
> You need to add DT binding for (a) also.
>
well, if the final scheme is to make dual and quad in mode, I agree
with your point.

>> > If the default width is in the spi_device, then a spi_transfer needs a
>> > width state to mean "unspecified".  I.e., the read width can be one,
>> > two, four, (eight), or unspecified bits.  Unspecified gets changed to
>> > the default from the spi_device, whereas one bit wide means one bit
>> > wide.
>> >
>> what do you mean by default, is that without init value? Do you want
>> to use bit by bit in spi_transfer?
>>
> [Pekon]: So default here means common basic capability which all
> devices are expected to meet at-least.
> And I think this is SPI_SINGLE_FULL_DUPLEX mode.
>
If so, I still can not figure out what Trent means. Is that if
spi_device select the default mode, then we need to give
spi_transfer(tx_nbits and rx_nbits) a init value to make it not
unspecified?

>> > Allowing the spi_device to have a width means the width can be checked
>> > against the master's supported widths at spi_setup() time, like the
>> > other mode bits.  But, since width can be set on a transfer by
>> > transfer basis and each transfer is not checked in the spi core, this
>> > doesn't really do the master driver much good.  The master driver
>> > still needs to check each transfer to see if the width is allowed.
>> >
>> agree. so do it in __spi_async() and to make sure that spi_transfer <=
>> spi_device.
>>
>> > I don't see the point of putting the width in C.  Current a
>> > spi_message can't change any of the settings from the spi_device or
>> > supply a default for the the settings in a spi_transfer, and I don't
>> > see why width is any different here.
>> >
> [Pekon]: Agreed.. if you are already adding tx_nbits & rx_nbits in
> spi_transfer, then you don't need to add same in spi_message.
> (at-least not for now).
>
>> > For the OF device tree, I don't think the master's node should have
>> > anything about the widths supported.  There is nothing about any of
>> > the other spi master capabilities in the DT.  The master driver
>> > supplies this information.
>> >
> [Pekon]: Nope not correct. See example above of having multiple
> SPI devices connected to same controller.
>
>> > For the OF slave device node, it's not clear it needs to be there
>> > either.  The current existing use cases are for memories that only use
>> > dual and quad for certain parts of certain commands.  Setting the
>> > width for the slave overall doesn't make sense.
>> >
>> > But, the memory might have four data lines connected on the board or
>> > it might just have one.  That does seem like information about board
>> > layout that belongs in the device tree.  So there does need to be done
>> > "enable quad mode" property.  However, is this a SPI property or a
>> > property of the memory device's driver?  Turning on quad mode support
>> > isn't the same that as setting the clock polarity.  Clock polarity it
>> > a SPI property supplied to spi_setup.  Quad mode means the driver
>> > should use the quad read command and the appropriate spi_transfers for
>> > using it.
>> How to set DT depend on whether to seperate the dual and quad from
>> mode.
>>
> [Pekon] you need separate DT binding for controller and child devices.
> And then separate for Tx and Rx in each of them.
> Also, I suggest you add devicetree-discuss maillist when you update the
> Device-tree documentation for same.
>
Just as you said above, master node describe the total data wires on
SOC, slave node describe the detailed capability selected, which is
logically right. But to master, the drivers have already supplied this
infomation in probe, so do we still need to add in device tree and
provide another method in master driver to get the info from device
tree?

> I havn't seen any traffic on next version of this patch?
>  Did I miss, or you are working on it?
>
No, because of other jobs so didn't fix the patch in time, sorry for
that. Even now I still in puzzled that whether to make quad in mode or
add a new member in all the related struct. My trend is the previous
scheme. However they both have strong and weak points.

If add in mode:
[strong points]:
The existed operation such as compare master mode and device mode, DT
interface, mode set in master driver and so on which will be a benifit
to make the quad code clear and smooth.
[weak points]
But to expand mode will make spidev not back compatible and the
modified spidev code to fix the compatible problems looks a little
wierd.
Also another point from Trent, to make it consistent between
spi_transfer and spi_device. just like:
spi_transfer->speed_hz depend on spi_device->speed_hz;
spi_transfer->bits_per_word depends on spi_device->bits_per_word;
So is that add the similar member as below looks better?
spi_transfer->tx_nbits depends on spi_device->tx_nbits not spi_device->mode.

if add new member:
The previous strong points and weak points will become exchanged.

I still hope some suggestions to make one scheme extremely powerful
and I will provide the patch this week.

Best regards.

> > [Pekon] you need separate DT binding for controller and child devices.
> > And then separate for Tx and Rx in each of them.
> > Also, I suggest you add devicetree-discuss maillist when you update the
> > Device-tree documentation for same.
> >
> Just as you said above, master node describe the total data wires on
> SOC, slave node describe the detailed capability selected, which is
> logically right. But to master, the drivers have already supplied this
> infomation in probe, so do we still need to add in device tree and
> provide another method in master driver to get the info from device
> tree?
> 
[Pekon]: Yes, for controller the controller_driver should parse or fill in
this info in the spi_master->mode.
But, still you need to add check while probing spi_device DT that
only common supported compatibilities get into spi_device.
Example: if spi_master->mode == SPI_TX_DUAL only
and of_spi_device() returns SPI_TX_QUAD, then you should return
back with error showing mis-match in controller and device capabilities.
So, spi_device capabilites should always be sub-set of spi_master capabilities.


> > I havn't seen any traffic on next version of this patch?
> >  Did I miss, or you are working on it?
> >
> No, because of other jobs so didn't fix the patch in time, sorry for
> that. Even now I still in puzzled that whether to make quad in mode or
> add a new member in all the related struct. My trend is the previous
> scheme. However they both have strong and weak points.

> If add in mode:
> [strong points]:
> The existed operation such as compare master mode and device mode, DT
> interface, mode set in master driver and so on which will be a benifit
> to make the quad code clear and smooth.
> [weak points]
> But to expand mode will make spidev not back compatible and the
> modified spidev code to fix the compatible problems looks a little
> wierd.
> Also another point from Trent, to make it consistent between
> spi_transfer and spi_device. just like:
> spi_transfer->speed_hz depend on spi_device->speed_hz;
> spi_transfer->bits_per_word depends on spi_device->bits_per_word;
> So is that add the similar member as below looks better?
> spi_transfer->tx_nbits depends on spi_device->tx_nbits not spi_device->mode.

> if add new member:
> The previous strong points and weak points will become exchanged.

> I still hope some suggestions to make one scheme extremely powerful
> and I will provide the patch this week.

[Pekon]: I'm also inclined to your suggestion of previous scheme,
but we don't break the binary compatibility with userspace applications.
(as Matt pointed out earlier). But I think its more of an upgrade|extension of
of IOCTL than re-defining it. And as SPI is evolving we need to make these
upgrades in our interface to adapt to newer SPI.
But its upto maintainers to accept that. So, I suggest you send the spidev
related patch separately, so that there could be more discussion on that.


with regards, pekon

Hi, Pekon

2013/8/9 Gupta, Pekon <pekon@ti.com>:
>> 2. I don't think that  SPI_NBITS_SINGLE_HALF_DUPLEX is necessary.
>> tx_nbits and rx_nbits in @spi_transfer should be passed to spi master
>> driver and the master driver will set the certain mode depend on the
>> info in @spi_transfer. So there is no need to let the master driver
>> know SPI_NBITS_SINGLE_HALF_DUPLEX, also master can get the 3-wire SPI
>> info from spi_device->mode.
> I should have been more explicit here.. it would be good it you replace
> 'spi_device->mode & SPI_3_WIRE' with your above mode & tx_nbit, rx_nbits
> so that the channel width information becomes consistent for all modes.
> All mode bit flags in same octet look better.
>
If still use mode in spi_master and spi_device, I don't think it is
possible to del SPI_3_WIRE. But we should make sure if SPI_3_WIRE is
selected by slave, then dual and quad should not be set again.

2013/8/9 Gupta, Pekon <pekon@ti.com>:
>> > [Pekon] you need separate DT binding for controller and child devices.
>> > And then separate for Tx and Rx in each of them.
>> > Also, I suggest you add devicetree-discuss maillist when you update the
>> > Device-tree documentation for same.
>> >
>> Just as you said above, master node describe the total data wires on
>> SOC, slave node describe the detailed capability selected, which is
>> logically right. But to master, the drivers have already supplied this
>> infomation in probe, so do we still need to add in device tree and
>> provide another method in master driver to get the info from device
>> tree?
>>
> [Pekon]: Yes, for controller the controller_driver should parse or fill in
> this info in the spi_master->mode.
> But, still you need to add check while probing spi_device DT that
> only common supported compatibilities get into spi_device.
> Example: if spi_master->mode == SPI_TX_DUAL only
> and of_spi_device() returns SPI_TX_QUAD, then you should return
> back with error showing mis-match in controller and device capabilities.
> So, spi_device capabilites should always be sub-set of spi_master capabilities.
>
Yes, it is necessary to check. But I don't think it is a good way to
fill in spi_master->mode from master node of DT. Master driver
developer should set dual or quad mode in spi_master->mode directly
just like SPI_CPHA and SPI_CPOL. To keep consistent, dual and quad
should also not appear in master node.

>> > I havn't seen any traffic on next version of this patch?
>> >  Did I miss, or you are working on it?
>> >
>> No, because of other jobs so didn't fix the patch in time, sorry for
>> that. Even now I still in puzzled that whether to make quad in mode or
>> add a new member in all the related struct. My trend is the previous
>> scheme. However they both have strong and weak points.
>
>> If add in mode:
>> [strong points]:
>> The existed operation such as compare master mode and device mode, DT
>> interface, mode set in master driver and so on which will be a benifit
>> to make the quad code clear and smooth.
>> [weak points]
>> But to expand mode will make spidev not back compatible and the
>> modified spidev code to fix the compatible problems looks a little
>> wierd.
>> Also another point from Trent, to make it consistent between
>> spi_transfer and spi_device. just like:
>> spi_transfer->speed_hz depend on spi_device->speed_hz;
>> spi_transfer->bits_per_word depends on spi_device->bits_per_word;
>> So is that add the similar member as below looks better?
>> spi_transfer->tx_nbits depends on spi_device->tx_nbits not spi_device->mode.
>
>> if add new member:
>> The previous strong points and weak points will become exchanged.
>
>> I still hope some suggestions to make one scheme extremely powerful
>> and I will provide the patch this week.
>
> [Pekon]: I'm also inclined to your suggestion of previous scheme,
> but we don't break the binary compatibility with userspace applications.
> (as Matt pointed out earlier). But I think its more of an upgrade|extension of
> of IOCTL than re-defining it. And as SPI is evolving we need to make these
> upgrades in our interface to adapt to newer SPI.
> But its upto maintainers to accept that. So, I suggest you send the spidev
> related patch separately, so that there could be more discussion on that.
>
>
OK, I will separate the patch. Thanks.

Best regards