Re: [Patch V3 1/3] tpm_tis-spi: Support hardware wait polling

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

On Thu, Feb 23, 2023 at 09:56:33PM +0530, Krishna Yarlagadda wrote:

> +       spi_bus_lock(phy->spi_device->master);
> +
> +       while (len) {

Why?

> +		spi_xfer[0].tx_buf = phy->iobuf;
> +		spi_xfer[0].len = 1;
> +		spi_message_add_tail(&spi_xfer[0], &m);
> +
> +		spi_xfer[1].tx_buf = phy->iobuf + 1;
> +		spi_xfer[1].len = 3;
> +		spi_message_add_tail(&spi_xfer[1], &m);

Why would we make these two separate transfers?

> +		if (out) {
> +			spi_xfer[2].tx_buf = &phy->iobuf[4];
> +			spi_xfer[2].rx_buf = NULL;
> +			memcpy(&phy->iobuf[4], out, transfer_len);
> +			out += transfer_len;
> +		}
> +
> +		if (in) {
> +			spi_xfer[2].tx_buf = NULL;
> +			spi_xfer[2].rx_buf = &phy->iobuf[4];
> +		}

This will use the same buffer for rx and tx if some bug manages to leave
them both set.  That shouldn't be an issue but it's an alarm bell
reading the code.

> index 988aabc31871..b88494e31239 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -184,8 +184,9 @@ struct spi_device {
>  	u8			chip_select;
>  	u8			bits_per_word;
>  	bool			rt;
> -#define SPI_NO_TX	BIT(31)		/* No transmit wire */
> -#define SPI_NO_RX	BIT(30)		/* No receive wire */
> +#define SPI_NO_TX		BIT(31)		/* No transmit wire */
> +#define SPI_NO_RX		BIT(30)		/* No receive wire */
> +#define SPI_TPM_HW_FLOW		BIT(29)		/* TPM flow control */

Additions to the SPI API should be a separate commit for SPI rather than
merged into a driver change.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]