lkml.org 
[lkml]   [2020]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: Re: [PATCH] spi: spi-amd: Add AMD SPI controller driver support
From
Date


On 4/14/2020 4:46 PM, Mark Brown wrote:
> On Sun, Apr 12, 2020 at 02:28:31PM -0500, Sanjay R Mehta wrote:
>
>> +++ b/drivers/spi/spi-amd.c
>> @@ -0,0 +1,341 @@
>> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
>> +/*
>> + * AMD SPI controller driver
>> + *
>
> Please make the entire comment a C++ one so things look more
> intentional.
>
>> +#define DRIVER_NAME "amd_spi"
>
> This is unused.
>
>> +/* M_CMD OP codes for SPI */
>> +#define SPI_XFER_TX 1
>> +#define SPI_XFER_RX 2
>
> These constants should be namespaced, they're likely to collide with
> generic additions.
>
>> +static void amd_spi_execute_opcode(struct spi_master *master)
>> +{
>> + struct amd_spi *amd_spi = spi_master_get_devdata(master);
>> + bool spi_busy;
>> +
>> + /* Set ExecuteOpCode bit in the CTRL0 register */
>> + amd_spi_setclear_reg32(master, AMD_SPI_CTRL0_REG, AMD_SPI_EXEC_CMD,
>> + AMD_SPI_EXEC_CMD);
>> +
>> + /* poll for SPI bus to become idle */
>> + spi_busy = (ioread32((u8 __iomem *)amd_spi->io_remap_addr +
>> + AMD_SPI_CTRL0_REG) & AMD_SPI_BUSY) == AMD_SPI_BUSY;
>> + while (spi_busy) {
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + schedule();
>> + set_current_state(TASK_RUNNING);
>> + spi_busy = (ioread32((u8 __iomem *)amd_spi->io_remap_addr +
>> + AMD_SPI_CTRL0_REG) & AMD_SPI_BUSY) == AMD_SPI_BUSY;
>> + }
>
> This is a weird way to busy wait - usually you'd use a cpu_relax()
> rather than a schedule(). There's also no timeout here so we could busy
> wait for ever if something goes wrong.
>
>> +static int amd_spi_master_setup(struct spi_device *spi)
>> +{
>> + struct spi_master *master = spi->master;
>> + struct amd_spi *amd_spi = spi_master_get_devdata(master);
>> +
>> + amd_spi->chip_select = spi->chip_select;
>> + amd_spi_select_chip(master);
>
> This looks like it will potentially affect devices other than the
> current one. setup() may be called while other devices are active it
> shouldn't do that.
>
>> + } else if (m_cmd & SPI_XFER_RX) {
>> + /* Store no. of bytes to be received from
>> + * FIFO
>> + */
>> + rx_len = xfer->len;
>> + buffer = (u8 *)xfer->rx_buf;
>
>> + /* Read data from FIFO to receive buffer */
>> + for (i = 0; i < rx_len; i++)
>> + buffer[i] = ioread8((u8 __iomem *)amd_spi->io_remap_addr
>> + + AMD_SPI_FIFO_BASE
>> + + tx_len + i);
>
> This will only work for messages with a single receive transfer, if
> there are multiple transfers then you'll need to store multiple buffers
> and their lengths.
>
>> +static int amd_spi_master_transfer(struct spi_master *master,
>> + struct spi_message *msg)
>> +{
>> + struct amd_spi *amd_spi = spi_master_get_devdata(master);
>> +
>> + /*
>> + * Extract spi_transfers from the spi message and
>> + * program the controller.
>> + */
>> + amd_spi_fifo_xfer(amd_spi, msg);
>> +
>> + return 0;
>> +}
>
> This function is completely redundant, just inline amd_spi_fifo_xfer().
> It also ignores all errors which isn't great.
>
>> + /* Initialize the spi_master fields */
>> + master->bus_num = 0;
>> + master->num_chipselect = 4;
>> + master->mode_bits = 0;
>> + master->flags = 0;
>
> This device is single duplex so should flag that.
>
>> + err = spi_register_master(master);
>> + if (err) {
>> + dev_err(dev, "error registering SPI controller\n");
>> + goto err_iounmap;
>
> It's best to print the error code to help people debug things.

Thanks Mark for the feedback. Will make all the suggested changes.
>

\
 
 \ /
  Last update: 2020-04-14 13:38    [W:1.457 / U:0.140 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site