Re: [PATCH 2/2] spi: microchip-core-qspi: Add support for microchip fpga qspi controllers

[<Conor.Dooley@microchip.com>]

On 01/08/2022 10:42, Naga Sureshkumar Relli wrote:
> Add a driver for Microchip FPGA QSPI controllers. This driver also
> supports "hard" QSPI controllers on Polarfire SoC.
> 
> Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com>
> ---

---8<---

> +static int mchp_coreqspi_probe(struct platform_device *pdev)
> +{
> +	struct spi_controller *ctlr;
> +	struct mchp_coreqspi *qspi;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	ctlr = spi_alloc_master(&pdev->dev, sizeof(*qspi));
> +	if (!ctlr)
> +		return -ENOMEM;

Argh... I am sorry for not mentioning this when you asked me if
I thought the driver was ready to be sent upstream, but I think
we should try to use the devm_ versions of these functions.

> +
> +	qspi = spi_controller_get_devdata(ctlr);

Is there a reason to use spi_controller_get_devdata() rather than
spi_master_get_devdata() ?

> +	platform_set_drvdata(pdev, qspi);
> +
> +	qspi->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(qspi->regs)) {
> +		ret = PTR_ERR(qspi->regs);
> +		goto remove_master;

Using devm_spi_alloc_master() above would simplify this
to just a return of dev_err_probe() & ditto for every
time we do a "goto remove_master"

> +	}
> +
> +	qspi->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(qspi->clk)) {
> +		dev_err(&pdev->dev, "clock not found.\n");
> +		ret = PTR_ERR(qspi->clk);
> +		goto remove_master;
> +	}
> +
> +	ret = clk_prepare_enable(qspi->clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable clock\n");
> +		goto remove_master;
> +	}
> +
> +	init_completion(&qspi->data_completion);
> +	mutex_init(&qspi->op_lock);
> +
> +	qspi->irq = platform_get_irq(pdev, 0);
> +	if (qspi->irq <= 0) {
> +		ret = qspi->irq;
> +		goto clk_dis_all;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, qspi->irq, mchp_coreqspi_isr,
> +			       0, pdev->name, qspi);
> +	if (ret != 0) {
> +		dev_err(&pdev->dev, "request_irq failed %d\n", ret);
> +		goto clk_dis_all;
> +	}
> +
> +	ctlr->bits_per_word_mask = SPI_BPW_MASK(8);
> +	ctlr->mem_ops = &mchp_coreqspi_mem_ops;
> +	ctlr->setup = mchp_coreqspi_setup_op;
> +	ctlr->mode_bits = SPI_CPOL | SPI_CPHA | SPI_RX_DUAL | SPI_RX_QUAD |
> +			  SPI_TX_DUAL | SPI_TX_QUAD;
> +	ctlr->dev.of_node = np;
> +
> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> +	if (ret) {
> +		dev_err(&pdev->dev, "spi_register_controller failed\n");
> +		goto clk_dis_all;
> +	}
> +
> +	return 0;
> +
> +clk_dis_all:
> +	clk_disable_unprepare(qspi->clk);

If we move the clk prepare & enable later in probe, ie after
getting the irq, this goto could be removed too because the
only place requiring teardown of the clock would be the error
path of devm_spi_register_controller().

> +remove_master:
> +	spi_controller_put(ctlr);

With devm_spi_alloc_master() this put is no longer needed &
we can return a nice dev_err_probe() for each error :)

> +
> +	return ret;
> +}

Thanks Suresh.
Conor.