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

[Hector Martin <marcan@marcan.st>]

Thanks for the review!

On 12/12/2021 21.39, Sven Peter wrote:
>> +
> 
> #include <linux/bits.h> for GENMASK even though it's probably
> pulled in by one of the #includes below

Ack, fixed for v2.

>> +	/* We will want to poll if the time we need to wait is
>> +	 * less than the context switching time.
>> +	 * Let's call that threshold 5us. The operation will take:
>> +	 *    bits_per_word * fifo_threshold / hz <= 5 * 10^-6
>> +	 *    200000 * bits_per_word * fifo_threshold <= hz
>> +	 */
>> +	return 200000 * t->bits_per_word * APPLE_SPI_FIFO_DEPTH / 2 <=
>> t->speed_hz;
> 
> Nice :-)

I stole this one from the sifive driver :-) (slightly adjusted)

>> +static int apple_spi_probe(struct platform_device *pdev)
>> +{
>> +	struct apple_spi *spi;
>> +	int ret, irq;
>> +	struct spi_controller *ctlr;
>> +
>> +	ctlr = spi_alloc_master(&pdev->dev, sizeof(struct apple_spi));
> 
> devm_spi_alloc_master and then you can get rid of the spi_controller_put
> error path.

Ack, fixed for v2. That simplifies a bunch of the error handling.

> 
>> +	if (!ctlr) {
>> +		dev_err(&pdev->dev, "out of memory\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	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)) {
>> +		ret = PTR_ERR(spi->regs);
>> +		goto put_ctlr;
>> +	}
>> +
>> +	spi->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(spi->clk)) {
>> +		dev_err(&pdev->dev, "Unable to find bus clock\n");
>> +		ret = PTR_ERR(spi->clk);
>> +		goto put_ctlr;
>> +	}
> 
> dev_err_probe can be used here in case devm_clk_get returns -EPROBE_DEFER.

Yup, good point. I switched most of the dev_errs to dev_err_probe.

> 
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0) {
>> +		ret = irq;
>> +		goto put_ctlr;
>> +	}
>> +
>> +	ret = devm_request_irq(&pdev->dev, irq, apple_spi_irq, 0,
>> +			       dev_name(&pdev->dev), spi);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Unable to bind to interrupt\n");
>> +		goto put_ctlr;
>> +	}
>> +
>> +	ret = clk_prepare_enable(spi->clk);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Unable to enable bus clock\n");
>> +		goto put_ctlr;
>> +	}
> 
> Unfortunately there's no devm_clk_prepare_enable but you could use
> devm_add_action_or_reset like almost all watchdog drivers as well.

Done.

>> +
>> +	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;
>> +	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);
>> +	pm_runtime_enable(&pdev->dev);
> 
> You could also use devm_pm_runtime_enable here and then everything
> should be devres managed.

Done, though I still need to wrap the remove remove function in 
pm_runtime calls, since the device might be suspended when it gets called.

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