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

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

[Jitendra Sharma]

Remove unused id_table entries
Currently this driver supports only device tree based configuration.
So, no need in keeping the id_table entries

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.

Signed-off-by: Jitendra Sharma <shajit@codeaurora.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8ed3906..44eea5c 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));
 
@@ -1065,17 +1062,6 @@ static int adv7511_remove(struct i2c_client *i2c)
 	return 0;
 }
 
-static const struct i2c_device_id adv7511_i2c_ids[] = {
-	{ "adv7511", ADV7511 },
-	{ "adv7511w", ADV7511 },
-	{ "adv7513", ADV7511 },
-#ifdef CONFIG_DRM_I2C_ADV7533
-	{ "adv7533", ADV7533 },
-#endif
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, adv7511_i2c_ids);
-
 static const struct of_device_id adv7511_of_ids[] = {
 	{ .compatible = "adi,adv7511", .data = (void *)ADV7511 },
 	{ .compatible = "adi,adv7511w", .data = (void *)ADV7511 },
@@ -1096,7 +1082,6 @@ static int adv7511_remove(struct i2c_client *i2c)
 		.name = "adv7511",
 		.of_match_table = adv7511_of_ids,
 	},
-	.id_table = adv7511_i2c_ids,
 	.probe = adv7511_probe,
 	.remove = adv7511_remove,
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

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

[Laurent Pinchart]

Hi Jitendra,

Thank you for the patch.

On Tuesday 18 Oct 2016 15:39:58 Jitendra Sharma wrote:
> Remove unused id_table entries
> Currently this driver supports only device tree based configuration.
> So, no need in keeping the id_table entries
> 
> 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.
> 
> Signed-off-by: Jitendra Sharma <shajit@codeaurora.org>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8ed3906..44eea5c
> 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));
> 
> @@ -1065,17 +1062,6 @@ static int adv7511_remove(struct i2c_client *i2c)
>  	return 0;
>  }
> 
> -static const struct i2c_device_id adv7511_i2c_ids[] = {
> -	{ "adv7511", ADV7511 },
> -	{ "adv7511w", ADV7511 },
> -	{ "adv7513", ADV7511 },
> -#ifdef CONFIG_DRM_I2C_ADV7533
> -	{ "adv7533", ADV7533 },
> -#endif
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(i2c, adv7511_i2c_ids);
> -
>  static const struct of_device_id adv7511_of_ids[] = {
>  	{ .compatible = "adi,adv7511", .data = (void *)ADV7511 },
>  	{ .compatible = "adi,adv7511w", .data = (void *)ADV7511 },
> @@ -1096,7 +1082,6 @@ static int adv7511_remove(struct i2c_client *i2c)
>  		.name = "adv7511",
>  		.of_match_table = adv7511_of_ids,
>  	},
> -	.id_table = adv7511_i2c_ids,

Have you tested this ? With your patch applied the OF-instantiated adv7611w is 
never probed on my system.

>  	.probe = adv7511_probe,
>  	.remove = adv7511_remove,
>  };

-- 
Regards,

Laurent Pinchart

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

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

[Sharma, Jitendra]

Hi Laurent,

Thanks for trying it out.

On 10/18/2016 6:37 PM, Laurent Pinchart wrote:
> Hi Jitendra,
>
> Thank you for the patch.
>
> On Tuesday 18 Oct 2016 15:39:58 Jitendra Sharma wrote:
>> Remove unused id_table entries
>> Currently this driver supports only device tree based configuration.
>> So, no need in keeping the id_table entries
>>
>> 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.
>>
>> Signed-off-by: Jitendra Sharma <shajit@codeaurora.org>
>> ---
>>   drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 17 +----------------
>>   1 file changed, 1 insertion(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8ed3906..44eea5c
>> 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));
>>
>> @@ -1065,17 +1062,6 @@ static int adv7511_remove(struct i2c_client *i2c)
>>   	return 0;
>>   }
>>
>> -static const struct i2c_device_id adv7511_i2c_ids[] = {
>> -	{ "adv7511", ADV7511 },
>> -	{ "adv7511w", ADV7511 },
>> -	{ "adv7513", ADV7511 },
>> -#ifdef CONFIG_DRM_I2C_ADV7533
>> -	{ "adv7533", ADV7533 },
>> -#endif
>> -	{ }
>> -};
>> -MODULE_DEVICE_TABLE(i2c, adv7511_i2c_ids);
>> -
>>   static const struct of_device_id adv7511_of_ids[] = {
>>   	{ .compatible = "adi,adv7511", .data = (void *)ADV7511 },
>>   	{ .compatible = "adi,adv7511w", .data = (void *)ADV7511 },
>> @@ -1096,7 +1082,6 @@ static int adv7511_remove(struct i2c_client *i2c)
>>   		.name = "adv7511",
>>   		.of_match_table = adv7511_of_ids,
>>   	},
>> -	.id_table = adv7511_i2c_ids,
> Have you tested this ? With your patch applied the OF-instantiated adv7611w is
> never probed on my system.
Will look into it
>
>>   	.probe = adv7511_probe,
>>   	.remove = adv7511_remove,
>>   };

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

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

[Jitendra Sharma]

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
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
 	{ }
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

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

[Laurent Pinchart]

Hi Jitendra,

Thank you for the patch.

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.

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.

>  	{ }
>  };

-- 
Regards,

Laurent Pinchart

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

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

[Sharma, Jitendra]

Hi Laurent,


On 10/19/2016 5:21 PM, Laurent Pinchart wrote:
> Hi Jitendra,
>
> Thank you for the patch.
>
> 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
>
>>   	{ }
>>   };

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

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

[Archit Taneja]

Hi.,

On 10/19/2016 6:37 PM, Sharma, Jitendra wrote:
> Hi Laurent,
>
>
> On 10/19/2016 5:21 PM, Laurent Pinchart wrote:
>> Hi Jitendra,
>>
>> Thank you for the patch.
>>
>> 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 ?

Tested v2 on Jitendra's behalf.

Thanks,
Archit

>>
>>> 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
>>
>>>       { }
>>>   };
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

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

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

[Sharma, Jitendra]

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@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?
>>>>    	{ }
>>>>    
>>>>    };

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

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

[Laurent Pinchart]

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@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?

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 564 bytes --]


> > 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).

IIUC: I'd suggest to leave it as it is.

There are people working on removing the quirky fallback to
i2c_device_id in case of_compatible is not provided. If that gets
mainline, you don't have i2c_device_id anymore with DT matching. So all
binding methods should be self-contained.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

[Sharma, Jitendra]

[-- Attachment #1.1: Type: text/plain, Size: 893 bytes --]

Thanks for comments.


On 10/20/2016 2:15 PM, Wolfram Sang wrote:
>>> 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).
> IIUC: I'd suggest to leave it as it is.
>
> There are people working on removing the quirky fallback to
> i2c_device_id in case of_compatible is not provided. If that gets
> mainline, you don't have i2c_device_id anymore with DT matching. So all
> binding methods should be self-contained.
ok
Should we leave as it is, redundant if-else code block also, or fix it 
as posted in patch v2
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


[-- Attachment #1.2: Type: text/html, Size: 1835 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

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

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 110 bytes --]


> Should we leave as it is, redundant if-else code block also, or fix it as
> posted in patch v2

Leave it.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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