| Message ID | 20110822213742.28083.69514.stgit@shuttle2.etheralp.ch |
|---|---|
| State | Changes Requested |
| Headers | show |
On Monday, August 22, 2011 17:37:42 Eric Jarrige wrote:
> +#if (!defined(CONFIG_DM9000_NO_SROM) && defined(CONFIG_DRIVER_DM9000))
why is this necessary ?
-mike
Hi Mike, On 22 août 2011, at 23:55, Mike Frysinger wrote: > On Monday, August 22, 2011 17:37:42 Eric Jarrige wrote: >> +#if (!defined(CONFIG_DM9000_NO_SROM) && defined(CONFIG_DRIVER_DM9000)) > > why is this necessary ? > -mike the methods dm9000_read_srom_word()/dm9000_write_srom_word() need these conditions to be compiled in dm9000x.c Therefore this avoid dead code and/or compilation failure if these conditions are not met. Best regards, Eric
On 08/22/2011 11:37 PM, Eric Jarrige wrote: > Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org> > Signed-off-by: Stefano Babic <sbabic@denx.de> > CC: Ben Warren <biggerbadderben@gmail.com> I think we should drop Ben's name as maintainer for the network because he retired some times ago, but his name is still in the maintainer list. Wolfgang is the maintainer at the moment, I added him in CC. > --- > README | 1 + > common/Makefile | 1 + > common/cmd_dm9000ee.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 86 insertions(+), 0 deletions(-) > create mode 100644 common/cmd_dm9000ee.c Why do not we add this code simply to the DM9000 driver ? I think it belongs to the driver and that is the right place to put this stuff. Best regards, Stefano Babic
On Tuesday, August 23, 2011 01:15:25 Eric Jarrige wrote: > On 22 août 2011, at 23:55, Mike Frysinger wrote: > > On Monday, August 22, 2011 17:37:42 Eric Jarrige wrote: > >> +#if (!defined(CONFIG_DM9000_NO_SROM) && defined(CONFIG_DRIVER_DM9000)) > > > > why is this necessary ? > > the methods dm9000_read_srom_word()/dm9000_write_srom_word() need these > conditions to be compiled in dm9000x.c Therefore this avoid dead code > and/or compilation failure if these conditions are not met. my point is that it is user error to select CONFIG_CMD_DM9000EE but not select the DM9000 settings. in your case, the user silently gets a build that makes no sense. in my case, they're forced to fix things. -mike
On 08/23/2011 03:42 PM, Mike Frysinger wrote: > On Tuesday, August 23, 2011 01:15:25 Eric Jarrige wrote: >> On 22 août 2011, at 23:55, Mike Frysinger wrote: >>> On Monday, August 22, 2011 17:37:42 Eric Jarrige wrote: >>>> +#if (!defined(CONFIG_DM9000_NO_SROM) && defined(CONFIG_DRIVER_DM9000)) >>> >>> why is this necessary ? >> >> the methods dm9000_read_srom_word()/dm9000_write_srom_word() need these >> conditions to be compiled in dm9000x.c Therefore this avoid dead code >> and/or compilation failure if these conditions are not met. > > my point is that it is user error to select CONFIG_CMD_DM9000EE but not select > the DM9000 settings. in your case, the user silently gets a build that makes > no sense. in my case, they're forced to fix things. Agree. This is also my point to move this code inside the driver itself, where CONFIG_DRIVER_DM9000 is set (or it is not compiled). Best regards, Stefano Babic
On 23 août 2011, at 16:21, Stefano Babic wrote: > On 08/23/2011 03:42 PM, Mike Frysinger wrote: >> On Tuesday, August 23, 2011 01:15:25 Eric Jarrige wrote: >>> On 22 août 2011, at 23:55, Mike Frysinger wrote: >>>> On Monday, August 22, 2011 17:37:42 Eric Jarrige wrote: >>>>> +#if (!defined(CONFIG_DM9000_NO_SROM) && defined(CONFIG_DRIVER_DM9000)) >>>> >>>> why is this necessary ? >>> >>> the methods dm9000_read_srom_word()/dm9000_write_srom_word() need these >>> conditions to be compiled in dm9000x.c Therefore this avoid dead code >>> and/or compilation failure if these conditions are not met. >> >> my point is that it is user error to select CONFIG_CMD_DM9000EE but not select >> the DM9000 settings. in your case, the user silently gets a build that makes >> no sense. in my case, they're forced to fix things. > > Agree. This is also my point to move this code inside the driver itself, > where CONFIG_DRIVER_DM9000 is set (or it is not compiled). > That makes sense - I can remove this line. Best regards, Eric
Dear Eric Jarrige, In message <20110822213742.28083.69514.stgit@shuttle2.etheralp.ch> you wrote: > Signed-off-by: Eric Jarrige <eric.jarrige@armadeus.org> > Signed-off-by: Stefano Babic <sbabic@denx.de> > CC: Ben Warren <biggerbadderben@gmail.com> > --- > README | 1 + > common/Makefile | 1 + > common/cmd_dm9000ee.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 86 insertions(+), 0 deletions(-) > create mode 100644 common/cmd_dm9000ee.c Checkpatch says: total: 0 errors, 2 warnings, 98 lines checked Please clean up and resubmit. Thanks. Best regards, Wolfgang Denk
diff --git a/README b/README index 0886987..c6981bf 100644 --- a/README +++ b/README @@ -707,6 +707,7 @@ The following options need to be configured: CONFIG_CMD_DATE * support for RTC, date/time... CONFIG_CMD_DHCP * DHCP support CONFIG_CMD_DIAG * Diagnostics + CONFIG_CMD_DM9000EE DM9000 external EEPROM support CONFIG_CMD_DS4510 * ds4510 I2C gpio commands CONFIG_CMD_DS4510_INFO * ds4510 I2C info command CONFIG_CMD_DS4510_MEM * ds4510 I2C eeprom/sram commansd diff --git a/common/Makefile b/common/Makefile index d662468..d22cbac 100644 --- a/common/Makefile +++ b/common/Makefile @@ -81,6 +81,7 @@ ifdef CONFIG_POST COBJS-$(CONFIG_CMD_DIAG) += cmd_diag.o endif COBJS-$(CONFIG_CMD_DISPLAY) += cmd_display.o +COBJS-$(CONFIG_CMD_DM9000EE) += cmd_dm9000ee.o COBJS-$(CONFIG_CMD_DTT) += cmd_dtt.o COBJS-$(CONFIG_CMD_ECHO) += cmd_echo.o COBJS-$(CONFIG_ENV_IS_IN_EEPROM) += cmd_eeprom.o diff --git a/common/cmd_dm9000ee.c b/common/cmd_dm9000ee.c new file mode 100644 index 0000000..0d53c70 --- /dev/null +++ b/common/cmd_dm9000ee.c @@ -0,0 +1,84 @@ +/* + * (C) Copyright 2008-2011 Armadeus Project + * (C) Copyright 2007 + * Stefano Babic, DENX Software Engineering, sbabic@denx.de. + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#include <common.h> +#include <command.h> +#include <dm9000.h> + +#if (!defined(CONFIG_DM9000_NO_SROM) && defined(CONFIG_DRIVER_DM9000)) + +static int do_read_dm9000_eeprom(cmd_tbl_t *cmdtp, int flag, int argc, + char *const argv[]) +{ + unsigned int i; + u8 data[2]; + + for (i = 0; i < 0x40; i++) { + if (!(i % 0x10)) + printf("\n%08x:", i); + dm9000_read_srom_word(i, data); + printf(" %02x%02x", data[1], data[0]); + } + printf("\n"); + return 0; +} + +static int do_write_dm9000_eeprom(cmd_tbl_t *cmdtp, int flag, int argc, + char *const argv[]) +{ + unsigned long offset, value; + + if (argc < 4) + return cmd_usage(cmdtp); + + strict_strtoul(argv[2], 16, &offset); + strict_strtoul(argv[3], 16, &value); + if (offset > 0x40) { + printf("Wrong offset : 0x%lx\n", offset); + return cmd_usage(cmdtp); + } + dm9000_write_srom_word(offset, value); + return 0; +} + +int do_dm9000_eeprom(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[]) +{ + if (argc < 2) + return cmd_usage(cmdtp); + + if (strcmp(argv[1], "read") == 0) + return do_read_dm9000_eeprom(cmdtp, flag, argc, argv); + else if (strcmp(argv[1], "write") == 0) + return do_write_dm9000_eeprom(cmdtp, flag, argc, argv); + else + return cmd_usage(cmdtp); +} + +U_BOOT_CMD(dm9000ee, 4, 1, do_dm9000_eeprom, + "Read/Write eeprom connected to Ethernet Controller", + "\ndm9000ee write <word offset> <value> \n" + "\tdm9000ee read \n" + "\tword:\t\t00-02 : MAC Address\n" + "\t\t\t03-07 : DM9000 Configuration\n" "\t\t\t08-63 : User data"); +#endif /* !CONFIG_DM9000_NO_SROM && CONFIG_DRIVER_DM9000 */