Skip to contentSkip to conversation

drivers/net/enc28j60.c - generalisation, how?

U-Boot Superseded Reinhard Meyer 5 participants 21 messages Jul 24, 2010 – Aug 31, 2010 Network drivers

lore
Share & tools
lore

Reinhard Meyer

author
#0 lore raw

Hello,

I would like to generalize that driver and make it also CONFIG_NET_MULTI-able.

Currently it contains hard coded functionality for the LPC2292 Cpu and is used by only two boards: lpc2292sodimm and SMN42.

Since the driver could be clearly generic, using just SPI and two I/O Pins for
/reset and /select (the latter could be considered part of SPI), it would make sense to ban out any board specific code from it.

When I do that, clearly I cannot test the two above boards for continued functionality.

Messing up the driver with #ifdefs to keep old code parallel to new one does not look like a nice approach either.

How to proceed?

Reinhard

#1 lore raw

Dear Reinhard Meyer,

Quoted 3 lines · In message <[email protected]> you wrote:

I would like to generalize that driver and make it also CONFIG_NET_MULTI-able.

...

Quoted 5 lines

When I do that, clearly I cannot test the two above boards for continued functionality.

Messing up the driver with #ifdefs to keep old code parallel to new one does not look like a nice approach either.

Submit patches, and put the board maintainers (if there are any) on Cc:

If nobody complains the patches go in.

Best regards,

Wolfgang Denk

Signature · 5 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Karl's version of Parkinson's Law: Work expands to  exceed  the  time
alloted it.
author
#2 lore raw
Quoted 5 lines · Wolfgang Denk

Submit patches, and put the board maintainers (if there are any) on Cc:

If nobody complains the patches go in.

Hello,

making the source adhere to coding style,
removing wrappers that only change function names,
and making it CONFIG_NET_MULTI-able seems to change almost every line in the source. Would it not be better to introduce it as a new driver and phase out the old one after a while?

Another issue is that the source has roughly 200 lines #defining the chips register layout. Those could be moved to a header file named "enc28j60.h". Where should that file be? drivers/net - like most other drivers?

I also need some shared function prototypes between driver and a board specific file, example for
int enc28j60_eth_initialize(int id, unsigned int phy_addr) where should that go?

Reinhard

#3 lore raw
In reply to Reinhard Meyer
Quoted 2 lines · On Saturday, July 24, 2010 02:11:16 Reinhard Meyer wrote:

I would like to generalize that driver and make it also CONFIG_NET_MULTI-able.

someone already has ... look at the archives in december of 09
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100725/606d682b/attachment.pgp

author
#4 lore raw
Quoted 8 lines · Mike Frysinger
Quoted 3 lines · On Saturday, July 24, 2010 02:11:16 Reinhard Meyer wrote:

I would like to generalize that driver and make it also CONFIG_NET_MULTI-able.

someone already has ... look at the archives in december of 09

Thanks for the hint Mike,
it seems the thread there has died, and the work left incomplete however. But it gives a head start to complete it.
The main issue I had is already proposed in that thread: rename the old driver and make a new one :)
Reinhard

#5 lore raw
Quoted 5 lines · On Sunday, July 25, 2010 16:06:31 Reinhard Meyer wrote:
Quoted 4 lines · Mike Frysinger
Quoted 2 lines · On Saturday, July 24, 2010 02:11:16 Reinhard Meyer wrote:

I would like to generalize that driver and make it also CONFIG_NET_MULTI-able.

Quoted 7 lines
Quoted 1 line

someone already has ... look at the archives in december of 09

Thanks for the hint Mike,
it seems the thread there has died, and the work left incomplete however. But it gives a head start to complete it.
The main issue I had is already proposed in that thread: rename the old driver and make a new one :)

best to continue that work though rather than try and start your own from scratch ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100725/adbdec3b/attachment.pgp

