Re: [PATCH V4 4/6] slim: qcom: Add Qualcomm Slimbus controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux