On Sat, Jul 23, 2022 at 12:48:30AM +0800, 周琰杰 (Zhou Yanjie) wrote:
This looks mostly good, a few small issues though:
+++ b/drivers/spi/spi-ingenic-sfc.cPlease make the entire comment a C++ one so things look more
@@ -0,0 +1,662 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/*
+ * Ingenic SoCs SPI Flash Controller Driver
intentional.
+static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)This doesn't pay any attention to any status registers in the chip so
+{
+ struct ingenic_sfc *sfc = data;
+
+ writel(0x1f, sfc->base + SFC_REG_INTC);
+
+ complete(&sfc->completion);
+
+ return IRQ_HANDLED;
+}
won't work if the interrupt is shared and won't notice any error reports
from the device...
+static int ingenic_sfc_setup(struct spi_device *spi)The setup() operation should be safe for use on one device while another
+{
+ struct ingenic_sfc *sfc = spi_controller_get_devdata(spi->master);
+ unsigned long rate;
+ int ret, val;
+
+ if (!spi->max_speed_hz)
+ return -EINVAL;
+
+ ret = clk_set_rate(sfc->clk, spi->max_speed_hz * 2);
+ if (ret)
+ return -EINVAL;
device is active. It's not going to be a problem until there's a
version of the IP with more than one chip select, but that could happen
some time (and someone might decide to make a board using GPIO chip
selects...) but this should really go into the data path.
+ ret = clk_prepare_enable(sfc->clk);Nothing ever disables this clock. It might also be nice to enable the
+ if (ret)
+ goto err_put_master;
clock only when the controller is in use, that bit is not super
important though.
+ ret = devm_request_irq(&pdev->dev, sfc->irq, ingenic_sfc_irq_handler, 0,It's not safe to use devm here...
+ dev_name(&pdev->dev), sfc);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed to request irq%d, ret = %d\n", sfc->irq, ret);
+ goto err_put_master;
+ }
+ ret = devm_spi_register_controller(&pdev->dev, ctlr);...unregistering the controller may free the driver data structure and
+ if (ret)
+ goto err_put_master;
the interrupt handler uses it so we could attempt to use freed data in
the window between the controller being unregistered and the interrupt
being freed.