| Message ID | 20201127130834.136348-2-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/3] spi: uapi: unify SPI modes into a single spi.h header | expand |
On Fri, Nov 27, 2020 at 3:08 PM Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > Transmit/receive only is a valid SPI mode. For example, the MOSI/TX line > might be missing from an ADC while for a DAC the MISO/RX line may be > optional. This patch adds these two new modes: SPI_NO_TX and > SPI_NO_RX. This way, the drivers will be able to identify if any of > these two lines is missing and to adjust the transfers accordingly. ... > + /* > + * check mode to prevent that any two of DUAL, QUAD and NO_MOSI/MISO > + * are set at the same time > */ Since you are here, check -> Check time -> time. ... > + if ((hweight_long(spi->mode & > + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_NO_TX)) > 1) || > + (hweight_long(spi->mode & > + (SPI_RX_DUAL | SPI_RX_QUAD | SPI_NO_RX)) > 1)) { > dev_err(&spi->dev, > - "setup: can not select dual and quad at the same time\n"); > + "setup: can not select any two of dual, quad and no-rx/tx " > + "at the same time\n"); Don't split literals, and probably rephrase (If I can't set 2, can I set 3?) "setup: can't select more than one out of dual, quad, and no-Rx / no-Tx at the same time\n"); > --- a/include/uapi/linux/spi/spi.h > +++ b/include/uapi/linux/spi/spi.h > @@ -43,5 +43,7 @@ > #define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */ > #define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */ > #define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround */ > +#define SPI_NO_TX 0x10000 /* no transmit wire */ > +#define SPI_NO_RX 0x20000 /* no receive wire */ Is it really material for uAPI? Perhaps we may have something like SPI_MODE_USER_MASK in uAPI and in internal headers SPI_MODE_KERNEL_MASK with static_assert(_USER_MASK & _KERNEL_MASK); // check conditional ?
On Fri, Nov 27, 2020 at 4:22 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Nov 27, 2020 at 3:08 PM Alexandru Ardelean > <alexandru.ardelean@analog.com> wrote: ... > > --- a/include/uapi/linux/spi/spi.h > > +++ b/include/uapi/linux/spi/spi.h > > @@ -43,5 +43,7 @@ > > #define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */ > > #define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */ > > #define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround */ > > +#define SPI_NO_TX 0x10000 /* no transmit wire */ > > +#define SPI_NO_RX 0x20000 /* no receive wire */ > > Is it really material for uAPI? > Perhaps we may have something like > SPI_MODE_USER_MASK in uAPI and > in internal headers > > SPI_MODE_KERNEL_MASK with > static_assert(_USER_MASK & _KERNEL_MASK); // check conditional > > ? And logically start bits for the kernel from the end (31, 30, ...).
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Friday, November 27, 2020 4:24 PM > To: Ardelean, Alexandru <alexandru.Ardelean@analog.com> > Cc: linux-spi <linux-spi@vger.kernel.org>; devicetree > <devicetree@vger.kernel.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org>; Rob Herring <robh+dt@kernel.org>; Mark Brown > <broonie@kernel.org>; Bogdan, Dragos <Dragos.Bogdan@analog.com> > Subject: Re: [PATCH v3 2/3] spi: Add SPI_NO_TX/RX support > > On Fri, Nov 27, 2020 at 4:22 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Nov 27, 2020 at 3:08 PM Alexandru Ardelean > > <alexandru.ardelean@analog.com> wrote: > > ... > > > > --- a/include/uapi/linux/spi/spi.h > > > +++ b/include/uapi/linux/spi/spi.h > > > @@ -43,5 +43,7 @@ > > > #define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */ > > > #define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */ > > > #define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround > */ > > > +#define SPI_NO_TX 0x10000 /* no transmit wire */ > > > +#define SPI_NO_RX 0x20000 /* no receive wire */ > > > > Is it really material for uAPI? > > Perhaps we may have something like > > SPI_MODE_USER_MASK in uAPI and > > in internal headers Hmm, in a way this could make sense for some SPIDEVs as well, to set these flags and get an error if they try to TX with the NO_TX flag set. Not really sure about this. Initially I mechanically added these here as an inertia to the previous patch which is just unifying the headers. Any other opinions? Thoughts? Mark? > > > > SPI_MODE_KERNEL_MASK with > > static_assert(_USER_MASK & _KERNEL_MASK); // check conditional > > > > ? > > And logically start bits for the kernel from the end (31, 30, ...). > > -- > With Best Regards, > Andy Shevchenko
On Thu, Dec 03, 2020 at 08:20:57AM +0000, Ardelean, Alexandru wrote: > > > > @@ -43,5 +43,7 @@ > > > > #define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */ > > > > #define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */ > > > > #define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround > > */ > > > > +#define SPI_NO_TX 0x10000 /* no transmit wire */ > > > > +#define SPI_NO_RX 0x20000 /* no receive wire */ > > > Is it really material for uAPI? > > > Perhaps we may have something like > > > SPI_MODE_USER_MASK in uAPI and > > > in internal headers > Hmm, in a way this could make sense for some SPIDEVs as well, to set these flags and get an error if they try to TX with the NO_TX flag set. > Not really sure about this. > Initially I mechanically added these here as an inertia to the previous patch which is just unifying the headers. > Any other opinions? Thoughts? > Mark? spidev is hacky at the best of times... It *is* probably better to only have the usefully mainpulable modes exposed in uapi and then define the rest internally though.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index cd3c395b4e90..17d4004bd2c6 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1941,6 +1941,9 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, /* Device DUAL/QUAD mode */ if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) { switch (value) { + case 0: + spi->mode |= SPI_NO_TX; + break; case 1: break; case 2: @@ -1962,6 +1965,9 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi, if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) { switch (value) { + case 0: + spi->mode |= SPI_NO_RX; + break; case 1: break; case 2: @@ -3329,12 +3335,17 @@ int spi_setup(struct spi_device *spi) unsigned bad_bits, ugly_bits; int status; - /* check mode to prevent that DUAL and QUAD set at the same time + /* + * check mode to prevent that any two of DUAL, QUAD and NO_MOSI/MISO + * are 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))) { + if ((hweight_long(spi->mode & + (SPI_TX_DUAL | SPI_TX_QUAD | SPI_NO_TX)) > 1) || + (hweight_long(spi->mode & + (SPI_RX_DUAL | SPI_RX_QUAD | SPI_NO_RX)) > 1)) { dev_err(&spi->dev, - "setup: can not select dual and quad at the same time\n"); + "setup: can not select any two of dual, quad and no-rx/tx " + "at the same time\n"); return -EINVAL; } /* if it is SPI_3WIRE mode, DUAL and QUAD should be forbidden @@ -3348,7 +3359,8 @@ int spi_setup(struct spi_device *spi) * SPI_CS_WORD has a fallback software implementation, * so it is ignored here. */ - bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD); + bad_bits = spi->mode & ~(spi->controller->mode_bits | SPI_CS_WORD | + SPI_NO_TX | SPI_NO_RX); /* nothing prevents from working with active-high CS in case if it * is driven by GPIO. */ @@ -3609,6 +3621,8 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message) * 2. check tx/rx_nbits match the mode in spi_device */ if (xfer->tx_buf) { + if (spi->mode & SPI_NO_TX) + return -EINVAL; if (xfer->tx_nbits != SPI_NBITS_SINGLE && xfer->tx_nbits != SPI_NBITS_DUAL && xfer->tx_nbits != SPI_NBITS_QUAD) @@ -3622,6 +3636,8 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message) } /* check transfer rx_nbits */ if (xfer->rx_buf) { + if (spi->mode & SPI_NO_RX) + return -EINVAL; if (xfer->rx_nbits != SPI_NBITS_SINGLE && xfer->rx_nbits != SPI_NBITS_DUAL && xfer->rx_nbits != SPI_NBITS_QUAD) diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h index ae028d64c779..b504e46a6f18 100644 --- a/include/uapi/linux/spi/spi.h +++ b/include/uapi/linux/spi/spi.h @@ -43,5 +43,7 @@ #define SPI_TX_OCTAL 0x2000 /* transmit with 8 wires */ #define SPI_RX_OCTAL 0x4000 /* receive with 8 wires */ #define SPI_3WIRE_HIZ 0x8000 /* high impedance turnaround */ +#define SPI_NO_TX 0x10000 /* no transmit wire */ +#define SPI_NO_RX 0x20000 /* no receive wire */ #endif /* _UAPI_SPI_H */