Hello Alexey,
Am 29.01.2014 09:57, schrieb Alexey Brodkin:
Hello Heiko,
On Wed, 2014-01-29 at 06:44 +0100, Heiko Schocher wrote:
Hello Alexey,
Thanks for your patches, more or less just nitpicking comments ...
Thanks for prompt review.
Am 29.01.2014 00:09, schrieb Alexey Brodkin:
Signed-off-by: Alexey Brodkin<abrod...@synopsys.com>
No commit message, please add one. (Are this files from the Linux
kernel? If so please add a comment in the commit message + add a
hint which linux commit you used, thanks!)
I thought from commit subject it's already clear what's presented in
each particular patch so I left commit messages empty.
Frankly I'm not sure still what info may I put in commit messages except
mention of headers borrowed from Linux - or this is exactly what is
needed?
Maybe this site helps you:
http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign
Agree I forgot to mention which header files came from Linux kernel.
Will add mentions.
diff --git a/arch/arc/include/asm/arch-arc700/hardware.h
b/arch/arc/include/asm/arch-arc700/hardware.h
new file mode 100644
index 0000000..e69de29
Empty file ?
Right, it looks weird, but I had to add this empty file just because of
"designware_i2c" driver which refers to "asm/arch/hardware.h".
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/designware_i2c.c;h=9ed929521a8944dc870fc2eff546507632b6e86a;hb=HEAD
And to remove "asm/arch/hardware.h" I would need to modify
"arch/hardware.h" and "include/configs/..." files for platform/boards I
don't own.
Basically this is just a work-around that allows me to use
"designware_i2c" driver as it is.
There was a similar dependency ("asm/arch/clk.h") in "dw_mmc" but in
that case it was possible to just remove it - what I did -
http://git.denx.de/?p=u-boot.git;a=commit;h=ca6d4d0f8f0fb8ae09a7ba271177367bdfdf3136
So if you insist on removal of this file we would need to fix
"designware_i2c" first.
Please let me know what do you think about this item.
Hmm.. at least you should an comment in this file, why it is necessary.
diff --git a/arch/arc/include/asm/arcregs.h b/arch/arc/include/asm/arcregs.h
new file mode 100644
index 0000000..87b0a60
...
+
+#ifndef __ASSEMBLY__
+
+/*
+ ******************************************************************
Bad comment style
+ * Inline ASM macros to read/write AUX Regs
+ * Essentially invocation of lr/sr insns from "C"
+ */
+
+#if 1
+
+#define read_aux_reg(reg) __builtin_arc_lr(reg)
+
+/* gcc builtin sr needs reg param to be long immediate */
+#define write_aux_reg(reg_immed, val) \
+ __builtin_arc_sr((unsigned int)val, reg_immed)
+
+#else
Please remove dead code ...
+
+#define read_aux_reg(reg) \
...
+ bogus_undefined(); \
+ } \
+}
Why do you use defines here instead of real functions?
+
+#define WRITE_BCR(reg, into) \
+{ \
+ unsigned int tmp; \
+ if (sizeof(tmp) == sizeof(into)) { \
+ tmp = (*(unsigned int *)(into)); \
+ write_aux_reg(reg, tmp); \
+ } else { \
+ extern void bogus_undefined(void); \
+ bogus_undefined(); \
+ } \
+}
and here?
Ok, so this header is borrowed from Linux sources. That's why I didn't
do any modifications and took it as it is on kernel.org.
Ah, ok, so this is ok I think.
diff --git a/arch/arc/include/asm/processor.h b/arch/arc/include/asm/processor.h
new file mode 100644
index 0000000..e69de29
Hups, one more empty file ...
I thought it was required by some common file. Double-checked and now
see that it could be safely removed.
Great!
diff --git a/arch/arc/include/asm/ptrace.h b/arch/arc/include/asm/ptrace.h
new file mode 100644
index 0000000..3b2df87
--- /dev/null
+++ b/arch/arc/include/asm/ptrace.h
@@ -0,0 +1,101 @@
+/*
+ * Copyright (C) 2004, 2007-2010, 2011-2012 Synopsys, Inc. All rights reserved.
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ */
+
+#ifndef __ASM_ARC_PTRACE_H
+#define __ASM_ARC_PTRACE_H
+
+#ifndef __ASSEMBLY__
+
+/* THE pt_regs: Defines how regs are saved during entry into kernel */
Is the "THE" a shortcut?
Another copy from Linux sources - thus taken as it is.
diff --git a/arch/arc/include/asm/string.h b/arch/arc/include/asm/string.h
new file mode 100644
index 0000000..e69de29
empty file
Somehow I missed a fact that ARC already has optimized "string"
routines. Will add them in re-spin.
Will send a v2 series soon.
Ok, hope to find time for your other patches ...
bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