#6 lore raw
Quoted 8 lines · On Sunday, July 25, 2010 16:40:14 Mike Frysinger wrote:
Quoted 7 lines · On Sunday, July 25, 2010 16:06:31 Reinhard Meyer wrote:
Quoted 5 lines · Mike Frysinger
Quoted 2 lines · On Saturday, July 24, 2010 02:11:16 Reinhard Meyer wrote:

I would like to generalize that driver and make it also CONFIG_NET_MULTI-able.

someone already has ... look at the archives in december of 09

Quoted 8 lines
Quoted 5 lines

Thanks for the hint Mike,
it seems the thread there has died, and the work left incomplete however. But it gives a head start to complete it.
The main issue I had is already proposed in that thread: rename the old driver and make a new one :)

best to continue that work though rather than try and start your own from scratch ...

btw, i wasnt able to test the driver previously because it required a GPIO SPI CS which the Blackfin SPI driver did not previously support. but ive implemented & pushed that since, so i should be able to test the driver once you post it to make sure it does work across platforms (like Blackfin boards). -mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100726/87d04b1e/attachment.pgp

#7 lore raw
In reply to Reinhard Meyer

Hi Reinhard,

Quoted 7 lines · Reinhard Meyer
Quoted 6 lines · Mike Frysinger
Quoted 3 lines · On Saturday, July 24, 2010 02:11:16 Reinhard Meyer wrote:

I would like to generalize that driver and make it also CONFIG_NET_MULTI-able.

Quoted 8 lines
Quoted 2 lines

someone already has ... look at the archives in december of 09

Thanks for the hint Mike,
it seems the thread there has died, and the work left incomplete however. But it gives a head start to complete it.
The main issue I had is already proposed in that thread: rename the old driver and make a new one :)

Then that seems like the right approach. Create the new driver, port the existing boards to use it and see if anybody cares. If somebody responds, you've found a tester. If not, there may be some boards in the tree that compile fine but don't run. Not a problem I'd lose much sleep over.

Quoted 2 lines

Reinhard

Thanks for the effort!
--Ben

author
#8 lore raw
Quoted 25 lines · Ben Warren

Hi Reinhard,

Quoted 9 lines · Reinhard Meyer
Quoted 7 lines · Mike Frysinger
Quoted 4 lines · On Saturday, July 24, 2010 02:11:16 Reinhard Meyer wrote:

I would like to generalize that driver and make it also CONFIG_NET_MULTI-able.

Quoted 10 lines
Quoted 3 lines

someone already has ... look at the archives in december of 09

Thanks for the hint Mike,
it seems the thread there has died, and the work left incomplete however. But it gives a head start to complete it.
The main issue I had is already proposed in that thread: rename the old driver and make a new one :)

Quoted 5 lines

Then that seems like the right approach. Create the new driver, port the existing boards to use it and see if anybody cares. If somebody responds, you've found a tester. If not, there may be some boards in the tree that compile fine but don't run. Not a problem I'd lose much sleep over.

Hello Ben,
I can rename the current driver to like "enc28j60_lpc2292.c" and make the two affected boards use it, so they still compile and work. I can't do those boards' maintainers work and add the required code to use the new driver and make a generic spi framework for the lpc2292. They currently use a very private spi approach.
Or I call the new driver something like "enc28j60_generic.c". I am definitely not going to port the existing two lpc2292 boards to use the new enc28j60 driver.
Reinhard

#9 lore raw
Quoted 8 lines · On Sunday, July 25, 2010 20:07:31 Reinhard Meyer wrote:

I can rename the current driver to like "enc28j60_lpc2292.c" and make the two affected boards use it, so they still compile and work. I can't do those boards' maintainers work and add the required code to use the new driver and make a generic spi framework for the lpc2292. They currently use a very private spi approach.
Or I call the new driver something like "enc28j60_generic.c". I am definitely not going to port the existing two lpc2292 boards to use the new enc28j60 driver.

