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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 28 May 2019 at 11:27, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> This patch adds support for controller found on synquacer platforms.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
>  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?

> +       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@linaro.org>");
> +MODULE_AUTHOR("Jassi Brar <jaswinder.singh@linaro.org>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.14.2
>

With the changes above,

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

but someone else has to review the SPI bits.