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 Brodkinabrodkin@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?
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=9ed...
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=ca6d4d0f8f0fb8ae09a7ba271177367b...
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.
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.
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.
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.
-Alexey