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
