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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for the review!

On 13/12/2021 08.41, Mark Brown wrote:
On Sun, Dec 12, 2021 at 12:47:26PM +0900, Hector Martin wrote:

This looks pretty good - one small issue and several stylistic nits
below.

@@ -0,0 +1,566 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Apple SoC SPI device driver
+ *

Please keep the entire comment a C++ one so things look more
intentional.

I thought this pattern was pretty much the standard style.

$ grep -r -A1 "// SPDX" | grep '/\*' | wc -l
13512

$ grep -r -A1 "// SPDX" | grep -v SPDX | grep '//' | wc -l
705

+#define APPLE_SPI_DRIVER_NAME           "apple_spi"

This is referenced exactly once, just inline it.
Done. Also changed to "apple-spi" since all the other apple drivers use the dash convention.


+	/* 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;

Some brackets or an intermediate variable wouldn't hurt here, especially
given the line length.

How about this?

return (200000 * t->bits_per_word * APPLE_SPI_FIFO_DEPTH / 2) <= t->speed_hz;

+	struct apple_spi *spi = spi_controller_get_devdata(ctlr);
+	bool poll = apple_spi_prep_transfer(spi, t);
+	const void *tx_ptr = t->tx_buf;
+	void *rx_ptr = t->rx_buf;
+	unsigned int bytes_per_word = t->bits_per_word > 16 ? 4 : t->bits_per_word > 8 ? 2 : 1;

Please don't abuse the ternery operator like this - just write normal
conditional statements, they're much easier to read.  In general the
driver is a bit too enthusiastic about them and this one is next level.

Ack, I switched it to an if chain. That does mean I had to move the subsequent initializers out of the declarations section, so it's overall a bit more verbose.

	if (t->bits_per_word > 16)
		bytes_per_word = 4;
	else if (t->bits_per_word > 8)
		bytes_per_word = 2;
	else
		bytes_per_word = 1;

	words = t->len / bytes_per_word;
	remaining_tx = tx_ptr ? words : 0;
	remaining_rx = rx_ptr ? words : 0;

+static int apple_spi_remove(struct platform_device *pdev)
+{
+	struct spi_controller *ctlr = platform_get_drvdata(pdev);
+	struct apple_spi *spi = spi_controller_get_devdata(ctlr);
+
+	pm_runtime_disable(&pdev->dev);
+
+	/* Disable all the interrupts just in case */
+	reg_write(spi, APPLE_SPI_IE_FIFO, 0);
+	reg_write(spi, APPLE_SPI_IE_XFER, 0);

Since the driver registers with the SPI subsystem using devm and
remove() gets run before devm gets unwound we still potentially have
transfers running when the driver gets removed and this probably isn't
going to cause them to go well - obviously it's an edge case and it's
unclear when someone would be removing the driver but we still shouldn't
do this.

With the other devm changes Sven suggested we don't need a remove function at all, so I've just gotten rid of it wholesale.

+static const struct of_device_id apple_spi_of_match[] = {
+	{ .compatible = "apple,spi", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, apple_spi_of_match);

This is an awfully generic compatible.  It's common to use the SoC name
for the IP compatibles when they're not otherwise identified?

Apple like to keep blocks compatible across SoC generations - I think this one dates, at least to some extent, to the original iPhone or thereabouts. We do use per-SoC compatibles in the DTs in case we need to throw in per-SoC quirks in the future ("apple,t8103-spi", "apple,spi"), but for drivers like this we prefer to use generic compatibles as long as backwards compatibility doesn't break. If Apple do something totally incompatible (like they did with AIC2 in the latest chips), we'll bump to something like apple,spi2. This happens quite rarely, so it makes sense to just keep things generic except for these instances. That way old kernels will happily bind to the block in newer SoCs if it is compatible.

If we had a detailed lineage of what SoCs used what blocks and when things changed we could try something else, like using the first SoC where the specific block version was introduced, but we don't... so I think it makes sense to just go with generic ones where we don't think things have changed much since the dark ages. FWIW, Apple calls this one spi-1,spimc and claims "spi-version = 1" and the driver has Samsung in the name... so the history of this block definitely goes back quite a ways :-)

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



[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux