Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 23 Jul 2022 20:32:19 +0100
From:   Mark Brown <broonie@kernel.org>
To:     Zhou Yanjie <zhouyanjie@wanyeetech.com>
Cc:     tudor.ambarus@microchip.com, p.yadav@ti.com, michael@walle.cc,
        miquel.raynal@bootlin.com, richard@nod.at, vigneshr@ti.com,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org,
        linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, aidanmacdonald.0x0@gmail.com,
        tmn505@gmail.com, paul@crapouillou.net, dongsheng.qiu@ingenic.com,
        aric.pzqi@ingenic.com, rick.tyliu@ingenic.com,
        jinghui.liu@ingenic.com, sernia.zhou@foxmail.com,
        reimu@sudomaker.com
Subject: Re: [PATCH 3/3] SPI: Ingenic: Add SFC support for Ingenic SoCs.
Message-ID: <YtxMwyRghFKS/vu5@sirena.org.uk>
References: <1658508510-15400-1-git-send-email-zhouyanjie@wanyeetech.com>
 <1658508510-15400-4-git-send-email-zhouyanjie@wanyeetech.com>
 <YtrukeLk9fInqQIL@sirena.org.uk>
 <89d22457-8c62-e441-3bf4-2734ec2a45e1@wanyeetech.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="kkIhzO/ccCxeVEBm"
Content-Disposition: inline
In-Reply-To: <89d22457-8c62-e441-3bf4-2734ec2a45e1@wanyeetech.com>
Precedence: bulk


--kkIhzO/ccCxeVEBm
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Jul 24, 2022 at 01:06:16AM +0800, Zhou Yanjie wrote:
> On 2022/7/23 =E4=B8=8A=E5=8D=882: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.

> > > +static irqreturn_t ingenic_sfc_irq_handler(int irq, void *data)
> > > +{
> > > +	struct ingenic_sfc *sfc =3D 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.

--kkIhzO/ccCxeVEBm
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmLcTMIACgkQJNaLcl1U
h9BGtQgAggYv7Qn1OTgVIXH8mGUCfK3TknrnNuKFde/MvOziGFcDFwDuHzjYQZa3
00276bM4+r3UL3UZqxzUBDDUfDrjD++d40lyIBtMOiSoTdMAM6IAXYmrVkPwe/0D
4A9fXEdkuFEQAB/aaQGHhlHVFSHhJG20/vTZ5n0DFVUFnT1O134ido7AZU6RUQZ3
/9g92pmE3u8sNn0OFpGRsNoOdK1pZtrd9D0ByyY3PJy9QeM3gkGtkhdC4DISvB0U
234iv6TDKZ/sfHTPZ7lnifdxuEKFzqVlYlPAchiu0xY0sEzq0JbxeZLCkjS3s8de
1QRQhBSwCyXFQPsacWETl7fHXwO+2A==
=AxAB
-----END PGP SIGNATURE-----

--kkIhzO/ccCxeVEBm--