Messages in this thread | | | Subject | Re: [PATCH][next] drm/rockchip: Remove redundant assignment of pointer connector | From | Alex Bee <> | Date | Wed, 22 Sep 2021 19:31:46 +0200 |
| |
Hi Heiko,
Am 22.09.21 um 18:45 schrieb Heiko Stübner: > Hi Alex, > > Am Mittwoch, 22. September 2021, 18:35:38 CEST schrieb Alex Bee: >> Hi Colin, >> Am 22.09.21 um 13:24 schrieb Colin King: >>> From: Colin Ian King <colin.king@canonical.com> >>> >>> The pointer connector is being assigned a value that is never >>> read, it is being updated immediately afterwards. The assignment >>> is redundant and can be removed. >> The pointer to the connector is used in rockchip_rgb_fini for >> drm_connector_cleanup. >> It's pretty much the same for the encoder, btw. > I think the issue is more the two lines > > connector = &rgb->connector; > connector = drm_bridge_connector_init(rgb->drm_dev, encoder); > > hence the connector = &rgb->connector being overwritten immediately after > > Now that I look at it again, the whole approach looks strange. > drm_bridge_connector_init() creates the connector structure and > returns a pointer to it.
Totally agreed.
The main reason I was doing it that way, was the way it was done already in rockchip_lvds.c, where the connector was already existent in the struct rockchip_lvds (and was already used in the panel-case - all places where it is used accept pointers also, btw) and is *no* pointer - and is done already this very strange way.
I wanted to re-use it for the bridge-case and didn't want to differ in coding in rockchip-rgb to much.
The only reason I can think of, why it was done that way is, that we might need a pointer to a fully initialized struct drm_connector for some reason (drm_connector_cleanup ?), what we wouldn't have if have just a pointer and something goes wrong before drm_connector_init respectivly drm_bridge_connector_init.
Alex
> So the first line below sets the connector pointer to point to the > &rgb->connector element and the second line then set a completely > different address into it. > > So the connector element in rockchip_lvds and rockchip_rgb should actually > become a pointer itself to hold the connector element returned from > drm_bridge_connector_init() . > > > Heiko > >> Regards, >> >> Alex >>> Addresses-Coverity: ("Unused value") >>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>> --- >>> drivers/gpu/drm/rockchip/rockchip_rgb.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c >>> index 09be9678f2bd..18fb84068a64 100644 >>> --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c >>> +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c >>> @@ -150,7 +150,6 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev, >>> if (ret) >>> goto err_free_encoder; >>> >>> - connector = &rgb->connector; >>> connector = drm_bridge_connector_init(rgb->drm_dev, encoder); >>> if (IS_ERR(connector)) { >>> DRM_DEV_ERROR(drm_dev->dev, >>> >> > > >
| |