Messages in this thread | | | Subject | Re: [PATCH 3/3] SPI: Ingenic: Add SFC support for Ingenic SoCs. | From | Zhou Yanjie <> | Date | Sun, 24 Jul 2022 01:06:16 +0800 |
| |
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!
| |