Re: [PATCH] drm/rockchip: Cleanup dangling devm pointers

[Daniel Kurtz <djkurtz@chromium.org>]

On Wed, Sep 21, 2016 at 3:36 PM, Sean Paul <seanpaul@chromium.org> wrote:
> On Mon, Sep 19, 2016 at 7:14 AM, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> Hi Sean,
>>
>> On Sat, Sep 17, 2016 at 2:22 AM, Sean Paul <seanpaul@chromium.org> wrote:
>>>
>>> Instead of assigning device managed resources to local variables,
>>> keep track of them in the vop struct.
>>
>> Why this patch?
>> Is it fixing an issue?
>> Or, is it preparing for some future use of ahb_rst outside of vop_initial?
>>
>
> Nah, this is just me being pedantic.
>
>> I thought that one of the nice features of using devm is you do not
>> need to carry around pointers to devm allocated resources in the
>> driver local device struct.
>>
>
> True, but it feels a bit weird to allocate something on driver load
> and intentionally abandon it for the lifetime of the driver. I'd feel
> better either keeping it around, or perhaps it should be put once
> we're done using it in vop_initial.

Yeah... if we don't ever use it again, I agree - no reason to devm it,
just use reset_control_get / _put here in vop_initial.

>
> Sean
>
>
>> -Dan
>>
>>>
>>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>>> ---
>>>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 12 ++++++------
>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> index 131ae0f..bed782e 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> @@ -142,6 +142,7 @@ struct vop {
>>>
>>>         /* vop dclk reset */
>>>         struct reset_control *dclk_rst;
>>> +       struct reset_control *ahb_rst;
>>>
>>>         struct vop_win win[];
>>>  };
>>> @@ -1333,7 +1334,6 @@ static int vop_initial(struct vop *vop)
>>>  {
>>>         const struct vop_data *vop_data = vop->data;
>>>         const struct vop_reg_data *init_table = vop_data->init_table;
>>> -       struct reset_control *ahb_rst;
>>>         int i, ret;
>>>
>>>         vop->hclk = devm_clk_get(vop->dev, "hclk_vop");
>>> @@ -1374,15 +1374,15 @@ static int vop_initial(struct vop *vop)
>>>         /*
>>>          * do hclk_reset, reset all vop registers.
>>>          */
>>> -       ahb_rst = devm_reset_control_get(vop->dev, "ahb");
>>> -       if (IS_ERR(ahb_rst)) {
>>> +       vop->ahb_rst = devm_reset_control_get(vop->dev, "ahb");
>>> +       if (IS_ERR(vop->ahb_rst)) {
>>>                 dev_err(vop->dev, "failed to get ahb reset\n");
>>> -               ret = PTR_ERR(ahb_rst);
>>> +               ret = PTR_ERR(vop->ahb_rst);
>>>                 goto err_disable_aclk;
>>>         }
>>> -       reset_control_assert(ahb_rst);
>>> +       reset_control_assert(vop->ahb_rst);
>>>         usleep_range(10, 20);
>>> -       reset_control_deassert(ahb_rst);
>>> +       reset_control_deassert(vop->ahb_rst);
>>>
>>>         memcpy(vop->regsbak, vop->regs, vop->len);
>>>
>>> --
>>> 2.8.0.rc3.226.g39d4020
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel