Re: [PATCH] Remove attempt by intel-spi-pci to turn the SPI flash chip writeable

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Mon, Aug 03, 2020 at 10:44:49AM -0300, Daniel Gutson wrote:
> Currently, the intel-spi-pci driver tries to unconditionally set
> the SPI chip writeable. After discussing in the LKML, the original
> author decided that it was better  to remove the attempt.
> 
> Context, the intel-spi has a module argument that controls
> whether the driver attempts to turn the SPI flash chip writeable.
> The default value is FALSE (don't try to make it writeable).
> However, this flag applies only for a number of devices, coming from the
> platform driver, whereas the devices detected through the PCI driver
> (intel-spi-pci) are not subject to this check since the configuration
> takes place in intel-spi-pci which doesn't have an argument.
> 
> This patch removes the code that attempts to turn the SPI chip writeable.

I think you should make the $subject to follow the convention used in
the SPI-NOR subsystem. You can see it running following command:

  $ git log --oneline drivers/mtd/spi-nor/controllers/intel-spi-pci.c

In this case I think it should be:

  mtd: spi-nor: intel-spi: Do not try to make the SPI flash chip writable

or something like that.

The patch itself looks good, one minor comment below.

> 
> Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com>
> ---
>  drivers/mtd/spi-nor/controllers/intel-spi-pci.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> index 81329f680bec..d721ba4e8b41 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -41,13 +41,7 @@ static int intel_spi_pci_probe(struct pci_dev *pdev,
>  	if (!info)
>  		return -ENOMEM;
>  
> -	/* Try to make the chip read/write */
>  	pci_read_config_dword(pdev, BCR, &bcr);
> -	if (!(bcr & BCR_WPD)) {
> -		bcr |= BCR_WPD;
> -		pci_write_config_dword(pdev, BCR, bcr);
> -		pci_read_config_dword(pdev, BCR, &bcr);
> -	}
>  	info->writeable = !!(bcr & BCR_WPD);

Perhaps we should log this in debug level (dev_dbg()) so when debugging
possible issues we can see that the chip is write protected by the BIOS.

>  
>  	ispi = intel_spi_probe(&pdev->dev, &pdev->resource[0], info);
> -- 
> 2.25.1