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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ea936063-2a24-406d-a7c6-f832a72d5da5@quicinc.com>
Date: Fri, 25 Apr 2025 16:24:25 +0800
From: Haixu Cui <quic_haixcui@...cinc.com>
To: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>, <broonie@...nel.org>,
        <virtio-dev@...ts.oasis-open.org>, <viresh.kumar@...aro.org>,
        <linux-spi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <hdanton@...a.com>, <qiang4.zhang@...ux.intel.com>,
        <alex.bennee@...aro.org>
CC: <quic_ztu@...cinc.com>
Subject: Re: [RFC PATCH v4 2/3] virtio-spi: Add virtio-spi.h

Hi Mukesh,


>> + * @mode_func_supported: indicates the following features are 
>> supported or not:
> mode_func_supported[b'6-0] : something like this may help to know size 
> of this variable.

I noticed the suggestion to use [b'6-0] to indicate the actual size of 
the mode_func_supported variable. However I haven't seen notation like
[b'6-0] used in Linux kernel.

And I think the definition of its bitfield below could clearly indicates 
that 7 bits of mode_func_supported are used. Could we keep the current 
description as it is?

>> + *   bit 0-1: CPHA feature
>> + *     0b00: invalid, should support as least one CPHA setting
>> + *     0b01: supports CPHA=0 only
>> + *     0b10: supports CPHA=1 only
>> + *     0b11: supports CPHA=0 and CPHA=1.
>> + *   bit 2-3: CPOL feature
>> + *     0b00: invalid, should support as least one CPOL setting
>> + *     0b01: supports CPOL=0 only
>> + *     0b10: supports CPOL=1 only
>> + *     0b11: supports CPOL=0 and CPOL=1.
>> + *   bit 4: chipselect active high feature, 0 for unsupported and 1 for
>> + *     supported, chipselect active low should always be supported.
> You mean to say "chipselect active low is default supported" ?
> 
> Just thinking instead of keeping always supported, can we mentione as 
> default supported ?

Yes, will update as "chipselect active low is supported by default".

> 
>> + *   bit 5: LSB first feature, 0 for unsupported and 1 for supported,
>> + *     MSB first should always be supported.
> MSB first is default supported ?

Yes.

>> + *   bit 6: loopback mode feature, 0 for unsupported and 1 for 
>> supported,
>> + *     normal mode should always be supported.
> we can reverse the write up for all "always be supported"
> 
> bit 6: if not specified, normal mode is supported by default. if set 1, 
> specifies loopback mode.

Sure, your statement is indeed clearer and more accurate, I will update 
in next patch.


>> +#define VIRTIO_SPI_RX_TX_SUPPORT_DUAL (1 << 0)
>> +#define VIRTIO_SPI_RX_TX_SUPPORT_QUAD (1 << 1)
>> +#define VIRTIO_SPI_RX_TX_SUPPORT_OCTAL (1 << 2)
> Can use BIT(x) ?
Will update the code accordingly:
#define VIRTIO_SPI_RX_TX_SUPPORT_DUAL    BIT(0)
#define VIRTIO_SPI_RX_TX_SUPPORT_QUAD    BIT(1)


Really Appreciate.

Best Regards
Haixu Cui