On Wed, Sep 4, 2013 at 12:07 AM, yuhang wang <wangyuhang2...@gmail.com> wrote:
>>> dma_addr_t rx_dma;
>>>
>>> unsigned cs_change:1;
>>> + u8 tx_nbits;
>>> + u8 rx_nbits;
>>> +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */
>>> +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */
>>> +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */
>>
>> These fields increase the size of a spi_transfer by 4 bytes. If you
>> used bitfields instead it wouldn't increase the size at all since
>> there are still 7 bits left after cs_change.
> Yes, it will increase the size by 4 bytes, but using bitfields here
> will make it logically weird. Bitfields may mean that really have the
> restriction of number of bits. If just for saving memory, then what
> about the other members such as bits_per_world, delay_usecs.
Rather than just talk about it, I'll make patches to do it. Then it
will be clear.
On Wed, Sep 4, 2013 at 12:07 AM, yuhang wang <wangyuhang2...@gmail.com> wrote:
> 2013/9/4 Trent Piepho <tpie...@gmail.com>:
>> On Sun, Aug 11, 2013 at 3:15 AM, wangyuhang <wangyuhang2...@gmail.com> wrote:
>>> fix the previous patch some mistake below:
>>
>> This isn't a very good commit message. "the previous patch" will have
>> no meaning in the kernel git repo. The rest of the message only
>> describes changes since a previous version of the patch and not the
>> actual patch in full.
>>
>>>
>>> + /* Device DUAL/QUAD mode */
>>> + prop = of_get_property(nc, "spi-tx-nbits", &len);
>>
>> Why not use of_property_read_u32() here?
>>
>>> + if (!prop || len < sizeof(*prop)) {
>>> + dev_err(&master->dev, "%s has no 'spi-tx-nbits'
>>> property\n",
>>> + nc->full_name);
>>> + spi_dev_put(spi);
>>> + continue;
>>
>> So if no spi-tx-nbits property is supplied, the device is rejected and
>> the loop continues to the next device entry? This means ALL EXISTING
>> DEVICE TREES with SPI devices will be rejected, since none of them
>> have this new property! Was this patch tested at all with any system
>> before being accepted?
>>
> This part has been fixed in patch[commit
> id:a822e99c70f448c4068ea85bb195dac0b2eb3afe]
>
>>> + /* check mode to prevent that DUAL and QUAD set at the same time
>>> + */
>>> + if (((spi->mode & SPI_TX_DUAL) && (spi->mode & SPI_TX_QUAD)) ||
>>> + ((spi->mode & SPI_RX_DUAL) && (spi->mode & SPI_RX_QUAD))) {
>>> + dev_err(&spi->dev,
>>> + "setup: can not select dual and quad at the same time\n");
>>> + return -EINVAL;
>>
>> This test can be done with fewer operations as:
>> if ((spi->mode & (SPI_TX_DUAL|SPI_TX_QUAD)) == (SPI_TX_DUAL|SPI_TX_QUAD) ||
>> (spi->mode & (SPI_RX_DUAL|SPI_RX_QUAD)) == (SPI_RX_DUAL|SPI_RX_QUAD)) {
>>
>>
>>> + }
>>> + /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden
>>> + */
>>> + if ((spi->mode & SPI_3WIRE) && (spi->mode &
>>> + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL | SPI_RX_QUAD)))
>>> + return -EINVAL;
>>
>> No dev_err message for this possibility? What's different than the
>> previous check that does produce a message?
>>
> OK, should be added. Thanks.
>
>>> /* help drivers fail *cleanly* when they need options
>>> * that aren't supported with their current master
>>> */
>> Following this comment is the code:
>> bad_bits = spi->mode & ~spi->master->mode_bits;
>> if (bad_bits) {
>> dev_err(&spi->dev, "setup: unsupported mode bits %x\n",
>> bad_bits);
>> return -EINVAL;
>> }
>>
>> Won't this always trigger for anything that sets one of the dual or quad
>> bits?
>>
> I can't get your idea clearly. Please provide more infomation.
>
>>> @@ -1370,12 +1428,50 @@ static int __spi_async(struct spi_device *spi,
>>> struct spi_message *message)
>>> /**
>>> * Set transfer bits_per_word and max speed as spi device default if
>>> * it is not set for this transfer.
>>> + * Set transfer tx_nbits and rx_nbits as single transfer default
>>> + * (SPI_NBITS_SINGLE) if it is not set for this transfer.
>>> */
>>> list_for_each_entry(xfer, &message->transfers, transfer_list) {
>>> if (!xfer->bits_per_word)
>>> xfer->bits_per_word = spi->bits_per_word;
>>> if (!xfer->speed_hz)
>>> xfer->speed_hz = spi->max_speed_hz;
>>> + if (xfer->tx_buf && !xfer->tx_nbits)
>>> + xfer->tx_nbits = SPI_NBITS_SINGLE;
>>> + if (xfer->rx_buf && !xfer->rx_nbits)
>>> + xfer->rx_nbits = SPI_NBITS_SINGLE;
>>> + /* check transfer tx/rx_nbits:
>>> + * 1. keep the value is not out of single, dual and quad
>>> + * 2. keep tx/rx_nbits is contained by mode in spi_device
>>> + * 3. if SPI_3WIRE, tx/rx_nbits should be in single
>>> + */
>>> + if (xfer->tx_nbits != SPI_NBITS_SINGLE &&
>>> + xfer->tx_nbits != SPI_NBITS_DUAL &&
>>> + xfer->tx_nbits != SPI_NBITS_QUAD)
>>> + return -EINVAL;
>>> + if ((xfer->tx_nbits == SPI_NBITS_DUAL) &&
>>> + !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD)))
>>> + return -EINVAL;
>>> + if ((xfer->tx_nbits == SPI_NBITS_QUAD) &&
>>> + !(spi->mode & SPI_TX_QUAD))
>>> + return -EINVAL;
>>> + if ((spi->mode & SPI_3WIRE) &&
>>> + (xfer->tx_nbits != SPI_NBITS_SINGLE))
>>> + return -EINVAL;
>>> + /* check transfer rx_nbits */
>>> + if (xfer->rx_nbits != SPI_NBITS_SINGLE &&
>>> + xfer->rx_nbits != SPI_NBITS_DUAL &&
>>> + xfer->rx_nbits != SPI_NBITS_QUAD)
>>> + return -EINVAL;
>>> + if ((xfer->rx_nbits == SPI_NBITS_DUAL) &&
>>> + !(spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD)))
>>> + return -EINVAL;
>>> + if ((xfer->rx_nbits == SPI_NBITS_QUAD) &&
>>> + !(spi->mode & SPI_RX_QUAD))
>>> + return -EINVAL;
>>> + if ((spi->mode & SPI_3WIRE) &&
>>> + (xfer->rx_nbits != SPI_NBITS_SINGLE))
>>> + return -EINVAL;
>>> }
>>
>> What a lot of code to check the transfer bits. It really needs this much?
>>
> This part is also corrected in patch [commit
> id:db90a44177ac39fc22b2da5235b231fccdd4c673].
>
>>> @@ -511,6 +524,11 @@ struct spi_transfer {
>>> dma_addr_t rx_dma;
>>>
>>> unsigned cs_change:1;
>>> + u8 tx_nbits;
>>> + u8 rx_nbits;
>>> +#define SPI_NBITS_SINGLE 0x01 /* 1bit transfer */
>>> +#define SPI_NBITS_DUAL 0x02 /* 2bits transfer */
>>> +#define SPI_NBITS_QUAD 0x04 /* 4bits transfer */
>>
>> These fields increase the size of a spi_transfer by 4 bytes. If you
>> used bitfields instead it wouldn't increase the size at all since
>> there are still 7 bits left after cs_change.
> Yes, it will increase the size by 4 bytes, but using bitfields here
> will make it logically weird. Bitfields may mean that really have the
> restriction of number of bits. If just for saving memory, then what
> about the other members such as bits_per_world, delay_usecs.
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance.
Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from
the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