i prefer the former
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100725/d8f70130/attachment.pgp

author
#10 lore raw
Quoted 12 lines · Mike Frysinger
Quoted 9 lines · On Sunday, July 25, 2010 20:07:31 Reinhard Meyer wrote:

I can rename the current driver to like "enc28j60_lpc2292.c" and make the two affected boards use it, so they still compile and work. I can't do those boards' maintainers work and add the required code to use the new driver and make a generic spi framework for the lpc2292. They currently use a very private spi approach.
Or I call the new driver something like "enc28j60_generic.c". I am definitely not going to port the existing two lpc2292 boards to use the new enc28j60 driver.

Quoted 2 lines

i prefer the former

I agree. So a first patch would be the renaming, editing the affected boards *.h and the Makefile in drivers/net:
COBJS-$(CONFIG_ENC28J60) += enc28j60.o
to
COBJS-$(CONFIG_ENC28J60_LPC2292) += enc28j60_lpc2292.o

In the same patch extract the 200 or so defines for the chip to enc28j60.h (to be used by both drivers).

One issue remains: the old driver has a lot of coding style violations, I can fix them, but see no point in that. The new driver will have them fixed.

But before I proceed I need Bens OK to go that way:)

Reinhard

#11 lore raw
Quoted 24 lines · Reinhard Meyer

Mike Frysinger wrote:

On Sunday, July 25, 2010 20:07:31 Reinhard Meyer wrote:

I can rename the current driver to like "enc28j60_lpc2292.c" and make the two affected boards use it, so they still compile and work. I can't do those boards' maintainers work and add the required code to use the new driver and make a generic spi framework for the lpc2292. They currently use a very private spi approach.
Or I call the new driver something like "enc28j60_generic.c". I am definitely not going to port the existing two lpc2292 boards to use the new enc28j60 driver.

i prefer the former

I agree. So a first patch would be the renaming, editing the affected boards *.h and the Makefile in drivers/net:
COBJS-$(CONFIG_ENC28J60) += enc28j60.o
to
COBJS-$(CONFIG_ENC28J60_LPC2292) += enc28j60_lpc2292.o

Quoted 8 lines

In the same patch extract the 200 or so defines for the chip to enc28j60.h (to be used by both drivers).

One issue remains: the old driver has a lot of coding style violations, I can fix them, but see no point in that. The new driver will have them fixed.

But before I proceed I need Bens OK to go that way:)

Reinhard

Sounds good. Go for it.

Regards,
Ben

#12 lore raw

Hi all,

u-boot-bounces at lists.denx.de wrote on Monday, July 26, 2010 4:51 PM:

Quoted 39 lines
Quoted 37 lines · Reinhard Meyer

Mike Frysinger wrote:

On Sunday, July 25, 2010 20:07:31 Reinhard Meyer wrote:

I can rename the current driver to like "enc28j60_lpc2292.c" and make the
two affected boards use it, so they still compile and work. I can't do
those boards' maintainers work and add the required code to use the new
driver and make a generic spi framework for the lpc2292. They currently
use a very private spi approach.
Or I call the new driver something like "enc28j60_generic.c". I am definitely not going to port the existing two lpc2292 boards to use
the new enc28j60 driver.

i prefer the former

I agree. So a first patch would be the renaming, editing the affected boards *.h and the Makefile in drivers/net:
COBJS-$(CONFIG_ENC28J60) += enc28j60.o
to
COBJS-$(CONFIG_ENC28J60_LPC2292) += enc28j60_lpc2292.o

In the same patch extract the 200 or so defines for the chip to enc28j60.h (to be used by both drivers).

One issue remains: the old driver has a lot of coding style violations, I can fix them, but see no point in that. The new driver will have them fixed.

Quoted 5 lines
Quoted 4 lines

But before I proceed I need Bens OK to go that way:)

Reinhard

Sounds good. Go for it.

