Re: [PATCH 3/3] spi: apple: Add driver for Apple SPI controller

[Hector Martin]

On 14/12/2021 00.56, Mark Brown wrote:
> On Mon, Dec 13, 2021 at 12:50:49PM +0900, Hector Martin wrote:
> > On 13/12/2021 08.41, Mark Brown wrote:
> > > On Sun, Dec 12, 2021 at 12:47:26PM +0900, Hector Martin wrote:
>
> > > > @@ -0,0 +1,566 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Apple SoC SPI device driver
> > > > + *
>
> > > Please keep the entire comment a C++ one so things look more
> > > intentional.
>
> > I thought this pattern was pretty much the standard style.
>
> It's common, especially given all the automated conversions, but ugly.

Sure, I'll change it if you insist :)

> > > Some brackets or an intermediate variable wouldn't hurt here, especially
> > > given the line length.
>
> > How about this?
>
> > return (200000 * t->bits_per_word * APPLE_SPI_FIFO_DEPTH / 2) <= t->speed_hz;
>
> That's better but it's still a very long line which is half the issue.

I think it's quite readable at this point (especially with the comment 
above explaining it anyway). Note that these days a lot of people 
consider lines up to 100 chars okay in the kernel, and checkpatch uses 
that limit. Do you have a specific change in mind?

> > > > +static const struct of_device_id apple_spi_of_match[] = {
> > > > +	{ .compatible = "apple,spi", },
> > > > +	{}
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, apple_spi_of_match);
>
> > > This is an awfully generic compatible.  It's common to use the SoC name
> > > for the IP compatibles when they're not otherwise identified?
>
> > Apple like to keep blocks compatible across SoC generations - I think this
> > one dates, at least to some extent, to the original iPhone or thereabouts.
> > We do use per-SoC compatibles in the DTs in case we need to throw in per-SoC
> > quirks in the future ("apple,t8103-spi", "apple,spi"), but for drivers like
> > this we prefer to use generic compatibles as long as backwards compatibility
> > doesn't break. If Apple do something totally incompatible (like they did
> > with AIC2 in the latest chips), we'll bump to something like apple,spi2.
> > This happens quite rarely, so it makes sense to just keep things generic
> > except for these instances. That way old kernels will happily bind to the
> > block in newer SoCs if it is compatible.
>
> There's currently a bit of a fashion for people with very old SPI blocks
> to make incompatible new versions recently, a lot of it seems to be
> driven by things like flash engine support.  Sometimes these things end
> up getting instantiated together as they have different purposes and the
> incompatibilties make the IPs larger.

I think if they haven't changed it by now they probably won't; e.g. they 
tacked on DMA using a coprocessor instead of changing the block itself. 
I don't think Apple uses SPI for anything performance-critical. They 
don't even bother with QSPI for the NOR flash (which is mostly only used 
for bootloaders and variable storage).

> > If we had a detailed lineage of what SoCs used what blocks and when things
> > changed we could try something else, like using the first SoC where the
> > specific block version was introduced, but we don't... so I think it makes
> > sense to just go with generic ones where we don't think things have changed
> > much since the dark ages. FWIW, Apple calls this one spi-1,spimc and claims
> > "spi-version = 1" and the driver has Samsung in the name... so the history
> > of this block definitely goes back quite a ways :-)
>
> Have you done a contrast and compare with the Samsung driver?  Given
> both this and your comments above about this dating back to the original
> iPhone...

You mean the *two* Samsung drivers? :-)

It seems Samsung like to keep making up incompatible SPI blocks. This 
one shares a *few* bits in a *couple* registers with spi-s3c24xx driver, 
which point to a common lineage, but those registers aren't even at the 
same addresses. Not enough in common for it to make sense to try to use 
one driver for both (unlike with UART, where it was close enough to be 
added as a new Samsung UART variant, or I2C, where we could refactor the 
pasemi driver to add a platform backend alongside the existing PCI 
support and mostly use it as-is).

--
Hector Martin (marcan@xxxxxxxxx)
Public Key: https://mrcn.st/pub