Re: [PATCH 3/3] SPI: Ingenic: Add SFC support for Ingenic SoCs.

[Zhou Yanjie]

Hi Mark,

On 2022/7/24 上午3:32, Mark Brown wrote:
> On Sun, Jul 24, 2022 at 01:06:16AM +0800, Zhou Yanjie wrote:
> > On 2022/7/23 上午2:38, Mark Brown wrote:
> > > > +++ 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?
> The above comment block uses both C /* */ and C++ // style comments,
> please make it just use the C++ style.


Sure, will do in the next version.


> > > > +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.
> Even if the device is not shared is there no possibility that the
> device can report an unexpected interrupt status?  It's not just
> the sharing case, it's also the fact that it looks like there's a
> status being reported but we're not checking it so if anything
> goes wrong then we're less likely to notice.  I'd worry about
> data corruption.


Sure, I will change this in the next version.


Thanks and best regards!