Re: [virtio-dev][PATCH V9 2/2] virtio-spi: add the device specification

[Haixu Cui <quic_haixcui@quicinc.com>]

Hello Harald,

On 12/21/2023 7:25 PM, Harald Mommer wrote:
> Hello,
> 
> On 12.12.23 04:33, Haixu Cui wrote:
>> +\field{bits_per_word_mask} is a mask indicating which values of 
>> bits_per_word are supported.
>> +If bit n of \field{bits_per_word_mask} is set, the bits_per_word with 
>> value (n+1) is supported.
>> +If all bits are not set, bits_per_word can be any value from 1 to 32.
> 
> "If all bits are not set" is not easy to understand as it means "if no 
> bit is set".
> 
> You can of course specify the value 0 this way to make code more 
> complicated.
> 
> To make code simple, 0 would be not given this special meaning. To allow 
> any value between 1 and 32 solely the value 0xFFFFFFFF would be allowed. 
> Which is already possible now without this additional sentence for 0. 
> Simply checking whether the respective bit is set in the mask and done 
> without having to add senseless additional code to handle the special 
> case that the mask is 0.
> 
> Giving 0 the same meaning as 0xFFFFFFFF brings nothing except confusion.
> 
Here I refer to the __spi_validate_bits_per_word in drivers/spi/spi.c, 
if bits_per_word_mask of spi controller is 0, no check will be done for 
bits_per_word parameter.

Above is just the Linux mechanism, I agree with you, 0 and 0xFFFFFFFF 
have the same meaning will cause confusion in this spec. I prefer 
treating 0 as an invalid value because the backend must support at least 
one bits_per_word value.

How about updating as:
For \field{bits_per_word_mask}, 0 is invalid because at least one 
bits_per_word value should be supported.

Thanks & Regards
Haixu Cui

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org