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? 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. > > 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot