Hi Fabio, First, sorry for the delay and thanks a lot for the review.
On 12/09/2024 at 09:40:32 -03, Fabio Estevam <[email protected]> wrote: > On Tue, Sep 10, 2024 at 7:21 AM Miquel Raynal <[email protected]> > wrote: > >> + /* Make sure bus domain is awake */ >> + ret = power_domain_on(&priv->pd_bus); >> + if (ret) >> + return ret; >> + >> + /* Put devices into reset */ >> + clrbits_le32(priv->base + BLK_SFT_RSTN, reset); >> + >> + /* Enable upstream clocks */ >> + ret = clk_enable(&priv->clk_apb); >> + if (ret) >> + goto dis_bus_pd; >> + >> + ret = clk_enable(&priv->clk_axi); >> + if (ret) >> + goto dis_bus_pd; >> + >> + /* Enable blk-ctrl clock to allow reset to propagate */ >> + ret = clk_enable(clk); >> + if (ret) >> + goto dis_bus_pd; >> + setbits_le32(priv->base + BLK_CLK_EN, reset); >> + >> + /* Power up upstream GPC domain */ >> + ret = power_domain_on(domain); >> + if (ret) >> + goto dis_bus_pd; > > The previously enabled clocks should be disabled on the error paths. That's right, I will. > Could you use clk_enable_bulk()? This not very convenient and not future-proof, because, while we just want to grab and enable the apb and axi clocks, the media_disp1_pix and media_disp2_pix will be used depending on the configuration. For not I've not included support for media_disp1_pix because I do not use it and cannot test it, but it is a legitimate addition that someone will soon or later want to bring. Hence I believe using the _bulk() helper is not really appropriate in this case. Thanks! Miquèl
