Hi Jitendra,
On Wednesday 19 Oct 2016 19:11:27 Sharma, Jitendra wrote:
> On 10/19/2016 7:03 PM, Laurent Pinchart wrote:
> > 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@xxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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()...
>
> Agreed.
> So, there could be two ways round either use id->driver_data
> unconditionally or use of_device_get_match_data().
> IMO it would be better to use id->driver_data unconditionally and save a
> call to of_device_get_match_data()
> What would you suggest to move ahead?
I'd suggest using id->driver_data unconditionally, but I'd like Wolfram's
opinion on this (CC'ed).
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
