Hi Mark,
On 2022/7/23 上午2:38, Mark Brown wrote:
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.c
@@ -0,0 +1,662 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/*
+ * Ingenic SoCs SPI Flash Controller Driver
Please make the entire comment a C++ one so things look more
intentional.
I'm sorry, I didn't understand well what you meant :(
Could you please explain a little more detail?
+static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
+{
+ struct ingenic_sfc *sfc = data;
+
+ writel(0x1f, sfc->base + SFC_REG_INTC);
+
+ complete(&sfc->completion);
+
+ return IRQ_HANDLED;
+}
This doesn't pay any attention to any status registers in the chip so
won't work if the interrupt is shared and won't notice any error reports
from the device...
This interrupt is exclusively owned by SFC, do we still
need to perform the operation you said? I haven't done
these operations before because I want to minimize the
overhead and avoid affecting performance.
+static int ingenic_sfc_setup(struct spi_device *spi)
+{
+ 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;
The setup() operation should be safe for use on one device while another
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.
Sure, I will change it in the next version.
+ ret = clk_prepare_enable(sfc->clk);
+ if (ret)
+ goto err_put_master;
Nothing ever disables this clock. It might also be nice to enable the
clock only when the controller is in use, that bit is not super
important though.
Sure, will add it.
+ ret = devm_request_irq(&pdev->dev, sfc->irq, ingenic_sfc_irq_handler, 0,
+ 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;
+ }
It's not safe to use devm here...
Sure, will fix it in the next version.
+ ret = devm_spi_register_controller(&pdev->dev, ctlr);
+ if (ret)
+ goto err_put_master;
...unregistering the controller may free the driver data structure and
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.
Sure.
Thanks and best regards!
