Re: [Openocd-development] [PATCH] updated Fujitsu FM3 Flash driver

2011-10-01 Thread Øyvind Harboe
Hi,

some comments:

1. please fix warning:

fm3.c: In function ‘fm3_chip_erase’:
fm3.c:785:25: error: declaration of ‘fm3_info’ shadows a global declaration
fm3.c:774:12: error: shadowed declaration is here


2. Remove this history, git keeps it:

+// History:
+// 2011-07-13 MWi First version
+// 2011-09-27 MWi Flash type 2 added, algorithm start address now relocateable


-- 
Øyvind Harboe - Can Zylin Consulting help on your project?
US toll free 1-866-980-3434
http://www.zylin.com/
___
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development


Re: [Openocd-development] [PATCH] updated Fujitsu FM3 Flash driver

2011-10-01 Thread Peter Stuge
openOCD.fseu wrote:
 please find attached the updated Fujistu CORTEX M3 NOR Flash driver.
 It includes:
 - Improvement of error handling of retval = target_read/write_xxx();

Great to get better error handling! All the repeated retval=..if()
return look absolutely horrible but that's indeed not your fault.

We should maybe have some handy macros for this in shared code, to
make drivers prettier and simpler.


 - No #if statements anymore
 - Added Flash type 2 support

Great.


Content-Description: 
0001-Updated-fm3.c-added-Flash-type-2-support-error-handl.patch
 --- a/src/flash/nor/fm3.c
 +++ b/src/flash/nor/fm3.c
 @@ -1,23 +1,28 @@
 -/***
 - *   Copyright (C) 2011 by Marc Willam, Holger Wech*
 - *   m.wil...@gmx.eu   *
 - *   Copyright (C) 2011 Ronny Strutz   *
 - * *
 - *   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. *
 - ***/
 +/*
 + *   Copyright (C) 2011 by Marc Willam, Holger Wech  
 *
 + *   openOCD.fseu(AT)de.fujitsu.com  
 *
 + *   Copyright (C) 2011 Ronny Strutz 
 *
 + *   
 *
 + *   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.   
 *
 + 
 */

Please avoid all whitespace-only changes. Perhaps especially so in
the license text for a file.


 + 
 +// History:
 +// 2011-07-13 MWi First version
 +// 2011-09-27 MWi Flash type 2 added, algorithm start address now 
 relocateable
 +

As Øyvind pointed out this is redundant, since git tracks history
already, and with good detail.


  enum fm3_variant
  {
 - mb9bfxx1,
 + mb9bfxx1,   /* Flash Type '1' */
   mb9bfxx2,
   mb9bfxx3,
   mb9bfxx4,
   mb9bfxx5,
 - mb9bfxx6
 + mb9bfxx6,
 + mb9afxx1,   /* Flash Type '2' */
 + mb9afxx2,
 + mb9afxx3,
 + mb9afxx4,
 + mb9afxx5,
 + mb9afxx6
 +};

It would help to have a separate patch that adds the comma to the
last entry in the enum, followed by another patch that adds the new
variants, making sure to include a comma after the last new entry,
and putting the two comments on separate lines instead of together
with actual values, so that in the future no lines will need to
change unless there is an actual code change. Changing existing lines
creates unneccessary noise in the source code