From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Wed, 22 Feb 2017 19:23:10 -0700 Subject: [U-Boot] [PATCH 3/7] clk: rockchip: add support for rk3328 In-Reply-To: <58AD5D9F.7020104@rock-chips.com> References: <1487318878-23597-1-git-send-email-kever.yang@rock-chips.com> <1487318878-23597-4-git-send-email-kever.yang@rock-chips.com> <58AD5D9F.7020104@rock-chips.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de +Tom Hi Kever, On 22 February 2017 at 02:45, Kever Yang wrote: > > Hi Simon, > > > On 02/22/2017 02:06 AM, Simon Glass wrote: >> >> Hi Kever, >> >> On 17 February 2017 at 01:07, Kever Yang wrote: >>> >>> Add rk3328 clock driver and cru structure definition. >>> >>> Signed-off-by: William Zhang >>> Signed-off-by: Kever Yang >>> --- >>> >>> arch/arm/include/asm/arch-rockchip/cru_rk3328.h | 65 +++ >>> drivers/clk/rockchip/Makefile | 1 + >>> drivers/clk/rockchip/clk_rk3328.c | 607 ++++++++++++++++++++++++ >>> 3 files changed, 673 insertions(+) >>> create mode 100644 arch/arm/include/asm/arch-rockchip/cru_rk3328.h >>> create mode 100644 drivers/clk/rockchip/clk_rk3328.c >>> >> [..] >> >>> + >>> +#define I2C_CLK_REG_MASK(bus) \ >>> + (CLK_I2C_DIV_CON_MASK << \ >>> + CLK_I2C ##bus## _DIV_CON_SHIFT | \ >>> + CLK_I2C_PLL_SEL_MASK << \ >>> + CLK_I2C ##bus## _PLL_SEL_SHIFT) >>> + >>> +#define (bus, clk_div) \ >>> + ((clk_div - 1) << \ >>> + CLK_I2C ##bus## _DIV_CON_SHIFT | \ >>> + CLK_I2C_PLL_SEL_GPLL << \ >>> + CLK_I2C ##bus## _PLL_SEL_SHIFT) >>> + >>> +#define I2C_CLK_DIV_VALUE(con, bus) \ >>> + (con >> CLK_I2C ##bus## _DIV_CON_SHIFT) & \ >>> + CLK_I2C_DIV_CON_MASK; >> >> Can we drop these three and instead write them out below? > > > I don't know why you don't like this kind of MACRO, like the size_mb in sdram driver, > we though this help people understand the C source and make the C source cold looks much clean, > in some platform, maintainer may ask for this when there are multi controller and can reuse > the same MACRO. > Anyway, I will make this fallback to normal shift/mask style in next version. > Let me try to explain this. In this code macros are created by pasting symbols together which means (for example) that it is not possible to find the definition of CLK_I2C 0_DIV_CON_SHIFT by searching the code. This can be very confusing for people trying to figure out what is going on. It is bad enough in a single file but gets worse when things migrate to header files. If you want to have a macro here, how about #define CLK_I2C DIV_CON_SHIFT(bus) instead? This can return the appropriate shift for the bus. The 'I2C_CLK_DIV_VALUE' macro is only used in rk3328_i2c_get_clk() and ends up creating repetitive code (all the macro logic happens in each case of the switch() , which the compiler hopefully can optimize, but who knows? In any case conceptually I do not think it simplifies the code. You end up unpicking the macros and printing out intermediate values, etc. I have definitely done my share of that. The rule of using shifts and masks in the code rather than hiding them originally came from Wolfgang and I have got used to it. One thing I have noticed is that with macros you can build up a tree of interdependent macros such that it is very hard to figure out what is actually going on, e.g. if you are looking for a bug. I am aware that the code style in coreboot and UEFI are different, although I am not an expert in either. Does that make sense? Regards, Simon