Re: [PATCH v2] drm/bridge: adv7511: Remove unused code blocks

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: "Sharma, Jitendra" <shajit@codeaurora.org>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/bridge: adv7511: Remove unused code blocks
Date: Wed, 19 Oct 2016 16:33:14 +0300	[thread overview]
Message-ID: <3685503.gXBRGfjrFE@avalon> (raw)
In-Reply-To: <bb184408-2e75-a1f3-667f-575e44744f84@codeaurora.org>

Hi Jitendra,

On Wednesday 19 Oct 2016 18:37:38 Sharma, Jitendra wrote:
> On 10/19/2016 5:21 PM, Laurent Pinchart wrote:
> > On Wednesday 19 Oct 2016 17:12:48 Jitendra Sharma wrote:
> >> Remove redundant condition check
> >> Remove not necessary if-else block for checking DT entry because else
> >> part will never be picked as in absence of device node, probe will
> >> fail in initial stage only.
> >> 
> >> Remove unused id->driver_data entries
> >> As id->driver_data is not used in driver source. So no need in
> >> Keeping these entries in id_table
> >> 
> >> Signed-off-by: Jitendra Sharma <shajit@codeaurora.org>
> >> ---
> >> Probe was not happening in Patch v1 due to removal of .id_table.As the
> >> intention of this patch is not to change any functionality, also
> >> changes looks simple enough.So, didn't verified Patch v1 over hardware
> > 
> > You should *ALWAYS* verify patches before sending them out.
> 
> Will keep in mind
> 
> > I assume you've now properly tested this one ?
> > 
> >> Hence fixing the issues in Patch v1 and posting patch v2
> >> 
> >> Changes for v2:
> >>   - Keep the id_table entries
> >>   - Keep the id->driver_data to 0
> >> 
> >> ---
> >> 
> >>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 13 +++++--------
> >>   1 file changed, 5 insertions(+), 8 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8ed3906..3279059
> >> 100644
> >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> @@ -942,10 +942,7 @@ static int adv7511_probe(struct i2c_client *i2c,
> >> const
> >> struct i2c_device_id *id) adv7511->powered = false;
> >> 
> >>   	adv7511->status = connector_status_disconnected;
> >> 
> >> -	if (dev->of_node)
> >> -		adv7511->type = (enum
> > 
> > adv7511_type)of_device_get_match_data(dev);
> > 
> >> -	else
> >> -		adv7511->type = id->driver_data;
> >> +	adv7511->type = (enum adv7511_type)of_device_get_match_data(dev);
> >> 
> >>   	memset(&link_config, 0, sizeof(link_config));
> >> 
> >> @@ -1066,11 +1063,11 @@ static int adv7511_remove(struct i2c_client *i2c)
> >> 
> >>   }
> >>   
> >>   static const struct i2c_device_id adv7511_i2c_ids[] = {
> >> 
> >> -	{ "adv7511", ADV7511 },
> >> -	{ "adv7511w", ADV7511 },
> >> -	{ "adv7513", ADV7511 },
> >> +	{ "adv7511", 0 },
> >> +	{ "adv7511w", 0 },
> >> +	{ "adv7513", 0 },
> >> 
> >>   #ifdef CONFIG_DRM_I2C_ADV7533
> >> 
> >> -	{ "adv7533", ADV7533 },
> >> +	{ "adv7533", 0 },
> >> 
> >>   #endif
> > 
> > What's the purpose of this ? It doesn't save any memory or CPU cycle.
> 
> Idea is to remove unnecessary code, variables and if possible to reduce
> lines of code for example here by eliminating obvious branching.
> Regarding memory or cpu cyles, no difference could be because of
> compiler optimization

For the code block in the probe function I understand, but for the 
initializers here I don't see the point.

And one might argue that using id->driver_data unconditionally would be better 
as it would save a call to of_device_get_match_data()...

> >>   	{ }
> >>   
> >>   };

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel