drm/rockchip: Cleanup dangling devm pointers
diff mbox

Message ID 1474050143-29962-1-git-send-email-seanpaul@chromium.org
State New
Headers show

Commit Message

Sean Paul Sept. 16, 2016, 6:22 p.m. UTC
Instead of assigning device managed resources to local variables,
keep track of them in the vop struct.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Daniel Kurtz Sept. 19, 2016, 4:14 a.m. UTC | #1
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?

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.

-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
Sean Paul Sept. 21, 2016, 7:36 a.m. UTC | #2
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.

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
Daniel Kurtz Sept. 21, 2016, 8:55 a.m. UTC | #3
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

Patch
diff mbox

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);