| Message ID | 1394712342-15778-368-Taiwan-albertk@realtek.com |
|---|---|
| Headers | show |
| Series | r8152: split the source code | expand |
On Mon, Jul 26, 2021 at 12:01:09PM +0800, Hayes Wang wrote: > Rename r8152.c with r8152_main.c. Move some basic definitions from > r8152_main.c to r8152_basic.h. Move the relative code of firmware > from r8152_main.c to r8152_fw.c. Rename the definition of "EFUSE" > with "USB_EFUSE". That is a lot of different things all happening in one commit, why? Please break this up into "one patch per change" and submit it that way. But the real question is why break this file up in the first place? What is wrong with the way it is today? What future changes require this file to be in smaller pieces? If none, why make this? If there are future changes, then please submit this change when you submit those, as that would show a real need. thanks, greg k-h
Greg KH <gregkh@linuxfoundation.org> > Sent: Monday, July 26, 2021 3:37 PM [...] > That is a lot of different things all happening in one commit, why? I plan to separate the file into two files. And I find I need an additional header file for it, so The patch includes adding that header file. > Please break this up into "one patch per change" and submit it that way. > > But the real question is why break this file up in the first place? > What is wrong with the way it is today? What future changes require > this file to be in smaller pieces? If none, why make this? If there > are future changes, then please submit this change when you submit > those, as that would show a real need. The purpose is let me easy to maintain the driver. The code is larger and larger. And I find that the r8169.c has been separated into three files. Therefore, I think maybe I could split the driver into small parts like r8169. Then, the code wouldn't be complex. Should I abandon these patches? Best Regards, Hayes
On Mon, Jul 26, 2021 at 08:26:00AM +0000, Hayes Wang wrote: > Greg KH <gregkh@linuxfoundation.org> > > Sent: Monday, July 26, 2021 3:37 PM > [...] > > That is a lot of different things all happening in one commit, why? > > I plan to separate the file into two files. And > I find I need an additional header file for it, so > The patch includes adding that header file. You also do other things, like renaming defines, which is not just moving code around, right? > > Please break this up into "one patch per change" and submit it that way. > > > > But the real question is why break this file up in the first place? > > What is wrong with the way it is today? What future changes require > > this file to be in smaller pieces? If none, why make this? If there > > are future changes, then please submit this change when you submit > > those, as that would show a real need. > > The purpose is let me easy to maintain the driver. > The code is larger and larger. And I find that the > r8169.c has been separated into three files. > Therefore, I think maybe I could split the driver > into small parts like r8169. Then, the code wouldn't > be complex. I do not know, is it really easier to find things in 3 different files instead of one? That's up to you, but you did not say why this change is needed. thanks, greg k-h
Greg KH <gregkh@linuxfoundation.org> > Sent: Monday, July 26, 2021 4:37 PM [...] > You also do other things, like renaming defines, which is not just > moving code around, right? Yes. You are right. [...] > I do not know, is it really easier to find things in 3 different files > instead of one? That's up to you, but you did not say why this change > is needed. We support a new chip or feature with a test driver. The test driver is similar with the upstream driver, except the method of the firmware. After we confirm that the test driver work fine, we compare the differences with the upstream driver and submit patches. And the code about firmware takes us more time to find out the differences. Therefore, I wish to move the part of the firmware out. I don't sure if it is acceptable for such patches. If it is unacceptable, I would abandon these patches. Best Regards, Hayes
On Mon, Jul 26, 2021 at 11:09:06AM +0000, Hayes Wang wrote: > Greg KH <gregkh@linuxfoundation.org> > > Sent: Monday, July 26, 2021 4:37 PM > [...] > > You also do other things, like renaming defines, which is not just > > moving code around, right? > > Yes. You are right. So resend the series and only do "one thing per patch" please. > [...] > > I do not know, is it really easier to find things in 3 different files > > instead of one? That's up to you, but you did not say why this change > > is needed. > > We support a new chip or feature with a test driver. > The test driver is similar with the upstream driver, except > the method of the firmware. After we confirm that the > test driver work fine, we compare the differences with > the upstream driver and submit patches. And the code > about firmware takes us more time to find out the > differences. Therefore, I wish to move the part of > the firmware out. Great, then submit the broken up driver as part of a patchset that adds new device support, as that makes more sense when that happens, right? thanks, greg k-h
Greg KH <gregkh@linuxfoundation.org> > Sent: Monday, July 26, 2021 7:14 PM [...] > > We support a new chip or feature with a test driver. > > The test driver is similar with the upstream driver, except > > the method of the firmware. After we confirm that the > > test driver work fine, we compare the differences with > > the upstream driver and submit patches. And the code > > about firmware takes us more time to find out the > > differences. Therefore, I wish to move the part of > > the firmware out. > > Great, then submit the broken up driver as part of a patchset that adds > new device support, as that makes more sense when that happens, right? I got it. I will submit them with the support of new device in the future. Best Regards, Hayes
The r8152.c is too large to find out the desired part, so I speparate it into r8152_main.c and r8152_fw.c. Hayes Wang (2): r8152: group the usb ethernet of realtek r8152: separate the r8152.c into r8152_main.c and r8152_fw.c MAINTAINERS | 10 +- drivers/net/usb/Kconfig | 30 +- drivers/net/usb/Makefile | 4 +- drivers/net/usb/realtek/Kconfig | 33 + drivers/net/usb/realtek/Makefile | 9 + drivers/net/usb/realtek/r8152_basic.h | 860 ++++++ drivers/net/usb/realtek/r8152_fw.c | 1557 ++++++++++ .../net/usb/{r8152.c => realtek/r8152_main.c} | 2585 +---------------- drivers/net/usb/{ => realtek}/r8153_ecm.c | 0 drivers/net/usb/{ => realtek}/rtl8150.c | 0 10 files changed, 2576 insertions(+), 2512 deletions(-) create mode 100644 drivers/net/usb/realtek/Kconfig create mode 100644 drivers/net/usb/realtek/Makefile create mode 100644 drivers/net/usb/realtek/r8152_basic.h create mode 100644 drivers/net/usb/realtek/r8152_fw.c rename drivers/net/usb/{r8152.c => realtek/r8152_main.c} (75%) rename drivers/net/usb/{ => realtek}/r8153_ecm.c (100%) rename drivers/net/usb/{ => realtek}/rtl8150.c (100%)