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

[Haixu Cui]

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