Messages in this thread | | | From | Sagar Dharia <> | Subject | Re: [PATCH V4 4/6] slim: qcom: Add Qualcomm Slimbus controller driver | Date | Fri, 15 Apr 2016 10:17:36 -0600 |
| |
Hello Mark Apologies for a late reply. I will incorporate most of your comments. Please see inline response for 2 comments: >> + ret = devm_request_irq(&pdev->dev, dev->irq, msm_slim_interrupt, >> + IRQF_TRIGGER_HIGH, "msm_slim_irq", dev); >> + if (ret) { >> + dev_err(&pdev->dev, "request IRQ failed\n"); >> + goto err_request_irq_failed; >> + } > Are you sure this is safe and we don't deallocate things the interrupt > handler uses before we disable the interrupt?
Since clock is not enabled before this step, we won't be getting any interrupts from HW at this stage.
>> + /* Register with framework before enabling frame, clock */ >> + ret = slim_register_controller(&dev->ctrl); >> + if (ret) { >> + dev_err(dev->dev, "error adding controller\n"); >> + goto err_ctrl_failed; >> + } > Should we have a devm_ version of slim_register_controller()? I'd also > expect this to be the last thing we do in probe, things may start using > the device before we've finished initializing it. register_controller also allocates controller's TX/RX ring buffers. These rings are needed when devices start reporting present on the bus once they are enabled. So register_controller needs to be done before enabling any devices. All steps after register_controller in this function are related to enabling various internal component devices (e.g. framer, interface, manager devices) of this slimbus controller.
Thank you for your comments Sagar
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
| |