Re: [PATCH 3/3] spi: apple: Add driver for Apple SPI controller

Thanks for the review!

On 12/12/2021 21.39, Sven Peter wrote:

quoted

+

#include <linux/bits.h> for GENMASK even though it's probably
pulled in by one of the #includes below

Ack, fixed for v2.

quoted

+	/* We will want to poll if the time we need to wait is
+	 * less than the context switching time.
+	 * Let's call that threshold 5us. The operation will take:
+	 *    bits_per_word * fifo_threshold / hz <= 5 * 10^-6
+	 *    200000 * bits_per_word * fifo_threshold <= hz
+	 */
+	return 200000 * t->bits_per_word * APPLE_SPI_FIFO_DEPTH / 2 <=
t->speed_hz;

Nice :-)

I stole this one from the sifive driver :-) (slightly adjusted)

quoted

+static int apple_spi_probe(struct platform_device *pdev)
+{
+	struct apple_spi *spi;
+	int ret, irq;
+	struct spi_controller *ctlr;
+
+	ctlr = spi_alloc_master(&pdev->dev, sizeof(struct apple_spi));

devm_spi_alloc_master and then you can get rid of the spi_controller_put
error path.

Ack, fixed for v2. That simplifies a bunch of the error handling.

quoted

+	if (!ctlr) {
+		dev_err(&pdev->dev, "out of memory\n");
+		return -ENOMEM;
+	}
+
+	spi = spi_controller_get_devdata(ctlr);
+	init_completion(&spi->done);
+	platform_set_drvdata(pdev, ctlr);
+
+	spi->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(spi->regs)) {
+		ret = PTR_ERR(spi->regs);
+		goto put_ctlr;
+	}
+
+	spi->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(spi->clk)) {
+		dev_err(&pdev->dev, "Unable to find bus clock\n");
+		ret = PTR_ERR(spi->clk);
+		goto put_ctlr;
+	}

dev_err_probe can be used here in case devm_clk_get returns -EPROBE_DEFER.

Yup, good point. I switched most of the dev_errs to dev_err_probe.

quoted

+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		ret = irq;
+		goto put_ctlr;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, apple_spi_irq, 0,
+			       dev_name(&pdev->dev), spi);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to bind to interrupt\n");
+		goto put_ctlr;
+	}
+
+	ret = clk_prepare_enable(spi->clk);
+	if (ret) {
+		dev_err(&pdev->dev, "Unable to enable bus clock\n");
+		goto put_ctlr;
+	}

Unfortunately there's no devm_clk_prepare_enable but you could use
devm_add_action_or_reset like almost all watchdog drivers as well.

Done.

quoted

+
+	ctlr->dev.of_node = pdev->dev.of_node;
+	ctlr->bus_num = pdev->id;
+	ctlr->num_chipselect = 1;
+	ctlr->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST;
+	ctlr->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
+	ctlr->flags = 0;
+	ctlr->prepare_message = apple_spi_prepare_message;
+	ctlr->set_cs = apple_spi_set_cs;
+	ctlr->transfer_one = apple_spi_transfer_one;
+	ctlr->auto_runtime_pm = true;
+
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);

You could also use devm_pm_runtime_enable here and then everything
should be devres managed.

Done, though I still need to wrap the remove remove function in 
pm_runtime calls, since the device might be suspended when it gets called.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub