All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/rockchip: Cleanup dangling devm pointers
@ 2016-09-16 18:22 Sean Paul
  2016-09-19  4:14 ` Daniel Kurtz
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Paul @ 2016-09-16 18:22 UTC (permalink / raw)
  To: mark.yao, heiko, dri-devel; +Cc: linux-rockchip

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

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

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/rockchip: Cleanup dangling devm pointers
  2016-09-16 18:22 [PATCH] drm/rockchip: Cleanup dangling devm pointers Sean Paul
@ 2016-09-19  4:14 ` Daniel Kurtz
  2016-09-21  7:36   ` Sean Paul
  0 siblings, 1 reply; 4+ messages in thread
From: Daniel Kurtz @ 2016-09-19  4:14 UTC (permalink / raw)
  To: Sean Paul; +Cc: open list:ARM/Rockchip SoC..., dri-devel

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/rockchip: Cleanup dangling devm pointers
  2016-09-19  4:14 ` Daniel Kurtz
@ 2016-09-21  7:36   ` Sean Paul
  2016-09-21  8:55     ` Daniel Kurtz
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Paul @ 2016-09-21  7:36 UTC (permalink / raw)
  To: Daniel Kurtz; +Cc: open list:ARM/Rockchip SoC..., dri-devel

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] drm/rockchip: Cleanup dangling devm pointers
  2016-09-21  7:36   ` Sean Paul
@ 2016-09-21  8:55     ` Daniel Kurtz
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Kurtz @ 2016-09-21  8:55 UTC (permalink / raw)
  To: Sean Paul; +Cc: open list:ARM/Rockchip SoC..., dri-devel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-09-21  8:55 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-16 18:22 [PATCH] drm/rockchip: Cleanup dangling devm pointers Sean Paul
2016-09-19  4:14 ` Daniel Kurtz
2016-09-21  7:36   ` Sean Paul
2016-09-21  8:55     ` Daniel Kurtz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.