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

[Janne Grunau <j@xxxxxxxxxx>]

On Mon, Nov 04, 2024 at 08:16:25PM +0100, Christophe JAILLET wrote:
> Le 01/11/2024 à 20:26, Janne Grunau via B4 Relay a écrit :
> > From: Hector Martin <marcan-WKacp4m3WJJeoWH0uzbU5w@xxxxxxxxxxxxxxxx>
> > 
> > This SPI controller is present in Apple SoCs such as the M1 (t8103) and
> > M1 Pro/Max (t600x). It is a relatively straightforward design with two
> > 16-entry FIFOs, arbitrary transfer sizes (up to 2**32 - 1) and fully
> > configurable word size up to 32 bits. It supports one hardware CS line
> > which can also be driven via the pinctrl/GPIO driver instead, if
> > desired. TX and RX can be independently enabled.
> > 
> > There are a surprising number of knobs for tweaking details of the
> > transfer, most of which we do not use right now. Hardware CS control
> > is available, but we haven't found a way to make it stay low across
> > multiple logical transfers, so we just use software CS control for now.
> > 
> > There is also a shared DMA offload coprocessor that can be used to handle
> > larger transfers without requiring an IRQ every 8-16 words, but that
> > feature depends on a bunch of scaffolding that isn't ready to be
> > upstreamed yet, so leave it for later.
> > 
> > The hardware shares some register bit definitions with spi-s3c24xx which
> > suggests it has a shared legacy with Samsung SoCs, but it is too
> > different to warrant sharing a driver.
> > 
> > Signed-off-by: Hector Martin <marcan-WKacp4m3WJJeoWH0uzbU5w@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Janne Grunau <j@xxxxxxxxxx>
> > ---
> 
> Hi,
> 
> > diff --git a/drivers/spi/spi-apple.c b/drivers/spi/spi-apple.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..1a3f61501db56d0d7689cc3d6f987bf636130cdb
> > --- /dev/null
> > +++ b/drivers/spi/spi-apple.c
> > @@ -0,0 +1,531 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Apple SoC SPI device driver
> > +//
> > +// Copyright The Asahi Linux Contributors
> > +//
> > +// Based on spi-sifive.c, Copyright 2018 SiFive, Inc.
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/clk.h>
> > +#include <linux/module.h>
> 
> Move a few lines below to keep alphabetical order?

done

> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/spi/spi.h>
> 
> ...
> 
> > +static int apple_spi_probe(struct platform_device *pdev)
> > +{
> > +	struct apple_spi *spi;
> > +	int ret, irq;
> > +	struct spi_controller *ctlr;
> > +
> > +	ctlr = devm_spi_alloc_master(&pdev->dev, sizeof(struct apple_spi));
> > +	if (!ctlr)
> > +		return -ENOMEM;
> > +
> > +	spi = spi_controller_get_devdata(ctlr);
> > +	init_completion(&spi->done);
> > +	platform_set_drvdata(pdev, ctlr);
> 
> Is it needed?
> There is no platform_get_drvdata()

no, it is not used anymore. It's a leftover from v1 which used
platform_get_drvdata() in apple_spi_remove() which is gone now.

thanks, I'll send a v4 shortly
Janne