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

On Tue, Dec 14, 2021 at 02:10:26AM +0900, Hector Martin wrote:

On 14/12/2021 00.56, Mark Brown wrote:

quoted

On Mon, Dec 13, 2021 at 12:50:49PM +0900, Hector Martin wrote:

quoted

quoted

quoted

Some brackets or an intermediate variable wouldn't hurt here, especially
given the line length.

quoted

quoted

How about this?

quoted

quoted

return (200000 * t->bits_per_word * APPLE_SPI_FIFO_DEPTH / 2) <= t->speed_hz;

quoted

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?

The 100 characters is a "don't send silly checkpatch fixes" thing not a
target to aim for (see also the ternery operator stuff).  Like I said an
intermediate variable wouldn't hurt, for example for the FIFO trigger
level into a fifo_trigger variable.

quoted

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).

This feels like tempting fate but I guess...

quoted

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).

Their older SPI block has quite a few issues IIRC, I think DMA was the
big difference between the two but ICBW.