[PATCH net-next v2 0/2] cleanup for Realtek 8139CP

[PATCH net-next v2 0/2] cleanup for Realtek 8139CP

[varkabhadram]

From: Varka Bhadram <varkab@cdac.in>

changes since v1:
	-removed unused varible warning

Varka Bhadram (2):
  ethernet: realtek: use module_pci_driver
  ethernet: realtek: use pci_device_id

 drivers/net/ethernet/realtek/8139cp.c |   36 +++++++++------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

-- 
1.7.9.5

[PATCH net-next v2 1/2] ethernet: realtek: use module_pci_driver

[varkabhadram]

From: Varka Bhadram <varkab@cdac.in>

This patch converts to use the macro module_pci_driver, which makes
the code smaller and simpler.

Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
 drivers/net/ethernet/realtek/8139cp.c |   22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 2bc728e..9fb68b9 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -1887,11 +1887,7 @@ static int cp_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
 	resource_size_t pciaddr;
 	unsigned int addr_len, i, pci_using_dac;
 
-#ifndef MODULE
-	static int version_printed;
-	if (version_printed++ == 0)
-		pr_info("%s", version);
-#endif
+	pr_info("%s", version);
 
 	if (pdev->vendor == PCI_VENDOR_ID_REALTEK &&
 	    pdev->device == PCI_DEVICE_ID_REALTEK_8139 && pdev->revision < 0x20) {
@@ -2121,18 +2117,4 @@ static struct pci_driver cp_driver = {
 #endif
 };
 
-static int __init cp_init (void)
-{
-#ifdef MODULE
-	pr_info("%s", version);
-#endif
-	return pci_register_driver(&cp_driver);
-}
-
-static void __exit cp_exit (void)
-{
-	pci_unregister_driver (&cp_driver);
-}
-
-module_init(cp_init);
-module_exit(cp_exit);
+module_pci_driver(cp_driver);
-- 
1.7.9.5

[PATCH net-next v2 2/2] ethernet: realtek: use pci_device_id

[varkabhadram]

From: Varka Bhadram <varkab@cdac.in>

This patch use the struct pci_device_id instead of using macro
DEFINE_PCI_DEVICE_TABLE which is deprecated and should not be used.
And also moves these ids after probe and remove functionalities.

Signed-off-by: Varka Bhadram <varkab@cdac.in>
---
 drivers/net/ethernet/realtek/8139cp.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
index 9fb68b9..6873122 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -382,13 +382,6 @@ static int cp_get_eeprom(struct net_device *dev,
 static int cp_set_eeprom(struct net_device *dev,
 			 struct ethtool_eeprom *eeprom, u8 *data);
 
-static DEFINE_PCI_DEVICE_TABLE(cp_pci_tbl) = {
-	{ PCI_DEVICE(PCI_VENDOR_ID_REALTEK,	PCI_DEVICE_ID_REALTEK_8139), },
-	{ PCI_DEVICE(PCI_VENDOR_ID_TTTECH,	PCI_DEVICE_ID_TTTECH_MC322), },
-	{ },
-};
-MODULE_DEVICE_TABLE(pci, cp_pci_tbl);
-
 static struct {
 	const char str[ETH_GSTRING_LEN];
 } ethtool_stats_keys[] = {
@@ -2106,6 +2099,13 @@ static int cp_resume (struct pci_dev *pdev)
 }
 #endif /* CONFIG_PM */
 
+static const struct pci_device_id cp_pci_tbl[] = {
+        { PCI_DEVICE(PCI_VENDOR_ID_REALTEK,     PCI_DEVICE_ID_REALTEK_8139), },
+        { PCI_DEVICE(PCI_VENDOR_ID_TTTECH,      PCI_DEVICE_ID_TTTECH_MC322), },
+        { },
+};
+MODULE_DEVICE_TABLE(pci, cp_pci_tbl);
+
 static struct pci_driver cp_driver = {
 	.name         = DRV_NAME,
 	.id_table     = cp_pci_tbl,
-- 
1.7.9.5

Re: [PATCH net-next v2 1/2] ethernet: realtek: use module_pci_driver

[David Miller]

From: varkabhadram@gmail.com
Date: Tue, 22 Jul 2014 12:50:21 +0530

> @@ -1887,11 +1887,7 @@ static int cp_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
>  	resource_size_t pciaddr;
>  	unsigned int addr_len, i, pci_using_dac;
>  
> -#ifndef MODULE
> -	static int version_printed;
> -	if (version_printed++ == 0)
> -		pr_info("%s", version);
> -#endif
> +	pr_info("%s", version);

Now you're changing behavior undesirably, it will now print the
version string into the logs for every instance of the device which is
discovered.

Seriously, the driver is worse off after these "cleanups".

Re: [PATCH net-next v2 1/2] ethernet: realtek: use module_pci_driver

[Varka Bhadram]

On 07/22/2014 12:58 PM, David Miller wrote:
> From: varkabhadram@gmail.com
> Date: Tue, 22 Jul 2014 12:50:21 +0530
>
>> @@ -1887,11 +1887,7 @@ static int cp_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
>>   	resource_size_t pciaddr;
>>   	unsigned int addr_len, i, pci_using_dac;
>>   
>> -#ifndef MODULE
>> -	static int version_printed;
>> -	if (version_printed++ == 0)
>> -		pr_info("%s", version);
>> -#endif
>> +	pr_info("%s", version);
> Now you're changing behavior undesirably, it will now print the
> version string into the logs for every instance of the device which is
> discovered.
>
> Seriously, the driver is worse off after these "cleanups".

Here we are having two possibilities here:

1. Removing the version info completely. Ofcourse this is not desirable.
  
2. Accepting the version info to be print into log messages.

I will put like this by removing #ifdefs:
	static int version_printed;
	if (version_printed++ == 0)
		pr_info("%s", version);


Will it be OK ..?

-- 
Regards,
Varka Bhadram.

Re: [PATCH net-next v2 1/2] ethernet: realtek: use module_pci_driver

[Sergei Shtylyov]

On 07/22/2014 11:20 AM, varkabhadram@gmail.com wrote:

> From: Varka Bhadram <varkab@cdac.in>

> This patch converts to use the macro module_pci_driver, which makes
> the code smaller and simpler.

> Signed-off-by: Varka Bhadram <varkab@cdac.in>
> ---
>   drivers/net/ethernet/realtek/8139cp.c |   22 ++--------------------
>   1 file changed, 2 insertions(+), 20 deletions(-)

> diff --git a/drivers/net/ethernet/realtek/8139cp.c b/drivers/net/ethernet/realtek/8139cp.c
> index 2bc728e..9fb68b9 100644
> --- a/drivers/net/ethernet/realtek/8139cp.c
> +++ b/drivers/net/ethernet/realtek/8139cp.c
> @@ -1887,11 +1887,7 @@ static int cp_init_one (struct pci_dev *pdev, const struct pci_device_id *ent)
>   	resource_size_t pciaddr;
>   	unsigned int addr_len, i, pci_using_dac;
>
> -#ifndef MODULE
> -	static int version_printed;
> -	if (version_printed++ == 0)
> -		pr_info("%s", version);
> -#endif
> +	pr_info("%s", version);

    Use pr_info_once().

WBR, Sergei

Re: [PATCH net-next v2 1/2] ethernet: realtek: use module_pci_driver

[Varka Bhadram]

On Tuesday 22 July 2014 08:08 PM, Sergei Shtylyov wrote:
> On 07/22/2014 11:20 AM, varkabhadram@gmail.com wrote:
>
>> From: Varka Bhadram <varkab@cdac.in>
>
>> This patch converts to use the macro module_pci_driver, which makes
>> the code smaller and simpler.
>
>> Signed-off-by: Varka Bhadram <varkab@cdac.in>
>> ---
>>   drivers/net/ethernet/realtek/8139cp.c |   22 ++--------------------
>>   1 file changed, 2 insertions(+), 20 deletions(-)
>
>> diff --git a/drivers/net/ethernet/realtek/8139cp.c 
>> b/drivers/net/ethernet/realtek/8139cp.c
>> index 2bc728e..9fb68b9 100644
>> --- a/drivers/net/ethernet/realtek/8139cp.c
>> +++ b/drivers/net/ethernet/realtek/8139cp.c
>> @@ -1887,11 +1887,7 @@ static int cp_init_one (struct pci_dev *pdev, 
>> const struct pci_device_id *ent)
>>       resource_size_t pciaddr;
>>       unsigned int addr_len, i, pci_using_dac;
>>
>> -#ifndef MODULE
>> -    static int version_printed;
>> -    if (version_printed++ == 0)
>> -        pr_info("%s", version);
>> -#endif
>> +    pr_info("%s", version);
>
>    Use pr_info_once().
>
> WBR, Sergei
>
I am going to utilize the removed code by removing the #ifndef MODULE...

It will be like:

	static int version_printed;

	if (version_printed++ == 0)
		pr_info("%s", version);


Will it be Ok...?

-- 
Regards,
Varka Bhadram

Re: [PATCH net-next v2 1/2] ethernet: realtek: use module_pci_driver

[Sergei Shtylyov]

On 07/22/2014 06:43 PM, Varka Bhadram wrote:

>>> From: Varka Bhadram <varkab@cdac.in>

>>> This patch converts to use the macro module_pci_driver, which makes
>>> the code smaller and simpler.

>>> Signed-off-by: Varka Bhadram <varkab@cdac.in>
>>> ---
>>>   drivers/net/ethernet/realtek/8139cp.c |   22 ++--------------------
>>>   1 file changed, 2 insertions(+), 20 deletions(-)

>>> diff --git a/drivers/net/ethernet/realtek/8139cp.c
>>> b/drivers/net/ethernet/realtek/8139cp.c
>>> index 2bc728e..9fb68b9 100644
>>> --- a/drivers/net/ethernet/realtek/8139cp.c
>>> +++ b/drivers/net/ethernet/realtek/8139cp.c
>>> @@ -1887,11 +1887,7 @@ static int cp_init_one (struct pci_dev *pdev, const
>>> struct pci_device_id *ent)
>>>       resource_size_t pciaddr;
>>>       unsigned int addr_len, i, pci_using_dac;
>>>
>>> -#ifndef MODULE
>>> -    static int version_printed;
>>> -    if (version_printed++ == 0)
>>> -        pr_info("%s", version);
>>> -#endif
>>> +    pr_info("%s", version);

>>    Use pr_info_once().

>> WBR, Sergei

> I am going to utilize the removed code by removing the #ifndef MODULE...

> It will be like:
>
>      static int version_printed;
>
>      if (version_printed++ == 0)
>          pr_info("%s", version);

    pr_info_once() is much shorter and the effect is the same.

WBR, Sergei

Re: [PATCH net-next v2 1/2] ethernet: realtek: use module_pci_driver

[Varka Bhadram]

On Tuesday 22 July 2014 08:31 PM, Sergei Shtylyov wrote:
> On 07/22/2014 06:43 PM, Varka Bhadram wrote:
>
>>>> From: Varka Bhadram <varkab@cdac.in>
>
>>>> This patch converts to use the macro module_pci_driver, which makes
>>>> the code smaller and simpler.
>
>>>> Signed-off-by: Varka Bhadram <varkab@cdac.in>
>>>> ---
>>>>   drivers/net/ethernet/realtek/8139cp.c |   22 ++--------------------
>>>>   1 file changed, 2 insertions(+), 20 deletions(-)
>
>>>> diff --git a/drivers/net/ethernet/realtek/8139cp.c
>>>> b/drivers/net/ethernet/realtek/8139cp.c
>>>> index 2bc728e..9fb68b9 100644
>>>> --- a/drivers/net/ethernet/realtek/8139cp.c
>>>> +++ b/drivers/net/ethernet/realtek/8139cp.c
>>>> @@ -1887,11 +1887,7 @@ static int cp_init_one (struct pci_dev 
>>>> *pdev, const
>>>> struct pci_device_id *ent)
>>>>       resource_size_t pciaddr;
>>>>       unsigned int addr_len, i, pci_using_dac;
>>>>
>>>> -#ifndef MODULE
>>>> -    static int version_printed;
>>>> -    if (version_printed++ == 0)
>>>> -        pr_info("%s", version);
>>>> -#endif
>>>> +    pr_info("%s", version);
>
>>>    Use pr_info_once().
>
>>> WBR, Sergei
>
>> I am going to utilize the removed code by removing the #ifndef MODULE...
>
>> It will be like:
>>
>>      static int version_printed;
>>
>>      if (version_printed++ == 0)
>>          pr_info("%s", version);
>
>    pr_info_once() is much shorter and the effect is the same.
>
> WBR, Sergei
>
Thanks for your suggestion. I will do this in next version.


-- 
Regards,
Varka Bhadram