Hi Heiko,
On 08/10/2017 08:22 PM, Heiko Stuebner wrote:
>>
>> >+ devm_add_action_or_reset(dev,
>> >+ rockchip_pcie_disable_clocks, rockchip);
>> >+
> err = devm_add_action_or_reset(...)
> if (err) {
> ...
> }
>
> devm_add_action_or_reset can fail. When it fails, it will call the
> action already, so your error handling does not need to disable
> clocks on its own.
>
>
> Also, as a more general comment, right now you do devm_request_irq
> from rockchip_pcie_parse_dt, which gets called_before_ clocks are
> enabled.
>
> This will likely bite you at some point as well, as the irq can fire at
> any point after it got requested ... including before you enable the
> clocks.
>
> So the order should probably be
>
> - enable clocks
> - register devm_action to shutdown clocks
> - parse_dt including requesting the irq.
>
>
> Heiko
it turns out this irq handler not only depends on clks, but also pm
domain :(
so handling it with devm_* functions would be a little
complicated(https://lkml.org/lkml/2017/8/15/146).
would it make sense to use request_irq/free_irq directly?
or maybe add a flag(for example probed), and check it in the irq handler
before read registers? then we would just need to make sure the priv
struct be freed later than the irq.
>
>
