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
