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

[Masahisa Kojima <masahisa.kojima@xxxxxxxxxx>]

Hi Ard,

Thank you for your comments.

On Thu, 30 May 2019 at 16:17, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>
> On Tue, 28 May 2019 at 11:27, Masahisa Kojima
> <masahisa.kojima@xxxxxxxxxx> wrote:
> >
> > This patch adds support for controller found on synquacer platforms.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@xxxxxxxxxx>
> > Signed-off-by: Jassi Brar <jaswinder.singh@xxxxxxxxxx>
> > ---
> >  drivers/spi/Kconfig         |  11 +
> >  drivers/spi/Makefile        |   1 +
> >  drivers/spi/spi-synquacer.c | 826 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 838 insertions(+)
> >  create mode 100644 drivers/spi/spi-synquacer.c
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index 0fba8f400c59..24a3eac72d12 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -732,6 +732,17 @@ config SPI_SUN6I
> >         help
> >           This enables using the SPI controller on the Allwinner A31 SoCs.
> >
> > +config SPI_SYNQUACER
> > +       tristate "Socionext's Synquacer HighSpeed SPI controller"
> > +       depends on ARCH_SYNQUACER || COMPILE_TEST
> > +       select SPI_BITBANG
>
> Do we really need this dependency?

No, I remove dependency to SPI_BITBANG.

I will also fix other comments.

> > +       help
> > +         SPI driver for Socionext's High speed SPI controller which provides
> > +         various operating modes for interfacing to serial peripheral devices
> > +         that use the de-facto standard SPI protocol.
> > +
> > +         It also supports the new dual-bit and quad-bit SPI protocol.
> > +
> >  config SPI_MXIC
> >          tristate "Macronix MX25F0A SPI controller"
> >          depends on SPI_MASTER
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index f2f78d03dc28..63dcab552bcb 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -106,6 +106,7 @@ obj-$(CONFIG_SPI_STM32_QSPI)                += spi-stm32-qspi.o
> >  obj-$(CONFIG_SPI_ST_SSC4)              += spi-st-ssc4.o
> >  obj-$(CONFIG_SPI_SUN4I)                        += spi-sun4i.o
> >  obj-$(CONFIG_SPI_SUN6I)                        += spi-sun6i.o
> > +obj-$(CONFIG_SPI_SYNQUACER)            += spi-synquacer.o
> >  obj-$(CONFIG_SPI_TEGRA114)             += spi-tegra114.o
> >  obj-$(CONFIG_SPI_TEGRA20_SFLASH)       += spi-tegra20-sflash.o
> >  obj-$(CONFIG_SPI_TEGRA20_SLINK)                += spi-tegra20-slink.o
> > diff --git a/drivers/spi/spi-synquacer.c b/drivers/spi/spi-synquacer.c
> > new file mode 100644
> > index 000000000000..27a9babaffc0
> > --- /dev/null
> > +++ b/drivers/spi/spi-synquacer.c
> > @@ -0,0 +1,826 @@
> ...
> > +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;
> > +       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;
> > +
> > +       init_completion(&sspi->transfer_done);
> > +
> > +       sspi->regs = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(sspi->regs)) {
> > +               ret = PTR_ERR(sspi->regs);
> > +               goto put_spi;
> > +       }
> > +
> > +       sspi->clk_src_type = SYNQUACER_HSSPI_CLOCK_SRC_IHCLK; /* Default */
> > +       device_property_read_u32(&pdev->dev, "socionext,ihclk-rate",
> > +                                &master->max_speed_hz); /* for ACPI */
> > +
> > +       if (dev_of_node(&pdev->dev)) {
> > +               if (device_property_match_string(&pdev->dev,
> > +                                        "clock-names", "iHCLK") >= 0) {
> > +                       sspi->clk_src_type = SYNQUACER_HSSPI_CLOCK_SRC_IHCLK;
> > +                       sspi->clk = devm_clk_get(sspi->dev, "iHCLK");
> > +               } else if (device_property_match_string(&pdev->dev,
> > +                                               "clock-names", "iPCLK") >= 0) {
> > +                       sspi->clk_src_type = SYNQUACER_HSSPI_CLOCK_SRC_IPCLK;
> > +                       sspi->clk = devm_clk_get(sspi->dev, "iPCLK");
> > +               } else {
> > +                       dev_err(&pdev->dev, "specified wrong clock source\n");
> > +                       ret = -EINVAL;
> > +                       goto put_spi;
> > +               }
> > +               if (IS_ERR(sspi->clk)) {
>
> You could have received an -EPROBE_DEFER return value here, in which
> case you should not print an error and just return.
>
> > +                       dev_err(&pdev->dev, "clock not found\n");
> > +                       ret = PTR_ERR(sspi->clk);
> > +                       goto put_spi;
> > +               }
> > +
> > +               ret = clk_prepare_enable(sspi->clk);
> > +               if (ret) {
> > +                       dev_err(&pdev->dev, "failed to enable clock (%d)\n",
> > +                               ret);
> > +                       goto put_spi;
> > +               }
> > +
> > +               master->max_speed_hz = clk_get_rate(sspi->clk);
> > +       }
> > +
> > +       if (!master->max_speed_hz) {
> > +               dev_err(&pdev->dev, "missing clock source\n");
> > +               return -EINVAL;
> > +       }
> > +       master->min_speed_hz = master->max_speed_hz / 254;
> > +
> > +       sspi->aces = device_property_read_bool(&pdev->dev,
> > +                                              "socionext,set-aces");
> > +       sspi->rtm = device_property_read_bool(&pdev->dev, "socionext,use-rtm");
> > +
> > +       master->num_chipselect = SYNQUACER_HSSPI_NUM_CHIP_SELECT;
> > +
> > +       rx_irq = platform_get_irq(pdev, 0);
> > +       if (rx_irq < 0) {
>
> <= 0
>
> > +               dev_err(&pdev->dev, "get rx_irq failed (%d)\n", rx_irq);
> > +               ret = rx_irq;
> > +               goto put_spi;
> > +       }
> > +       snprintf(sspi->rx_irq_name, SYNQUACER_HSSPI_IRQ_NAME_MAX, "%s-%s",
>
> Why not just use "%s-rx" as the format string?
>
> > +                dev_name(&pdev->dev), "rx");
> > +       ret = devm_request_irq(&pdev->dev, rx_irq, sq_spi_rx_handler,
> > +                               0, sspi->rx_irq_name, sspi);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "request rx_irq failed (%d)\n", ret);
> > +               goto put_spi;
> > +       }
> > +
> > +       tx_irq = platform_get_irq(pdev, 1);
> > +       if (tx_irq < 0) {
>
> <= 0
>
> > +               dev_err(&pdev->dev, "get tx_irq failed (%d)\n", tx_irq);
> > +               ret = tx_irq;
> > +               goto put_spi;
> > +       }
> > +       snprintf(sspi->tx_irq_name, SYNQUACER_HSSPI_IRQ_NAME_MAX, "%s-%s",
>
> Likewise
>
> > +                dev_name(&pdev->dev), "tx");
> > +       ret = devm_request_irq(&pdev->dev, tx_irq, sq_spi_tx_handler,
> > +                               0, sspi->tx_irq_name, sspi);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "request tx_irq failed (%d)\n", ret);
> > +               goto put_spi;
> > +       }
> > +
> > +       master->dev.of_node = np;
>
> please add
>
> master->dev.fwnode = pdev->dev.fwnode;
>
> here as well
>
> > +       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->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:
> > +       if (!IS_ERR(sspi->clk))
>
> You can drop this check: clk_disable_unprepare() already ignores NULL
> or ERR values.
>
> > +               clk_disable_unprepare(sspi->clk);
> > +put_spi:
> > +       spi_master_put(master);
> > +
> > +       return ret;
> > +}
> > +
> > +static int synquacer_spi_remove(struct platform_device *pdev)
> > +{
> > +       struct spi_master *master = platform_get_drvdata(pdev);
> > +       struct synquacer_spi *sspi = spi_master_get_devdata(master);
> > +
> > +       pm_runtime_disable(sspi->dev);
> > +
> > +       if (!IS_ERR(sspi->clk))
> > +               clk_disable_unprepare(sspi->clk);
> > +
>
> Same here
>
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused synquacer_spi_suspend(struct device *dev)
> > +{
> > +       struct spi_master *master = dev_get_drvdata(dev);
> > +       struct synquacer_spi *sspi = spi_master_get_devdata(master);
> > +       int ret;
> > +
> > +       ret = spi_master_suspend(master);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (!pm_runtime_suspended(dev))
> > +               if (!IS_ERR(sspi->clk))
> > +                       clk_disable_unprepare(sspi->clk);
> > +
>
> ... and here
>
> > +       return ret;
> > +}
> > +
> > +static int __maybe_unused synquacer_spi_resume(struct device *dev)
> > +{
> > +       struct spi_master *master = dev_get_drvdata(dev);
> > +       struct synquacer_spi *sspi = spi_master_get_devdata(master);
> > +       int ret;
> > +
> > +       if (!pm_runtime_suspended(dev)) {
> > +               /* Ensure reconfigure during next xfer */
> > +               sspi->speed = 0;
> > +
> > +               if (!IS_ERR(sspi->clk)) {
>
> ... and here
>
> > +                       ret = clk_prepare_enable(sspi->clk);
> > +                       if (ret < 0) {
> > +                               dev_err(dev, "failed to enable clk (%d)\n",
> > +                                       ret);
> > +                               return ret;
> > +                       }
> > +               }
> > +
> > +               ret = synquacer_spi_enable(master);
> > +               if (ret) {
> > +                       dev_err(dev, "failed to enable spi (%d)\n", ret);
> > +                       return ret;
> > +               }
> > +       }
> > +
> > +       ret = spi_master_resume(master);
> > +       if (ret < 0) {
> > +               if (!IS_ERR(sspi->clk))
> > +                       clk_disable_unprepare(sspi->clk);
>
> .. and here
>
> > +       }
> > +
> > +       return ret;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(synquacer_spi_pm_ops, synquacer_spi_suspend,
> > +                        synquacer_spi_resume);
> > +
> > +static const struct of_device_id synquacer_spi_of_match[] = {
> > +       {.compatible = "socionext,synquacer-spi"},
> > +       {}
> > +};
> > +MODULE_DEVICE_TABLE(of, synquacer_spi_of_match);
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id synquacer_hsspi_acpi_ids[] = {
> > +       { "SCX0004" },
> > +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, synquacer_hsspi_acpi_ids);
> > +#endif
> > +
> > +static struct platform_driver synquacer_spi_driver = {
> > +       .driver = {
> > +               .name = "synquacer-spi",
> > +               .pm = &synquacer_spi_pm_ops,
> > +               .of_match_table = synquacer_spi_of_match,
> > +               .acpi_match_table = ACPI_PTR(synquacer_hsspi_acpi_ids),
> > +       },
> > +       .probe = synquacer_spi_probe,
> > +       .remove = synquacer_spi_remove,
> > +};
> > +module_platform_driver(synquacer_spi_driver);
> > +
> > +MODULE_DESCRIPTION("Socionext Synquacer HS-SPI controller driver");
> > +MODULE_AUTHOR("Masahisa Kojima <masahisa.kojima@xxxxxxxxxx>");
> > +MODULE_AUTHOR("Jassi Brar <jaswinder.singh@xxxxxxxxxx>");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.14.2
> >
>
> With the changes above,
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>
> but someone else has to review the SPI bits.