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

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 20 May 2021 19:33:52 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@....com>
Cc:     linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Shyam Sundar S K <Shyam-sundar.S-k@....com>,
        Liang Liang <liang.liang@....com>
Subject: Re: [PATCH] spi:amd: Add support for latest platform

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.

Download attachment "signature.asc" of type "application/pgp-signature" (485 bytes)