Re: [PATCH] spi:amd: Add support for latest platform

[Mark Brown]

On Thu, May 20, 2021 at 07:09:46PM +0530, Nehal Bakulchandra Shah wrote:

> *Support for latest platform
> *Hardware Fifo has 72 bytes limitation so fix for the larger data size.

These two things sound like they should be separate patches, and it
looks like there are some other changes mixed in here which aren't
called out in the changelog.  This should be a patch series with one
change per patch, that makes things much easier to review.

>  	while (spi_busy) {
> -		usleep_range(10, 20);
> +		usleep_range(10, 40);

Why change the delay?  This looks like a separate patch.

>  	return 0;
> @@ -146,9 +180,14 @@ static void amd_spi_execute_opcode(struct spi_master *master)
>  {
>  	struct amd_spi *amd_spi = spi_master_get_devdata(master);
>  
> +	amd_spi_busy_wait(amd_spi);
>  	/* Set ExecuteOpCode bit in the CTRL0 register */

A blank line after the busy wait, and it'd be good to have a comment
saying why there's a busy wait before actually doing the operation since
this looks quite odd.

> @@ -241,7 +325,8 @@ static int amd_spi_master_transfer(struct spi_master *master,
>  	 * program the controller.
>  	 */
>  	amd_spi_fifo_xfer(amd_spi, master, msg);
> -
> +	if (amd_spi->devtype_data->version)
> +		amd_spi_clear_chip(master);

Does this disable the chip select?  Should there be a separate set_cs()
operation?

>  	amd_spi = spi_master_get_devdata(master);
> -	amd_spi->io_remap_addr = devm_platform_ioremap_resource(pdev, 0);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	amd_spi->io_remap_addr = devm_ioremap_resource(&pdev->dev, res);
> +

res is never referenced so it's not clear why this change is being made?

>  	/* Initialize the spi_master fields */
>  	master->bus_num = 0;
>  	master->num_chipselect = 4;
>  	master->mode_bits = 0;
> -	master->flags = SPI_MASTER_HALF_DUPLEX;
>  	master->setup = amd_spi_master_setup;
>  	master->transfer_one_message = amd_spi_master_transfer;

I'm not seeing a change anywhere that looks like it adds full duplex
support for the v1 hardware (or v2 for that matter) and this isn't
called out in the changelog.

Attachment: signature.asc
Description: PGP signature