On Thu, May 30, 2019 at 11:24:04PM +0300, Sergei Shtylyov wrote:
> +config SPI_RPCIF
> + tristate "Renesas RPC-IF SPI driver"
> + select MFD_RPCIF
> + depends on ARCH_RENESAS || COMPILE_TEST
> + help
> + SPI driver for Renesas R-Car Gen3 RPC-IF.
> +
Why are we selecting the MFD rather than depending on it like normal?
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RPC-IF SPI/QSPI/Octa driver
> + *
Please make the entire header a C++ one so this looks more intentional.
> +static void rpcif_spi_transfer_setup(struct rpcif_spi *spi,
> + struct spi_message *msg)
> +{
> + struct spi_transfer *t, xfer[4] = { };
Don't mix initialized and non-initialized declarations in a single line
(as per coding style).
> + u32 i, xfercnt, xferpos = 0;
> + struct rpcif_op op = { };
> +
> + spi->totalxferlen = 0;
> + spi->xfer_dir = SPI_MEM_NO_DATA;
> +
> + list_for_each_entry(t, &msg->transfers, transfer_list) {
> + if (t->tx_buf) {
> + xfer[xferpos].tx_buf = t->tx_buf;
> + xfer[xferpos].tx_nbits = t->tx_nbits;
> + }
xfer is hard coded to 4 elements but I'm not seeing any validation that
we don't have more transfers than that in the message, and there's lots
of assumptions later on about the number of transfers.
> + if (list_is_last(&t->transfer_list, &msg->transfers)) {
> + if (xferpos > 1) {
> + if (t->rx_buf)
> + spi->xfer_dir = SPI_MEM_DATA_IN;
> + else if (t->tx_buf)
> + spi->xfer_dir = SPI_MEM_DATA_OUT;
> + }
> + }
Transfers can be bidirectional... if the device can't support that it
should set SPI_CONTROLLER_HALF_DUPLEX.
> +static inline int rpcif_spi_xfer_message(struct rpcif_spi *spi,
> + struct spi_transfer *data_xfer)
This has exactly one caller and contains a single statement - why have a
separate function?
> +{
> + return rpcif_io_xfer(spi->rpc,
> + spi->xfer_dir == SPI_MEM_DATA_OUT ?
> + data_xfer->tx_buf : NULL,
> + spi->xfer_dir == SPI_MEM_DATA_IN ?
> + data_xfer->rx_buf : NULL);
This is really hard to read. Why are we abusing the ternery operator
here, especially when there's other places where we already set things
up based on the direction?
> +static int rpcif_spi_remove(struct platform_device *pdev)
> +{
> + struct spi_controller *ctlr = platform_get_drvdata(pdev);
> + struct rpcif *rpc = dev_get_drvdata(pdev->dev.parent);
> +
> + rpcif_disable_rpm(rpc);
> + spi_unregister_controller(ctlr);
> +
> + return 0;
> +}
Shouldn't we unregister the controller before we disable the RPM? The
probe was the other way around and this means that we might still be
processing messages while the hardware is disabled which doesn't seem
good.
Attachment:
signature.asc
Description: PGP signature
