Re: [PATCH 1/2] spi: add ability to validate xfer->bits_per_word in SPI core

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 04/01/2013 01:52 PM, Trent Piepho wrote:
> On Tue, Mar 26, 2013 at 7:37 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> Allow SPI masters to define the set of bits_per_word values they support.
>> If they do this, then the SPI core will reject transfers that attempt to
>> use an unsupported bits_per_word value. This eliminates the need for each
>> SPI driver to implement this checking in most cases.

>> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c

>> +               if (master->bits_per_word_mask) {
>> +                       /* Only 32 bits fit in the mask */
>> +                       if (xfer->bits_per_word > 32)
>> +                               return -EINVAL;
>> +                       if (!(master->bits_per_word_mask &
>> +                                       BIT(xfer->bits_per_word - 1)))
>> +                               return -EINVAL;
>> +               }
> 
> This could be simplified to:
> 
>         if (master->bits_per_word_mask &&
>             !(master->bits_per_word_mask & BIT(xfer->bits_per_word - 1)))
>                 return -EINVAL;
> 
> It's not necessary to handle bits_per_word > 32 differently.  The
> resulting mask will either be zero when unsigned long is 32 bits, or
> have a bit set if unsigned long is 64 bits.  Either way, it won't
> match any of the bits set in bits_per_word_mask and the desired result
> is produced.  The extra test for > 32 will just slow down the common
> case.

That's certainly true, and I did consider that. However, I preferred to
be explicit. I imagine the compiler optimizes both to the same thing anyway.

>> @@ -301,6 +306,9 @@ struct spi_master {
>>         /* spi_device.mode flags understood by this controller driver */
>>         u16                     mode_bits;
>>
>> +       /* bitmask of supported bits_per_word for transfers */
>> +       u32                     bits_per_word_mask;
>> +
>>         /* other constraints relevant to this driver */
>>         u16                     flags;
> 
> If the u32 field was before the u16 fields, then the structure would
> have be more likely to not need padding in the future.  There are four
> u16 in front of bits_per_word_mask so it doesn't need padding now
> (however, there are 2 bytes of padding after flags), but if someone
> adds a new 16-bit field in front of bits_per_word_mask then padding
> will be added.

I was assuming whoever added any new fields would put thought into the
alignment; the fields can easily be swapped around to avoid any extra
padding when new fields are added.

------------------------------------------------------------------------------
Own the Future-Intel(R) Level Up Game Demo Contest 2013
Rise to greatness in Intel's independent game demo contest. Compete 
for recognition, cash, and the chance to get your game on Steam. 
$5K grand prize plus 10 genre and skill prizes. Submit your demo 
by 6/6/13. http://altfarm.mediaplex.com/ad/ck/12124-176961-30367-2