From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH 09/11] clk: lpc32xx: add common clock framework driver Date: Fri, 20 Nov 2015 21:20:26 +0100 Message-ID: <4933690.HhBPeGYMb3@wuerfel> References: <1447981511-29653-1-git-send-email-vz@mleia.com> <3762298.NKYXldtldq@wuerfel> <564F6172.3090509@mleia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Return-path: In-Reply-To: <564F6172.3090509@mleia.com> Sender: linux-clk-owner@vger.kernel.org To: Vladimir Zapolskiy Cc: linux-arm-kernel@lists.infradead.org, Rob Herring , Stephen Boyd , Michael Turquette , Roland Stigge , devicetree@vger.kernel.org, Russell King , linux-clk@vger.kernel.org List-Id: devicetree@vger.kernel.org On Friday 20 November 2015 20:07:46 Vladimir Zapolskiy wrote: > On 20.11.2015 16:04, Arnd Bergmann wrote: > > On Friday 20 November 2015 03:05:09 Vladimir Zapolskiy wrote: > >> + > >> +struct clk_proto_t { > >> + const char *name; > >> + const u8 parents[LPC32XX_CLK_PARENTS_MAX]; > >> + u8 num_parents; > >> + unsigned long flags; > >> +}; > >> + > >> +#define CLK_PREFIX(LITERAL) LPC32XX_CLK_ ## LITERAL > >> +#define NUMARGS(...) (sizeof((int[]){__VA_ARGS__})/sizeof(int)) > >> + > >> +#define LPC32XX_CLK_DEFINE(_idx, _name, _flags, ...) \ > >> + [CLK_PREFIX(_idx)] = { \ > >> + .name = #_name, \ > >> + .flags = _flags, \ > >> + .parents = { __VA_ARGS__ }, \ > >> + .num_parents = NUMARGS(__VA_ARGS__), \ > >> + } > >> + > > > > Try to not outsmart yourself with the macros. It's better to avoid > > string concatenation so it's possible to grep for uses of some > > constant. > > > > I would probably not use a macro at all here and just open-code the > > entire table. If you ensure that '0' is not a valid parent, then > > you can leave out the .num_parents field and just look for the > > zero-termination. > > Macros are here for simplicity, code size reduction and to avoid some > stupid mistakes like different number of .parents and .num_parents. > > I believe macro unwrapping in this code will add another 1000 LoC and > will result in quite unreadable and less maintainable code. I mean specifically the macro above: static const struct clk_proto_t clk_proto[LPC32XX_CLK_CCF_MAX] __initconst = { + LPC32XX_CLK_DEFINE(XTAL, xtal, 0x0), + LPC32XX_CLK_DEFINE(XTAL_32K, xtal_32k, 0x0), + + LPC32XX_CLK_DEFINE(RTC, rtc, 0x0, LPC32XX_CLK_XTAL_32K), + LPC32XX_CLK_DEFINE(OSC, osc, CLK_IGNORE_UNUSED, LPC32XX_CLK_XTAL), + LPC32XX_CLK_DEFINE(SYS, sys, CLK_IGNORE_UNUSED, + LPC32XX_CLK_OSC, LPC32XX_CLK_PLL397X), + LPC32XX_CLK_DEFINE(PLL397X, pll_397x, CLK_IGNORE_UNUSED, + LPC32XX_CLK_RTC), can become static const struct clk_proto_t clk_proto[] __initconst = { [LPC32XX_CLK_XTAL] = { "xtal" }, [LPC32XX_CLK_XTAL_32K] = { "xtal_32k" }, [LPC32XX_CLK_RTC] = { "rtc", .parents = { LPC32XX_CLK_XTAL_32K, 0 } }, [LPC32XX_CLK_OSC] = { "osc", CLK_IGNORE_UNUSED, .parents = { LPC32XX_CLK_XTAL, 0 } }, [LPC32XX_CLK_SYS] = { "sys", CLK_IGNORE_UNUSED, .parents = { LPC32XX_CLK_OSC, LPC32XX_CLK_PLL397X, 0) }, [LPC32XX_CLK_PLL397X] = { "pll_397x", CLK_IGNORE_UNUSED, .parents = { LPC32XX_CLK_RTC, 0 }, Not harder to read at all, not really longer, but easier to grep for. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Fri, 20 Nov 2015 21:20:26 +0100 Subject: [PATCH 09/11] clk: lpc32xx: add common clock framework driver In-Reply-To: <564F6172.3090509@mleia.com> References: <1447981511-29653-1-git-send-email-vz@mleia.com> <3762298.NKYXldtldq@wuerfel> <564F6172.3090509@mleia.com> Message-ID: <4933690.HhBPeGYMb3@wuerfel> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Friday 20 November 2015 20:07:46 Vladimir Zapolskiy wrote: > On 20.11.2015 16:04, Arnd Bergmann wrote: > > On Friday 20 November 2015 03:05:09 Vladimir Zapolskiy wrote: > >> + > >> +struct clk_proto_t { > >> + const char *name; > >> + const u8 parents[LPC32XX_CLK_PARENTS_MAX]; > >> + u8 num_parents; > >> + unsigned long flags; > >> +}; > >> + > >> +#define CLK_PREFIX(LITERAL) LPC32XX_CLK_ ## LITERAL > >> +#define NUMARGS(...) (sizeof((int[]){__VA_ARGS__})/sizeof(int)) > >> + > >> +#define LPC32XX_CLK_DEFINE(_idx, _name, _flags, ...) \ > >> + [CLK_PREFIX(_idx)] = { \ > >> + .name = #_name, \ > >> + .flags = _flags, \ > >> + .parents = { __VA_ARGS__ }, \ > >> + .num_parents = NUMARGS(__VA_ARGS__), \ > >> + } > >> + > > > > Try to not outsmart yourself with the macros. It's better to avoid > > string concatenation so it's possible to grep for uses of some > > constant. > > > > I would probably not use a macro at all here and just open-code the > > entire table. If you ensure that '0' is not a valid parent, then > > you can leave out the .num_parents field and just look for the > > zero-termination. > > Macros are here for simplicity, code size reduction and to avoid some > stupid mistakes like different number of .parents and .num_parents. > > I believe macro unwrapping in this code will add another 1000 LoC and > will result in quite unreadable and less maintainable code. I mean specifically the macro above: static const struct clk_proto_t clk_proto[LPC32XX_CLK_CCF_MAX] __initconst = { + LPC32XX_CLK_DEFINE(XTAL, xtal, 0x0), + LPC32XX_CLK_DEFINE(XTAL_32K, xtal_32k, 0x0), + + LPC32XX_CLK_DEFINE(RTC, rtc, 0x0, LPC32XX_CLK_XTAL_32K), + LPC32XX_CLK_DEFINE(OSC, osc, CLK_IGNORE_UNUSED, LPC32XX_CLK_XTAL), + LPC32XX_CLK_DEFINE(SYS, sys, CLK_IGNORE_UNUSED, + LPC32XX_CLK_OSC, LPC32XX_CLK_PLL397X), + LPC32XX_CLK_DEFINE(PLL397X, pll_397x, CLK_IGNORE_UNUSED, + LPC32XX_CLK_RTC), can become static const struct clk_proto_t clk_proto[] __initconst = { [LPC32XX_CLK_XTAL] = { "xtal" }, [LPC32XX_CLK_XTAL_32K] = { "xtal_32k" }, [LPC32XX_CLK_RTC] = { "rtc", .parents = { LPC32XX_CLK_XTAL_32K, 0 } }, [LPC32XX_CLK_OSC] = { "osc", CLK_IGNORE_UNUSED, .parents = { LPC32XX_CLK_XTAL, 0 } }, [LPC32XX_CLK_SYS] = { "sys", CLK_IGNORE_UNUSED, .parents = { LPC32XX_CLK_OSC, LPC32XX_CLK_PLL397X, 0) }, [LPC32XX_CLK_PLL397X] = { "pll_397x", CLK_IGNORE_UNUSED, .parents = { LPC32XX_CLK_RTC, 0 }, Not harder to read at all, not really longer, but easier to grep for. Arnd