Some time ago I modified the current encc28j60 driver to use the generic SPI api and to work with CONFIG_NET_MULTI. It was for a project not part of mainline U-Boot (and a bit of a hack), so I didn't post the patches on the mailing list.

Reinhard, if you are interested, I could send you my patch, or post it on the mailing list.

Regards,
Martin

author
#13 lore raw
Quoted 8 lines · Martin Krause

Hi all,

Quoted 1 line

Some time ago I modified the current encc28j60 driver to use the

generic SPI api and to work with CONFIG_NET_MULTI. It was for a project not part of mainline U-Boot (and a bit of a hack), so I didn't post the patches on the mailing list.

Reinhard, if you are interested, I could send you my patch, or post it on the mailing list.

Yes, of course, at your discretion to the list, but if it is not yet in "acceptable patch format" it will suffice to send it to me. I will try to make it work on our AT91 based board (as a 2nd LAN interface, so CONFIG_NET_MULTI will be really required) and later submit a patch if all goes well.

Best Regards, Reinhard

#14 lore raw

Reinhard Meyer (-VC) wrote on Tuesday, July 27, 2010 9:29 AM:

Quoted 7 lines
Quoted 6 lines · Martin Krause

Hi all,

Quoted 1 line

Some time ago I modified the current encc28j60 driver to use the

generic SPI api and to work with CONFIG_NET_MULTI. It was for a project not part of mainline U-Boot (and a bit of a hack), so I didn't post the patches on the mailing list.

Quoted 8 lines
Quoted 2 lines

Reinhard, if you are interested, I could send you my patch, or post it on the mailing list.

Yes, of course, at your discretion to the list, but if it is not yet in "acceptable patch format" it will suffice to send it to me. I will try to make it work on our AT91 based board (as a 2nd LAN interface, so CONFIG_NET_MULTI will be really required) and later submit a patch if all goes well.

OK, I send the patch directly to you, because it _is_ not in an "acceptable patch format" for the list ...

Regards,
Martin

author
#15 lore raw

PATCH 1/1 NET: move legacy enc28j60.c to sidetrack as enc28j60_lpc2292.c

In reply to Ben Warren

This patch is required before the upcoming new enc28j60 driver using SPI framework patch can be applied:
- Move legacy enc28j60.c to enc28j60_lpc2292.c.
- Change Makefile and the two affected boards' definition files.

Tested with ./MAKEALL ARM7 that both boards still compile.

Signed-off-by Reinhard Meyer <[email protected]>
---
 drivers/net/Makefile            |    2 +-
 drivers/net/enc28j60.c          |  982 --------------------------------------
 drivers/net/enc28j60_lpc2292.c  |  983 +++++++++++++++++++++++++++++++++++++++
 include/configs/SMN42.h         |    2 +-
 include/configs/lpc2292sodimm.h |    2 +-
 5 files changed, 986 insertions(+), 985 deletions(-)
 delete mode 100644 drivers/net/enc28j60.c
 create mode 100644 drivers/net/enc28j60_lpc2292.c
drivers/net/Makefile +1 -1
index 218eeff..479954d 100644
@@ -39,7 +39,7 @@ COBJS-$(CONFIG_DRIVER_DM9000) += dm9000x.o
 COBJS-$(CONFIG_DNET) += dnet.o
 COBJS-$(CONFIG_E1000) += e1000.o
 COBJS-$(CONFIG_EEPRO100) += eepro100.o
-COBJS-$(CONFIG_ENC28J60) += enc28j60.o
+COBJS-$(CONFIG_ENC28J60_LPC2292) += enc28j60_lpc2292.o
 COBJS-$(CONFIG_EP93XX) += ep93xx_eth.o
 COBJS-$(CONFIG_ETHOC) += ethoc.o
 COBJS-$(CONFIG_FEC_MXC) += fec_mxc.o
drivers/net/enc28j60.c deleted +0 -982

