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

On Mon, Dec 13, 2021 at 12:50:49PM +0900, Hector Martin wrote:

On 13/12/2021 08.41, Mark Brown wrote:

quoted

On Sun, Dec 12, 2021 at 12:47:26PM +0900, Hector Martin wrote:

quoted

quoted

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

quoted

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

I thought this pattern was pretty much the standard style.

It's common, especially given all the automated conversions, but ugly.

quoted

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;

quoted

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;

That's better but it's still a very long line which is half the issue.

quoted

quoted

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

quoted

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.

There's currently a bit of a fashion for people with very old SPI blocks
to make incompatible new versions recently, a lot of it seems to be
driven by things like flash engine support.  Sometimes these things end
up getting instantiated together as they have different purposes and the
incompatibilties make the IPs larger.

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 :-)

Have you done a contrast and compare with the Samsung driver?  Given
both this and your comments above about this dating back to the original
iPhone...