Hi Bjorn, On Wed, 27 Aug 2025 at 00:32, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Tue, Aug 26, 2025 at 11:32:34PM +0530, Anand Moon wrote: > > Hi Bjorn, > > > > Thanks for your review comments. > > On Tue, 26 Aug 2025 at 21:55, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > > > > > In subject, remove "dwc: " to follow historical convention. (See > > > "git log --oneline") > > > > > Ok I will keep it as per the git history. > > > > > On Tue, Aug 26, 2025 at 05:12:40PM +0530, Anand Moon wrote: > > > > Currently, the driver acquires clocks and prepare/enable/disable/unprepare > > > > the clocks individually thereby making the driver complex to read. > > > > > > > > The driver can be simplified by using the clk_bulk*() APIs. > > > > > > > > Use: > > > > - devm_clk_bulk_get_all() API to acquire all the clocks > > > > - clk_bulk_prepare_enable() to prepare/enable clocks > > > > - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk > > > > > > I assume this means the order in which we prepare/enable and > > > disable/unprepare will now depend on the order the clocks appear in > > > the device tree instead of the order in the code? If so, please > > > mention that here and verify that all upstream device trees have the > > > correct order. > > > > > Following is the order in the device tree > > > > clocks = <&crg HISTB_PCIE_AUX_CLK>, > > <&crg HISTB_PCIE_PIPE_CLK>, > > <&crg HISTB_PCIE_SYS_CLK>, > > <&crg HISTB_PCIE_BUS_CLK>; > > clock-names = "aux", "pipe", "sys", "bus"; > > The current order in the code is: > > histb_pcie_host_enable > clk_prepare_enable(hipcie->bus_clk); > clk_prepare_enable(hipcie->sys_clk); > clk_prepare_enable(hipcie->pipe_clk); > clk_prepare_enable(hipcie->aux_clk); > > histb_pcie_host_disable > clk_disable_unprepare(hipcie->aux_clk); > clk_disable_unprepare(hipcie->pipe_clk); > clk_disable_unprepare(hipcie->sys_clk); > clk_disable_unprepare(hipcie->bus_clk); > > After this patch, it looks like we'll have: > > histb_pcie_host_enable > clk_bulk_prepare_enable > aux > pipe > sys > bus > > histb_pcie_host_disable > clk_bulk_disable_unprepare > bus > sys > pipe > aux > > So it looks like this patch will reverse the ordering both for > enabling and disabling, right? I'm totally fine with this as long as > it works. > Thank you for the input. Let's wait for additional feedback regarding the changes. > Bjorn Thanks -Anand