982 lines not on this page · raw message

drivers/net/enc28j60_lpc2292.c new file +983 -0

983 lines not on this page · raw message

include/configs/SMN42.h +1 -1
index adb6ac5..ba3ada1 100644
@@ -198,6 +198,6 @@
 #define CONFIG_INITRD_TAG
 #define CONFIG_MMC			1
 /* we use this ethernet chip */
-#define CONFIG_ENC28J60
+#define CONFIG_ENC28J60_LPC2292
 
 #endif	/* __CONFIG_H */
include/configs/lpc2292sodimm.h +1 -1
index 7ce8d6d..65276a2 100644
@@ -156,6 +156,6 @@
 #define CONFIG_INITRD_TAG
 #define CONFIG_MMC 1
 /* we use this ethernet chip */
-#define CONFIG_ENC28J60
+#define CONFIG_ENC28J60_LPC2292
 
 #endif	/* __CONFIG_H */
#16 lore raw

PATCH 1/1 NET: move legacy enc28j60.c to sidetrack as enc28j60_lpc2292.c

Quoted 15 lines · Reinhard Meyer (-VC)

This patch is required before the upcoming new enc28j60 driver using SPI framework patch can be applied:
- Move legacy enc28j60.c to enc28j60_lpc2292.c.
- Change Makefile and the two affected boards' definition files.

Tested with ./MAKEALL ARM7 that both boards still compile.

Signed-off-by Reinhard Meyer<[email protected]>
---
  drivers/net/Makefile            |    2 +-
  drivers/net/enc28j60.c          |  982 --------------------------------------
  drivers/net/enc28j60_lpc2292.c  |  983 +++++++++++++++++++++++++++++++++++++++
  include/configs/SMN42.h         |    2 +-
  include/configs/lpc2292sodimm.h |    2 +-
  5 files changed, 986 insertions(+), 985 deletions(-)
Quoted 2 lines

delete mode 100644 drivers/net/enc28j60.c create mode 100644 drivers/net/enc28j60_lpc2292.c

Please use 'git mv' followed by 'git format-patch -C' when moving files. It's much easier to review.

regards,
Ben

author
#17 lore raw

PATCH v2 1/1 NET: move legacy enc28j60.c to sidetrack as enc28j60_lpc2292.c

This patch is required before the upcoming new enc28j60 driver using SPI framework patch can be applied:
- Move legacy enc28j60.c to enc28j60_lpc2292.c.
- Change Makefile and the two affected boards' definition files.

Tested with ./MAKEALL ARM7 that both boards still compile.

Signed-off-by Reinhard Meyer<[email protected]>

---
now with -C option on format-patch. Cheers!

 drivers/net/Makefile                           |    2 +-
 drivers/net/{enc28j60.c => enc28j60_lpc2292.c} |    1 +
 include/configs/SMN42.h                        |    2 +-
 include/configs/lpc2292sodimm.h                |    2 +-
 4 files changed, 4 insertions(+), 3 deletions(-)
 rename drivers/net/{enc28j60.c => enc28j60_lpc2292.c} (99%)
drivers/net/Makefile +1 -1
index 218eeff..479954d 100644
@@ -39,7 +39,7 @@ COBJS-$(CONFIG_DRIVER_DM9000) += dm9000x.o
 COBJS-$(CONFIG_DNET) += dnet.o
 COBJS-$(CONFIG_E1000) += e1000.o
 COBJS-$(CONFIG_EEPRO100) += eepro100.o
-COBJS-$(CONFIG_ENC28J60) += enc28j60.o
+COBJS-$(CONFIG_ENC28J60_LPC2292) += enc28j60_lpc2292.o
 COBJS-$(CONFIG_EP93XX) += ep93xx_eth.o
 COBJS-$(CONFIG_ETHOC) += ethoc.o
 COBJS-$(CONFIG_FEC_MXC) += fec_mxc.o
drivers/net/enc28j60_lpc2292.c renamed from drivers/net/enc28j60.c +1 -0
similarity index 99%
rename from drivers/net/enc28j60.c
rename to drivers/net/enc28j60_lpc2292.c
index 3238a50..bf95052 100644
@@ -15,6 +15,7 @@
  * MA 02111-1307 USA
  */
 
+#warning This driver is depreciated. Please update to new SPI framework enc28j60 driver
 #include <config.h>
 #include <common.h>
 #include <net.h>
include/configs/SMN42.h +1 -1
index adb6ac5..ba3ada1 100644
@@ -198,6 +198,6 @@
 #define CONFIG_INITRD_TAG
 #define CONFIG_MMC			1
 /* we use this ethernet chip */
-#define CONFIG_ENC28J60
+#define CONFIG_ENC28J60_LPC2292
 
 #endif	/* __CONFIG_H */
include/configs/lpc2292sodimm.h +1 -1
index 7ce8d6d..65276a2 100644
@@ -156,6 +156,6 @@
 #define CONFIG_INITRD_TAG
 #define CONFIG_MMC 1
 /* we use this ethernet chip */
-#define CONFIG_ENC28J60
+#define CONFIG_ENC28J60_LPC2292
 
 #endif	/* __CONFIG_H */
Applied
#18 lore raw

PATCH v2 1/1 NET: move legacy enc28j60.c to sidetrack as enc28j60_lpc2292.c

Hello Reinhard,

Quoted 9 lines · Reinhard Meyer (-VC)

This patch is required before the upcoming new enc28j60 driver using SPI framework patch can be applied:
- Move legacy enc28j60.c to enc28j60_lpc2292.c.
- Change Makefile and the two affected boards' definition files.

Tested with ./MAKEALL ARM7 that both boards still compile.

Signed-off-by Reinhard Meyer<[email protected]>

---

Applied to net/next

regards,
Ben

#19 lore raw

PATCH 1/1 NET: move legacy enc28j60.c to sidetrack as enc28j60_lpc2292.c

In reply to Ben Warren
Quoted 21 lines · On Friday, August 06, 2010 12:31:13 Ben Warren wrote:
Quoted 17 lines · Reinhard Meyer (-VC)

This patch is required before the upcoming new enc28j60 driver using SPI framework patch can be applied:
- Move legacy enc28j60.c to enc28j60_lpc2292.c.
- Change Makefile and the two affected boards' definition files.

Tested with ./MAKEALL ARM7 that both boards still compile.

Signed-off-by Reinhard Meyer<[email protected]>

---

  drivers/net/Makefile            |    2 +-
  drivers/net/enc28j60.c          |  982
  -------------------------------------- drivers/net/enc28j60_lpc2292.c 
  |  983 +++++++++++++++++++++++++++++++++++++++ include/configs/SMN42.h
          |    2 +-
  include/configs/lpc2292sodimm.h |    2 +-
  5 files changed, 986 insertions(+), 985 deletions(-)
Quoted 2 lines

delete mode 100644 drivers/net/enc28j60.c create mode 100644 drivers/net/enc28j60_lpc2292.c

Quoted 2 lines

Please use 'git mv' followed by 'git format-patch -C' when moving files. It's much easier to review.

-C should figure things out without needing git mv
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100806/ce3c91ef/attachment.pgp

author
#20 lore raw

PATCH 1/1 NET: move legacy enc28j60.c to sidetrack as enc28j60_lpc2292.c

Review

Quoted 7 lines · Mike Frysinger
Quoted 3 lines

Please use 'git mv' followed by 'git format-patch -C' when moving files. It's much easier to review.

(quoted diff) +0 -2
-C should figure things out without needing `git mv`
-mike

I had used "git mv" anyway, and "git status" showed it nicely. After "git format-patch master"
I was wondering why it became a delete/create action.

Reinhard