Re: [Patch v2] driver/clk/clk-si5338: Add common clock framework driver for si5338

[York Sun <yorksun@freescale.com>]

Paul,

Thanks for reviewing.

On 06/16/2015 01:21 AM, Paul Bolle wrote:
> One question and a few nits follow.
> 
> On Mon, 2015-06-15 at 10:07 -0700, York Sun wrote:
>> SI5338 is a programmable clock generator. It has 4 sets of inputs,
>> PLL, multisynth and dividers to make 4 outputs. This driver splits
>> them into multiple clocks to comply with common clock framework.
>>
>> See Documentation/devicetree/bindings/clock/silabs,si5338.txt for
>> details.
>>
>> Signed-off-by: York Sun <yorksun@freescale.com>
>> CC: Mike Turquette <mturquette@linaro.org>
> 
> Apparently that's now mturquette@baylibre.com .

Thanks. Will change.

> 
>> CC: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
>> CC: Guenter Roeck <linux@roeck-us.net>
>> CC: Andrey Filippov <andrey@elphel.com>
> 
>> --- a/drivers/clk/Kconfig
>> +++ b/drivers/clk/Kconfig
> 
>>  config COMMON_CLK
>> -	bool
>> +	tristate "Common Clock"
>>  	select HAVE_CLK_PREPARE
>>  	select CLKDEV_LOOKUP
>>  	select SRCU
> 
> Why? The commit explanation doesn't mention this. Did you use an unclean
> tree? If not, you just created over a dozen of new modules:

Thanks for catching this. I was testing building the driver within and outside
of kernel tree for another kernel version. If this driver is built-in, I don't
need to make it tristate. Will revert in next version.

>     $ git grep -nw CONFIG_COMMON_CLK -- "*Makefile*"
>     arch/arm/mach-mmp/Makefile:13:ifeq ($(CONFIG_COMMON_CLK), )
>     arch/arm/mach-shmobile/Makefile:21:ifndef CONFIG_COMMON_CLK
>     arch/powerpc/platforms/512x/Makefile:4:obj-$(CONFIG_COMMON_CLK) += clock-commonclk.o
>     drivers/clk/Makefile:4:obj-$(CONFIG_COMMON_CLK) += clk.o
>     drivers/clk/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk-divider.o
>     drivers/clk/Makefile:6:obj-$(CONFIG_COMMON_CLK) += clk-fixed-factor.o
>     drivers/clk/Makefile:7:obj-$(CONFIG_COMMON_CLK) += clk-fixed-rate.o
>     drivers/clk/Makefile:8:obj-$(CONFIG_COMMON_CLK) += clk-gate.o
>     drivers/clk/Makefile:9:obj-$(CONFIG_COMMON_CLK) += clk-mux.o
>     drivers/clk/Makefile:10:obj-$(CONFIG_COMMON_CLK)        += clk-composite.o
>     drivers/clk/Makefile:11:obj-$(CONFIG_COMMON_CLK)        += clk-fractional-divider.o
>     drivers/clk/Makefile:12:obj-$(CONFIG_COMMON_CLK)        += clk-gpio-gate.o
>     drivers/clk/Makefile:14:obj-$(CONFIG_COMMON_CLK)        += clk-conf.o
>     drivers/clk/Makefile:59:ifeq ($(CONFIG_COMMON_CLK), y)
>     drivers/clk/samsung/Makefile:5:obj-$(CONFIG_COMMON_CLK) += clk.o clk-pll.o
>     drivers/gpu/drm/msm/Makefile:53:msm-$(CONFIG_COMMON_CLK) += mdp/mdp4/mdp4_lvds_pll.o
>     drivers/sh/Makefile:5:ifneq ($(CONFIG_COMMON_CLK),y)
> 
>> +config COMMON_CLK_SI5338
>> +	tristate "Clock driver for SiLabs 5338"
>> +	depends on I2C
>> +	select REGMAP_I2C
>> +	select RATIONAL
>> +	---help---
>> +	  This driver supports Silicon Labs 5338 programmable clock generators,
>> +	  using common clock framework. It needs parent clock as input(s).
>> +	  Internal clocks are registered with unique names in case multiple
>> +	  devices exist. See devicetree/bindings/clock/silabs,si5338.txt
>> +	  under Documentation for details.
> 
>> --- /dev/null
>> +++ b/drivers/clk/clk-si5338.c
> 
>> +unsigned long si5338_divrefclk_recalc_rate(struct clk_hw *hw,
>> +					   unsigned long parent_rate)
>> +{
>> +	[...]
>> +}
> 
> Can't this be made static? It compiles cleanly with static too. Is there
> some subtle issue I'm missing?
> 

Absolutely. I must have missed them for some functions.


>> +static const struct clk_ops si5338_divrefclk_ops = {
>> +	.recalc_rate = si5338_divrefclk_recalc_rate,
>> +};
> 
>> +unsigned long si5338_divfbclk_recalc_rate(struct clk_hw *hw,
>> +					   unsigned long parent_rate)
>> +{
>> +	[...]
>> +}
> 
> Ditto.
> 
>> +static const struct clk_ops si5338_divfbclk_ops = {
>> +	.recalc_rate = si5338_divfbclk_recalc_rate,
>> +};
> 
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/clk-si5338.h
> 
>> +#ifndef _DT_BINDINGS_CLK_DSI5338_H
>> +#define _DT_BINDINGS_CLK_DSI5338_H
> 
> (I spotted a D that looks odd here.)

Me, too. It takes fresh eyes to spot this non-sense error.

> 
> And git am whines:
>     new blank line at EOF.

Thanks.

York