Re: [PATCH 2/3] mtd: spi-nor: intel-spi: Convert to SPI MEM

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Thu, Sep 30, 2021 at 1:08 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> The preferred way to implement SPI-NOR controller drivers is through SPI
> subsubsystem utilizing the SPI MEM core functions. This converts the
> Intel SPI flash controller driver over the SPI MEM by moving the driver
> from SPI-NOR subsystem to SPI subsystem and in one go make it use the
> SPI MEM functions. The driver name will be changed from intel-spi to
> spi-intel to match the convention used in the SPI subsystem.

...

> +config SPI_INTEL_PCI
> +       tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> +       depends on PCI && (X86 || COMPILE_TEST)

Perhaps two entries, one of which will be the same as for platform case?

> +       depends on SPI_MEM
> +       select SPI_INTEL
> +       help
> +         This enables PCI support for the Intel PCH/PCU SPI controller in
> +         master mode. This controller is present in modern Intel hardware
> +         and is used to hold BIOS and other persistent settings. Using
> +         this driver it is possible to upgrade BIOS directly from Linux.
> +
> +         Say N here unless you know what you are doing. Overwriting the
> +         SPI flash may render the system unbootable.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called spi-intel-pci.
> +
> +config SPI_INTEL_PLATFORM
> +       tristate "Intel PCH/PCU SPI flash platform driver (DANGEROUS)"
> +       depends on X86 || COMPILE_TEST
> +       depends on SPI_MEM
> +       select SPI_INTEL
> +       help
> +         This enables platform support for the Intel PCH/PCU SPI
> +         controller in master mode. This controller is present in modern
> +         Intel hardware and is used to hold BIOS and other persistent
> +         settings. Using this driver it is possible to upgrade BIOS
> +         directly from Linux.
> +
> +         Say N here unless you know what you are doing. Overwriting the
> +         SPI flash may render the system unbootable.
> +
> +         To compile this driver as a module, choose M here: the module
> +         will be called spi-intel-platform.

...

+ Blank line ?

>  #include <linux/mtd/partitions.h>
>  #include <linux/mtd/spi-nor.h>

+ Blank line?

> +#include <linux/spi/flash.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>

The rationale is to show that we use two sub(sub)sytems here.

...

> -                       dev_err(ispi->dev, "read error: %llx: %#x\n", from,
> +                       dev_err(ispi->dev, "read error: %x: %#x\n", from,
>                                 status);

Now one line?

...

> -                       dev_err(ispi->dev, "write error: %llx: %#x\n", to,
> +                       dev_err(ispi->dev, "write error: %x: %#x\n", to,
>                                 status);

Ditto.

...

> +               ret = intel_spi_sw_cycle(ispi, opcode, 0,
> +                                        OPTYPE_WRITE_WITH_ADDR);
> +               return ret ? ret : 0;

Why not simply return intel_spi_dw_cycle(...); ?

...

> +       val = readl(ispi->base + HSFSTS_CTL);
> +       val &= ~(HSFSTS_CTL_FDBC_MASK | HSFSTS_CTL_FCYCLE_MASK);
> +       val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;

> +       val |= cmd;
> +       val |= HSFSTS_CTL_FGO;

Maybe swap these lines to group constants?

...

> +       status = readl(ispi->base + HSFSTS_CTL);
> +       if (status & HSFSTS_CTL_FCERR)
> +               return -EIO;

> +       else if (status & HSFSTS_CTL_AEL)

Redundant 'else'

> +               return -EACCES;

...

> +static int intel_spi_exec_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> +{
> +       struct intel_spi *ispi = spi_master_get_devdata(mem->spi->master);
> +       size_t nbytes = op->data.nbytes;
> +       u8 opcode = op->cmd.opcode;
> +
> +       if (op->addr.nbytes) {
> +               if  (op->data.dir == SPI_MEM_DATA_IN)
> +                       return intel_spi_read(ispi, op->addr.val, nbytes,
> +                                             op->data.buf.in);

> +               else if (op->data.dir == SPI_MEM_DATA_OUT)

Redundant 'else' here and nearby.

> +                       return intel_spi_write(ispi, op->addr.val, nbytes,
> +                                              op->data.buf.out);
> +               else if (op->data.dir == SPI_MEM_NO_DATA)
> +                       return intel_spi_erase(ispi, opcode, op->addr.val);
> +       } else {
> +               if  (op->data.dir == SPI_MEM_DATA_IN)
> +                       return intel_spi_read_reg(ispi, opcode, op->data.buf.in,
> +                                                 nbytes);
> +               else if (op->data.dir == SPI_MEM_DATA_OUT)
> +                       return intel_spi_write_reg(ispi, opcode, op->data.buf.out,
> +                                                  nbytes);
> +               else if (op->data.dir == SPI_MEM_NO_DATA)
> +                       return intel_spi_write_reg(ispi, opcode, NULL, 0);
>         }

> +       return -EINVAL;
> +}

-- 
With Best Regards,
Andy Shevchenko