lkml.org 
[lkml]   [2021]   [Dec]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 3/3] spi: apple: Add driver for Apple SPI controller
Date
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

\
 
 \ /
  Last update: 2021-12-13 04:34    [W:0.205 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site