| Message ID | 1392444566-23605-1-git-send-email-iivanov@mm-sol.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Mark Brown |
| Headers | show |
On Sat, Feb 15, 2014 at 08:09 +0200, Ivan T. Ivanov wrote: > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > SPI transfer lenght should be a power-of-two multiple > of eight bits. Please re-check for "lenght" typos in subjects and commit logs (code comments appear to be correct already). The commit message still confuses me. You don't check that the length of an SPI transfer has a power-of-two length in bytes. Instead what the check implements is IIUC - pad "odd" bits-per-word settings before the check to full 1/2/4 byte length specs (this is the power-of-two thing) - the total length of the SPI transfer cannot be empty (which I'd consider an optimization, not a violation, and may need a separate discussion) - the total length of the SPI transfer must be such that each "word" must be provided within a full 1/2/4 byte entity, with padding bits if the bits-per-word is "odd" Is this a misunderstanding on my side? A terminology thing? To me, the "SPI transfer" is the total payload and may have any arbitrary length. What you check for is a constraint on the transfer's length derived from or based on the "word length" ('word' in SPI context). So the code may be appropriate, yet the description may need an update, to not have the next person ask the same questions again. > @@ -1668,6 +1669,22 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message) > return -EINVAL; > } > > + /* > + * SPI transfer length should be multiple of SPI word size > + * where SPI word size should be power-of-two multiple > + */ > + if (xfer->bits_per_word <= 8) > + w_size = 1; > + else if (xfer->bits_per_word <= 16) > + w_size = 2; > + else > + w_size = 4; > + > + n_words = xfer->len / w_size; > + /* No partial transfers accepted */ > + if (!n_words || xfer->len % xfer->bits_per_word) > + return -EINVAL; > + > if (xfer->speed_hz && master->min_speed_hz && > xfer->speed_hz < master->min_speed_hz) > return -EINVAL; [ just left the code here for comparison with the above description ] virtually yours Gerhard Sittig
Hi, On Sun, 2014-02-16 at 15:25 +0100, Gerhard Sittig wrote: > On Sat, Feb 15, 2014 at 08:09 +0200, Ivan T. Ivanov wrote: > > > > From: "Ivan T. Ivanov" <iivanov@mm-sol.com> > > > > SPI transfer lenght should be a power-of-two multiple > > of eight bits. > > Please re-check for "lenght" typos in subjects and commit logs > (code comments appear to be correct already). Thanks, will correct them. > > > The commit message still confuses me. You don't check that the > length of an SPI transfer has a power-of-two length in bytes. > > Instead what the check implements is IIUC > - pad "odd" bits-per-word settings before the check to full 1/2/4 > byte length specs (this is the power-of-two thing) > - the total length of the SPI transfer cannot be empty (which I'd > consider an optimization, not a violation, and may need a > separate discussion) I am not sure that I understand this one. I will say "the total length of the SPI transfer _should not_ be empty". There is no check for this currently. > - the total length of the SPI transfer must be such that each > "word" must be provided within a full 1/2/4 byte entity, with > padding bits if the bits-per-word is "odd" > > Is this a misunderstanding on my side? A terminology thing? To > me, the "SPI transfer" is the total payload and may have any > arbitrary length. What you check for is a constraint on the > transfer's length derived from or based on the "word length" > ('word' in SPI context). > > So the code may be appropriate, yet the description may need an > update, to not have the next person ask the same questions again. > I just forgot to update commit message. Will try to be more careful next time. Regards, Ivan > > virtually yours > Gerhard Sittig -- 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
Hi, Please ignore this patch. On Sat, 2014-02-15 at 08:09 +0200, Ivan T. Ivanov wrote: > > + /* > + * SPI transfer length should be multiple of SPI word size > + * where SPI word size should be power-of-two multiple > + */ > + if (xfer->bits_per_word <= 8) > + w_size = 1; > + else if (xfer->bits_per_word <= 16) > + w_size = 2; > + else > + w_size = 4; > + > + n_words = xfer->len / w_size; > + /* No partial transfers accepted */ > + if (!n_words || xfer->len % xfer->bits_per_word) This should be if (!n_words || xfer->len % w_size) Regards, Ivan -- 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 Sun, Feb 16, 2014 at 03:25:12PM +0100, Gerhard Sittig wrote: > On Sat, Feb 15, 2014 at 08:09 +0200, Ivan T. Ivanov wrote: > - the total length of the SPI transfer cannot be empty (which I'd > consider an optimization, not a violation, and may need a > separate discussion) We probably want to allow that for people doing fun stuff with cs_change though I'm not convinced anything doing that is actually a good idea. > - the total length of the SPI transfer must be such that each > "word" must be provided within a full 1/2/4 byte entity, with > padding bits if the bits-per-word is "odd" > Is this a misunderstanding on my side? A terminology thing? To > me, the "SPI transfer" is the total payload and may have any > arbitrary length. What you check for is a constraint on the > transfer's length derived from or based on the "word length" > ('word' in SPI context). > So the code may be appropriate, yet the description may need an > update, to not have the next person ask the same questions again. It seems fairly clear to me - if we're transferring 16 bit words we need the transfer to me a multiple of 16 bits and so on? The requirement for padding is unclear I have to say.
On Tue, Feb 18, 2014 at 09:09 +0900, Mark Brown wrote: > > On Sun, Feb 16, 2014 at 03:25:12PM +0100, Gerhard Sittig wrote: > > On Sat, Feb 15, 2014 at 08:09 +0200, Ivan T. Ivanov wrote: > > > - the total length of the SPI transfer cannot be empty (which I'd > > consider an optimization, not a violation, and may need a > > separate discussion) > > We probably want to allow that for people doing fun stuff with cs_change > though I'm not convinced anything doing that is actually a good idea. > > > - the total length of the SPI transfer must be such that each > > "word" must be provided within a full 1/2/4 byte entity, with > > padding bits if the bits-per-word is "odd" > > > Is this a misunderstanding on my side? A terminology thing? To > > me, the "SPI transfer" is the total payload and may have any > > arbitrary length. What you check for is a constraint on the > > transfer's length derived from or based on the "word length" > > ('word' in SPI context). > > > So the code may be appropriate, yet the description may need an > > update, to not have the next person ask the same questions again. > > It seems fairly clear to me - if we're transferring 16 bit words we need > the transfer to me a multiple of 16 bits and so on? The requirement for > padding is unclear I have to say. I meant "padding" in the sense that e.g. 12bit bits-per-word require data to be provided or consumed in 16bit quantities (2 full bytes), 20bit bits-per-word require 4 bytes per SPI word. Why not 3 bytes? I'd guess this is due to FIFO port width. At least this is how I read the check which this patch implements. virtually yours Gerhard Sittig
On Tue, Feb 18, 2014 at 08:37:47PM +0100, Gerhard Sittig wrote: > On Tue, Feb 18, 2014 at 09:09 +0900, Mark Brown wrote: > > It seems fairly clear to me - if we're transferring 16 bit words we need > > the transfer to me a multiple of 16 bits and so on? The requirement for > > padding is unclear I have to say. > I meant "padding" in the sense that e.g. 12bit bits-per-word > require data to be provided or consumed in 16bit quantities (2 > full bytes), 20bit bits-per-word require 4 bytes per SPI word. > Why not 3 bytes? I'd guess this is due to FIFO port width. I do tend to agree that things need to be rounded up just to the nearest byte, I'm not sure what the standard thing for hardware to require in terms of port sizing is here - it's going to need an audit of the users to see if we need to worry and what they do if it is an issue I think. > At least this is how I read the check which this patch > implements. I don't recall the actual code now, sorry - I was mostly just following up on the review comment here.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index d0b28bb..9bed365 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1617,6 +1617,7 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message) { struct spi_master *master = spi->master; struct spi_transfer *xfer; + int w_size, n_words; if (list_empty(&message->transfers)) return -EINVAL; @@ -1668,6 +1669,22 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message) return -EINVAL; } + /* + * SPI transfer length should be multiple of SPI word size + * where SPI word size should be power-of-two multiple + */ + if (xfer->bits_per_word <= 8) + w_size = 1; + else if (xfer->bits_per_word <= 16) + w_size = 2; + else + w_size = 4; + + n_words = xfer->len / w_size; + /* No partial transfers accepted */ + if (!n_words || xfer->len % xfer->bits_per_word) + return -EINVAL; + if (xfer->speed_hz && master->min_speed_hz && xfer->speed_hz < master->min_speed_hz) return -EINVAL;