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
