| Message ID | 1474050143-29962-1-git-send-email-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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);
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(-)