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

[Hector Martin]

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@xxxxxxxxx)
Public Key: https://mrcn.st/pub