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.
> +#define APPLE_SPI_DRIVER_NAME "apple_spi"
This is referenced exactly once, just inline it.
> + /* 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.
> + 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.
> +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.
> +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?
Attachment:
signature.asc
Description: PGP signature
