linux-kernel - Re: [PATCH v2 2/3] spi: apple: Add driver for Apple SPI controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9bf24861-f792-4315-9492-e71f7d9c457f@wanadoo.fr>
Date: Fri, 1 Nov 2024 16:59:21 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Janne Grunau via B4 Relay <devnull+j.jannau.net@...nel.org>
Cc: asahi@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-spi@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, Janne Grunau <j@...nau.net>,
 Hector Martin <marcan@...can.st>, Sven Peter <sven@...npeter.dev>,
 Alyssa Rosenzweig <alyssa@...enzweig.io>, Mark Brown <broonie@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH v2 2/3] spi: apple: Add driver for Apple SPI controller

Le 01/11/2024 à 15:25, Janne Grunau via B4 Relay a écrit :
> From: Hector Martin <marcan-WKacp4m3WJJeoWH0uzbU5w@...lic.gmane.org>
> 
> 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@...lic.gmane.org>
> Signed-off-by: Janne Grunau <j@...nau.net>

Hi,

a few nitpicks, if they make sense to you or in case of v3.


> +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 dev_err_probe(&pdev->dev, -ENOMEM, "out of memory\n");

Usually, no message is needed, because kmalloc like functions are 
already verbose.

> +
> +	spi = spi_controller_get_devdata(ctlr);
> +	init_completion(&spi->done);
> +	platform_set_drvdata(pdev, ctlr);
> +
> +	spi->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(spi->regs))
> +		return PTR_ERR(spi->regs);
> +
> +	spi->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(spi->clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(spi->clk),
> +				     "Unable to find or enable bus clock\n");
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_irq(&pdev->dev, irq, apple_spi_irq, 0,
> +			       dev_name(&pdev->dev), spi);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Unable to bind to interrupt\n");
> +
> +	ctlr->dev.of_node = pdev->dev.of_node;
> +	ctlr->bus_num = pdev->id;
> +	ctlr->num_chipselect = 1;
> +	ctlr->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST;
> +	ctlr->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> +	ctlr->flags = 0;

Nitpick: not needed, the memory if already zeroed.

> +	ctlr->prepare_message = apple_spi_prepare_message;
> +	ctlr->set_cs = apple_spi_set_cs;
> +	ctlr->transfer_one = apple_spi_transfer_one;
> +	ctlr->auto_runtime_pm = true;
> +
> +	pm_runtime_set_active(&pdev->dev);
> +	devm_pm_runtime_enable(&pdev->dev);

There could be an error handling, or if intentional, maybe a (void) or a 
comment to show the intent.

> +
> +	apple_spi_init(spi);
> +
> +	ret = devm_spi_register_controller(&pdev->dev, ctlr);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "devm_spi_register_controller failed\n");
> +
> +	return 0;
> +}

...

CJ