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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <283bdfef-cbd9-5d87-f77d-10f007c37a0c@microchip.com>
Date:   Mon, 1 Aug 2022 10:40:33 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <Nagasuresh.Relli@...rochip.com>, <broonie@...nel.org>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>
CC:     <linux-spi@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] spi: microchip-core-qspi: Add support for microchip
 fpga qspi controllers

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@...rochip.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.