[PATCH] spi: add bits_per_word_mask check in spi_setup

[PATCH] spi: add bits_per_word_mask check in spi_setup

[Scott Jiang]

The spi_setup should fail if underlying controller or its driver
does not support requested bits_per_word. So if the master driver
set mask and let core do bits_per_word check then we should also
test this mask in spi_setup.

Signed-off-by: Scott Jiang <scott.jiang.linux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 45cb6a9..dc8fde0 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1326,6 +1326,15 @@ int spi_setup(struct spi_device *spi)
 	if (!spi->bits_per_word)
 		spi->bits_per_word = 8;
 
+	if (spi->master->bits_per_word_mask) {
+		/* Only 32 bits fit in the mask */
+		if (spi->bits_per_word > 32)
+			return -EINVAL;
+		if (!(spi->master->bits_per_word_mask &
+				BIT(spi->bits_per_word - 1)))
+			return -EINVAL;
+	}
+
 	if (spi->master->setup)
 		status = spi->master->setup(spi);
 
-- 
1.7.0.4

Re: [PATCH] spi: add bits_per_word_mask check in spi_setup

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 782 bytes --]

On Wed, Jun 26, 2013 at 06:54:46PM -0400, Scott Jiang wrote:

> +	if (spi->master->bits_per_word_mask) {
> +		/* Only 32 bits fit in the mask */
> +		if (spi->bits_per_word > 32)
> +			return -EINVAL;
> +		if (!(spi->master->bits_per_word_mask &
> +				BIT(spi->bits_per_word - 1)))
> +			return -EINVAL;
> +	}

Thinking about this a bit it might reject a valid transfer - it only
checks the setting in the SPI device, not the setting in the transfer.
This means that if the transfer overrides an unsuitable setting in the
controller then an error will be generated as the fixed setting won't be
seen.  It's hard to imagine when that would happen without something
being very confused but still feels better to check the bits per word
from the first transfer too.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 213 bytes --]

_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b@public.gmane.org
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel

Re: [PATCH] spi: add bits_per_word_mask check in spi_setup

[Scott Jiang]

>> +     if (spi->master->bits_per_word_mask) {
>> +             /* Only 32 bits fit in the mask */
>> +             if (spi->bits_per_word > 32)
>> +                     return -EINVAL;
>> +             if (!(spi->master->bits_per_word_mask &
>> +                             BIT(spi->bits_per_word - 1)))
>> +                     return -EINVAL;
>> +     }
>
> Thinking about this a bit it might reject a valid transfer - it only
> checks the setting in the SPI device, not the setting in the transfer.
> This means that if the transfer overrides an unsuitable setting in the
> controller then an error will be generated as the fixed setting won't be
> seen.  It's hard to imagine when that would happen without something
> being very confused but still feels better to check the bits per word
> from the first transfer too.

But the API description said that this call would fail if the protocol
driver specifies an option
that the underlying controller or its driver does not support. It's
strange if I request 9 bits per
word in setup successfully while calling it in transfer fail.

------------------------------------------------------------------------------
This SF.net email is sponsored by Windows:

Build for Windows Store.

http://p.sf.net/sfu/windows-dev2dev

Re: [PATCH] spi: add bits_per_word_mask check in spi_setup

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 642 bytes --]

On Fri, Jun 28, 2013 at 10:51:44AM +0800, Scott Jiang wrote:

> > Thinking about this a bit it might reject a valid transfer - it only
> > checks the setting in the SPI device, not the setting in the transfer.

> But the API description said that this call would fail if the protocol
> driver specifies an option
> that the underlying controller or its driver does not support. It's
> strange if I request 9 bits per
> word in setup successfully while calling it in transfer fail.

Yes, it's a bit odd (and I'd never expect a failure to actually trigger
here that wasn't going to be repeated in the actual transfer) but it
seems more robust.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 213 bytes --]

_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel-ZG0+EudsQA8dtHy/vicBwGD2FQJk+8+b@public.gmane.org
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel