Re: [PATCH v3 1/5] net: rnpgbe: Add build support for rnpgbe

[MD Danish Anwar <danishanwar@ti.com>]

On 13/08/25 12:14 pm, Yibo Dong wrote:
> On Tue, Aug 12, 2025 at 09:48:07PM +0530, Anwar, Md Danish wrote:
>> On 8/12/2025 3:09 PM, Dong Yibo wrote:
>>> Add build options and doc for mucse.
>>> Initialize pci device access for MUCSE devices.
>>>
>>> Signed-off-by: Dong Yibo <dong100@mucse.com>
>>> ---
>>>  .../device_drivers/ethernet/index.rst         |   1 +
>>>  .../device_drivers/ethernet/mucse/rnpgbe.rst  |  21 +++
>>>  MAINTAINERS                                   |   8 +
>>>  drivers/net/ethernet/Kconfig                  |   1 +
>>>  drivers/net/ethernet/Makefile                 |   1 +
>>>  drivers/net/ethernet/mucse/Kconfig            |  34 ++++
>>>  drivers/net/ethernet/mucse/Makefile           |   7 +
>>>  drivers/net/ethernet/mucse/rnpgbe/Makefile    |   8 +
>>>  drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |  25 +++
>>>  .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   | 161 ++++++++++++++++++
>>>  10 files changed, 267 insertions(+)
>>>  create mode 100644 Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst
>>>  create mode 100644 drivers/net/ethernet/mucse/Kconfig
>>>  create mode 100644 drivers/net/ethernet/mucse/Makefile
>>>  create mode 100644 drivers/net/ethernet/mucse/rnpgbe/Makefile
>>>  create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
>>>  create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
>>
>> [ ... ]
>>
>>> + **/
>>> +static int __init rnpgbe_init_module(void)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = pci_register_driver(&rnpgbe_driver);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return 0;
>>> +}
>>
>> Unnecessary code - can be simplified to just `return
>> pci_register_driver(&rnpgbe_driver);`
>>
> 
> Yes, but if I add some new codes which need some free after
> pci_register_driver failed, the new patch will be like this:
> 
> -return pci_register_driver(&rnpgbe_driver);
> +int ret:
> +wq = create_singlethread_workqueue(rnpgbe_driver_name);
> +ret = pci_register_driver(&rnpgbe_driver);
> +if (ret) {
> +	destroy_workqueue(wq);
> +	return ret;
> +}
> +return 0;
> 
> Is this ok? Maybe not good to delete code for adding new feature?
> This is what Andrew suggested not to do.
> 

In this patch series you are not modifying rnpgbe_init_module() again.
If you define a function as something in one patch and in later patches
you change it to something else - That is not encouraged, you should not
remove the code that you added in previous patches.

However here throughout your series you are not modifying this function.
Now the diff that you are showing, I don't know when you plan to post
that but as far as this series is concerned this diff is not part of the
series.

static int __init rnpgbe_init_module(void)
{
	int ret;

	ret = pci_register_driver(&rnpgbe_driver);
	if (ret)
		return ret;

	return 0;
}

This to me just seems unnecessary. You can just return
pci_register_driver() now and in future whenever you add other code you
can modify the function.

It would have  made sense for you to keep it as it is if some later
patch in your series would have modified it.

>>> +
>>> +module_init(rnpgbe_init_module);
>>> +
>>> +/**
>>> + * rnpgbe_exit_module - Driver remove routine
>>> + *
>>> + * rnpgbe_exit_module is called when driver is removed
>>> + **/
>>> +static void __exit rnpgbe_exit_module(void)
>>> +{
>>> +	pci_unregister_driver(&rnpgbe_driver);
>>> +}
>>> +
>>> +module_exit(rnpgbe_exit_module);
>>> +
>>> +MODULE_DEVICE_TABLE(pci, rnpgbe_pci_tbl);
>>> +MODULE_AUTHOR("Mucse Corporation, <techsupport@mucse.com>");
>>> +MODULE_DESCRIPTION("Mucse(R) 1 Gigabit PCI Express Network Driver");
>>> +MODULE_LICENSE("GPL");
>>
>> -- 
>> Thanks and Regards,
>> Md Danish Anwar
>>
>>
> 
> Thanks for your feedback.

-- 
Thanks and Regards,
Danish