Re: [PATCH 5/8] imx: power-domain: Add support for the MEDIAMIX control block

[Miquel Raynal <miquel.raynal@bootlin.com>]

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 <festevam@gmail.com> wrote:

> On Tue, Sep 10, 2024 at 7:21 AM Miquel Raynal <miquel.raynal@bootlin.com> 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