Hi Laurent,
On 10/19/2016 7:03 PM, Laurent Pinchart wrote:
> 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 at 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()...
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?
>>>> { }
>>>>
>>>> };
