Re: [PATCH v5 3/3] spi: Add spi driver for Socionext Synquacer platform

[Masahisa Kojima <masahisa.kojima@xxxxxxxxxx>]

Thank you for your comments.

On Wed, 22 May 2019 at 01:38, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> On Tue, May 21, 2019 at 3:00 PM Masahisa Kojima
> <masahisa.kojima@xxxxxxxxxx> wrote:
> >
> > This patch adds support for controller found on synquacer platforms.
>
> > +       case 8:
> > +               {
> > +               u8 *buf = sspi->rx_buf;
> > +
> > +               readsb(sspi->regs + SYNQUACER_HSSPI_REG_RX_FIFO, buf, len);
> > +               sspi->rx_buf = buf + len;
> > +               break;
> > +               }
>
> Slightly better style
> case FOO: {
>   ...
> }
>
> > +       /* Full Duplex only on 1-bit wide bus */
> > +       if (xfer->rx_buf && xfer->tx_buf &&
> > +           (xfer->rx_nbits != 1 || xfer->tx_nbits != 1)) {
> > +               dev_err(sspi->dev,
> > +                       "RX and TX bus widths must match for Full-Duplex!\n");
>
> The message is not telling full truth. Not only match, but also be equal 1.
>
> > +               return -EINVAL;
> > +       }
>
> > +static int synquacer_spi_transfer_one(struct spi_master *master,
> > +                                     struct spi_device *spi,
> > +                                     struct spi_transfer *xfer)
> > +{
> > +       struct synquacer_spi *sspi = spi_master_get_devdata(master);
> > +       int ret;
> > +       int status = 0;
> > +       unsigned int words;
> > +       u8 bpw;
> > +       u32 val;
> > +
> > +       val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG);
> > +       val |= SYNQUACER_HSSPI_FIFOCFG_RX_FLUSH;
> > +       val |= SYNQUACER_HSSPI_FIFOCFG_TX_FLUSH;
> > +       writel_relaxed(val, sspi->regs + SYNQUACER_HSSPI_REG_FIFOCFG);
> > +
> > +       /*
> > +        * See if we can transfer 4-bytes as 1 word
> > +        * to maximize the FIFO buffer effficiency
>
> Typo here, and period is missed.
>
> > +        */
>
> > +       case 8:
> > +               words = xfer->len;
> > +               break;
> > +       case 16:
> > +               words = xfer->len / 2;
> > +               break;
> > +       default:
> > +               words = xfer->len / 4;
> > +               break;
>
> Hmm... Shouldn't be rather "less then or equal" comparisons?

As Mark's comment, I will explicitly list the values(8, 16, 24 and 32).

> > +       unsigned int retries = 0xfffff;
>
> Hmm... better to use decimal value.

Instead of implementing retry with counter, I will implement
wait function with checking time_before(jiffies, timeout).

>
> > +       /* Disable module */
> > +       writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_MCTRL);
>
> > +       while ((readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_MCTRL) &
> > +               SYNQUACER_HSSPI_MCTRL_MES) && --retries)
> > +               cpu_relax();
> > +       if (!retries)
> > +               return -EBUSY;
>
> And here something like
> do {
> } while (--retries)
>
> would look slightly better due to understanding that we do at least
> one iteration.
>
> Also, can readx_poll_timeout be used here?
>
> > +static irqreturn_t sq_spi_tx_handler(int irq, void *priv)
> > +{
> > +       uint32_t val;
> > +       struct synquacer_spi *sspi = priv;
> > +
> > +       val = readl_relaxed(sspi->regs + SYNQUACER_HSSPI_REG_TXF);
> > +
> > +       if (val & SYNQUACER_HSSPI_TXF_FIFO_EMPTY) {
> > +               if (sspi->tx_words == 0) {
> > +                       writel_relaxed(0, sspi->regs + SYNQUACER_HSSPI_REG_TXE);
> > +                       complete(&sspi->transfer_done);
>
> > +                       return 0;
>
> irqreturn_t type, please. We have corresponding defines.
>
> > +               }
> > +               write_fifo(sspi);
> > +       }
> > +
> > +       return 0;
>
> Ditto.
>
> > +}
>
> > +static int synquacer_spi_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *np = pdev->dev.of_node;
> > +       struct spi_master *master;
> > +       struct synquacer_spi *sspi;
> > +       struct resource *res;
> > +       int ret;
> > +       int rx_irq, tx_irq;
> > +
> > +       master = spi_alloc_master(&pdev->dev, sizeof(*sspi));
> > +       if (!master)
> > +               return -ENOMEM;
> > +
> > +       platform_set_drvdata(pdev, master);
> > +
> > +       sspi = spi_master_get_devdata(master);
> > +       sspi->dev = &pdev->dev;
> > +
>
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       sspi->regs = devm_ioremap_resource(sspi->dev, res);
>
> devm_platform_ioremap_resource()
>
> > +       if (IS_ERR(sspi->regs)) {
> > +               ret = PTR_ERR(sspi->regs);
> > +               goto put_spi;
> > +       }
>
> > +       } else {
> > +               dev_err(&pdev->dev, "specified wrong clock source\n");
> > +               ret = -EINVAL;
> > +               goto put_spi;
> > +       }
>
> Not an issue for ACPI.
>
> > +       if (IS_ERR(sspi->clk)) {
> > +               dev_err(&pdev->dev, "clock not found\n");
> > +               ret = PTR_ERR(sspi->clk);
> > +               goto put_spi;
> > +       }
> > +
> > +       sspi->aces = of_property_read_bool(np, "socionext,set-aces");
> > +       sspi->rtm = of_property_read_bool(np, "socionext,use-rtm");
> > +
> > +       master->num_chipselect = SYNQUACER_HSSPI_NUM_CHIP_SELECT;
> > +
> > +       init_completion(&sspi->transfer_done);
> > +
> > +       rx_irq = platform_get_irq(pdev, 0);
> > +       if (rx_irq < 0)
> > +               dev_err(&pdev->dev, "get rx_irq failed\n");
> > +
> > +       tx_irq = platform_get_irq(pdev, 1);
> > +       if (tx_irq < 0)
> > +               dev_err(&pdev->dev, "get tx_irq failed\n");
> > +
> > +       ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler,
> > +                               0, "synquacer-spi-rx", sspi);
> > +       ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler,
> > +                               0, "synquacer-spi-tx", sspi);
> > +
> > +       ret = clk_prepare_enable(sspi->clk);
> > +       if (ret)
> > +               goto put_spi;
> > +
> > +       master->dev.of_node = np;
> > +       master->auto_runtime_pm = true;
> > +       master->bus_num = pdev->id;
> > +
> > +       master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_TX_DUAL | SPI_RX_DUAL |
> > +                           SPI_TX_QUAD | SPI_RX_QUAD;
> > +       master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(24) |
> > +                                    SPI_BPW_MASK(16) | SPI_BPW_MASK(8);
> > +
> > +       master->max_speed_hz = clk_get_rate(sspi->clk);
> > +       master->min_speed_hz = master->max_speed_hz / 254;
> > +
> > +       master->set_cs = synquacer_spi_set_cs;
> > +       master->transfer_one = synquacer_spi_transfer_one;
> > +
> > +       ret = synquacer_spi_enable(master);
> > +       if (ret)
> > +               goto fail_enable;
> > +
> > +       pm_runtime_set_active(sspi->dev);
> > +       pm_runtime_enable(sspi->dev);
> > +
> > +       ret = devm_spi_register_master(sspi->dev, master);
> > +       if (ret)
> > +               goto disable_pm;
> > +
> > +       return 0;
> > +
> > +disable_pm:
> > +       pm_runtime_disable(sspi->dev);
> > +fail_enable:
> > +       clk_disable_unprepare(sspi->clk);
> > +put_spi:
> > +       spi_master_put(master);
> > +
> > +       return ret;
> > +}
>
>
> > +       if (!pm_runtime_suspended(dev))
>
> This is not enough to check.

I checked other drivers, but I could find what is missing.
Could you kindly share more details?

>
> > +       if (!pm_runtime_suspended(dev)) {
>
> Ditto.
>
> --
> With Best Regards,
> Andy Shevchenko