| Message ID | 20180717032052.12273-3-david@lechnology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 16, 2018 at 10:20:50PM -0500, David Lechner wrote: > This adds a new SPI mode flag, SPI_CS_WORD, that is used to indicate > that a SPI device requires the chip select to be toggled after each > word that is transferred. This feels like it should have a soft implementation if it is going to be truly usable, the vast majority of SPI controllers don't do this and I can only think of a few that have the hardware feature. I'd also expect to see some validation added to the core spi_setup() since at present a client driver could set the mode option but then have it ignored by the controller which would presumably break things, we currently only have checks for specific modes and nothing that'd catch an unknown flag like this. Ideally we'd also have some ability to use this as an optimization where possible with longer sequences (I can see a regmap cache sync being able to take advantage of this for example) but that might be more trouble than it's worth.
On 7/18/18 10:04 AM, Mark Brown wrote: > On Mon, Jul 16, 2018 at 10:20:50PM -0500, David Lechner wrote: >> This adds a new SPI mode flag, SPI_CS_WORD, that is used to indicate >> that a SPI device requires the chip select to be toggled after each >> word that is transferred. > > This feels like it should have a soft implementation if it is going to > be truly usable, the vast majority of SPI controllers don't do this and This occurred to me as well. Another possibility, though, would be to leave it up to the client device drivers to support both cases, e.g.: if (master has SPI_CS_WORD support) setup message as single transfer else setup message as multiple one-word transfers This seems like that would be more efficient than having a generic implementation for masters that says: if (master does not have SPI_CS_WORD support) allocate enough transfers for each word of each each transfer of the message allocate and setup a new message for these transfers loop through the original transfers of the original message and copy them to the new transfers send the new message free allocated message and transfers > I can only think of a few that have the hardware feature. I'd also > expect to see some validation added to the core spi_setup() since at > present a client driver could set the mode option but then have it > ignored by the controller which would presumably break things, we > currently only have checks for specific modes and nothing that'd catch > an unknown flag like this. There is already a generic mode flags check in spi_setup() that will catch this and return an error if the device has the SPI_CS_WORD flag set and the controller does not. (I know this works because I ran into it during development.) > > Ideally we'd also have some ability to use this as an optimization where > possible with longer sequences (I can see a regmap cache sync being able > to take advantage of this for example) but that might be more trouble > than it's worth. > -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 18, 2018 at 11:47:30AM -0500, David Lechner wrote: > On 7/18/18 10:04 AM, Mark Brown wrote: > > This feels like it should have a soft implementation if it is going to > > be truly usable, the vast majority of SPI controllers don't do this and > This occurred to me as well. Another possibility, though, would be to leave > it up to the client device drivers to support both cases, e.g.: > if (master has SPI_CS_WORD support) > setup message as single transfer > else > setup message as multiple one-word transfers > This seems like that would be more efficient than having a generic > implementation for masters that says: That then requires every single user to open code this which immediately suggests that there should be a helper which is going to look a lot like any generic implementation. > if (master does not have SPI_CS_WORD support) > allocate enough transfers for each word of each > each transfer of the message > allocate and setup a new message for these transfers > loop through the original transfers of the original > message and copy them to the new transfers > send the new message > free allocated message and transfers I'd imagine that the much bigger problem is that you end up with enormous numbers of operations for any non-trivial transfers which is going to happen anyway. It's really only the copying bit that's at all an overhead here. > > I can only think of a few that have the hardware feature. I'd also > > expect to see some validation added to the core spi_setup() since at > > present a client driver could set the mode option but then have it > > ignored by the controller which would presumably break things, we > > currently only have checks for specific modes and nothing that'd catch > > an unknown flag like this. > There is already a generic mode flags check in spi_setup() that will catch > this and return an error if the device has the SPI_CS_WORD flag set and the > controller does not. (I know this works because I ran into it during > development.) Ah, good - I'd forgotten it was there and didn't spot it when I went to check.
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index a64235e05321..7cc1466111f5 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -163,6 +163,7 @@ struct spi_device { #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 */ +#define SPI_CS_WORD 0x1000 /* toggle cs after each word */ int irq; void *controller_state; void *controller_data; @@ -177,7 +178,6 @@ struct spi_device { * the controller talks to each chip, like: * - memory packing (12 bit samples into low bits, others zeroed) * - priority - * - drop chipselect after each word * - chipselect delays * - ... */
This adds a new SPI mode flag, SPI_CS_WORD, that is used to indicate that a SPI device requires the chip select to be toggled after each word that is transferred. Signed-off-by: David Lechner <david@lechnology.com> --- include/linux/spi/spi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)