Re: [PATCH v4 2/3] virtio-spi: Add virtio-spi.h

On 8/28/2025 6:34 PM, Michael S. Tsirkin wrote:
> On Mon, Aug 25, 2025 at 05:19:03PM +0800, Haixu Cui wrote:
>>
>>
>> On 8/21/2025 4:45 PM, Michael S. Tsirkin wrote:
>>
>>
>>>> +
>>>> +/* Sample data on trailing clock edge */
>>>> +#define VIRTIO_SPI_CPHA			_BITUL(0)
>>>> +/* Clock is high when IDLE */
>>>> +#define VIRTIO_SPI_CPOL			_BITUL(1)
>>>> +/* Chip Select is active high */
>>>> +#define VIRTIO_SPI_CS_HIGH			_BITUL(2)
>>>> +/* Transmit LSB first */
>>>> +#define VIRTIO_SPI_MODE_LSB_FIRST		_BITUL(3)
>>>> +/* Loopback mode */
>>>> +#define VIRTIO_SPI_MODE_LOOP			_BITUL(4)
>>>
>>> It is generally preferable to have an enum with just bit
>>> numbers.
>>>
>>>
>>> E.g.
>>>
>>> enum {
>>> VIRTIO_SPI_F_CPHA = 0,
>>> }
>>>
>>>
>>> Userspace can add _BITUL wrappers itself if it
>>> wants.
>>>
>>>
>>
>> Hi Michael,
>>
>> Thank you for the suggestion regarding the bit definitions.
>>
>> Would it be acceptable to keep the current macro definitions with _BITUL()
>> because these macros are also used within the virtio SPI driver itself?
>>
>> Looking forward to your guidance.
>>
>> Best regards,
>> Haixu Cui
>>
> 
> 
> move them to the .c file if you want them.
> 

Hi Michael,
I've observed that other virtio drivers - such as virtio-i2c and 
virtio-net - commonly define feature bits directly in their headers and 
use them in their drivers.

To maintain alignment with those drivers and ensure consistency in 
usage, I’d prefer to retain the current macro-based approach for 
defining feature bits.

Do you think this would be reasonable in this case?

Thanks & BR