[PATCH 2/3] ARM: OMAP2+: Add support to parse optional clk info from DT

[Mark Rutland]

[Adding Mike Turquette and dt maintainers to Cc]

On Tue, Jul 23, 2013 at 07:24:38AM +0100, Rajendra Nayak wrote:
> With clocks for OMAP moving to DT, its now possible to pass all optional clock
> data for each device from DT instead of having it in hwmod.
> 
> Signed-off-by: Rajendra Nayak <rnayak at ti.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c |   66 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 12fa589..e5c804b 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -805,6 +805,65 @@ static int _init_interface_clks(struct omap_hwmod *oh)
>  	return ret;
>  }
>  
> +static const char **_parse_opt_clks_dt(struct omap_hwmod *oh,
> +				       struct device_node *np,
> +				       int *opt_clks_cnt)
> +{
> +	int i, clks_cnt;
> +	const char *clk_name;
> +	const char **opt_clk_names;
> +
> +	clks_cnt = of_property_count_strings(np, "clock-names");
> +	if (!clks_cnt)
> +		return NULL;
> +
> +	opt_clk_names = kzalloc(sizeof(char *)*clks_cnt, GFP_KERNEL);
> +	if (!opt_clk_names)
> +		return NULL;
> +
> +	for (i = 0; i < clks_cnt; i++) {
> +		of_property_read_string_index(np, "clock-names", i, &clk_name);
> +		if (!strcmp(clk_name, "fck"))
> +			continue;
> +		opt_clks_cnt++;
> +		opt_clk_names[i] = clk_name;
> +	}
> +	return opt_clk_names;
> +}
> +
> +static int _init_opt_clks_dt(struct omap_hwmod *oh, struct device_node *np)
> +{
> +	struct clk *c;
> +	int i, opt_clks_cnt = 0;
> +	int ret = 0;
> +	const char **opt_clk_names;
> +
> +	opt_clk_names = _parse_opt_clks_dt(oh, np, &opt_clks_cnt);
> +	if (!opt_clk_names)
> +		return -EINVAL;
> +
> +	oh->opt_clks = kzalloc(sizeof(struct omap_hwmod_opt_clk *)
> +			       * opt_clks_cnt, GFP_KERNEL);
> +	if (!oh->opt_clks)
> +		return -ENOMEM;
> +
> +	oh->opt_clks_cnt = opt_clks_cnt;
> +
> +	for (i = 0; i < oh->opt_clks_cnt; i++) {
> +		c = of_clk_get_by_name(np, opt_clk_names[i]);
> +		if (IS_ERR(c)) {
> +			pr_warn("omap_hwmod: %s: cannot clk_get opt_clk %s\n",
> +				oh->name, opt_clk_names[i]);
> +			ret = -EINVAL;
> +		}
> +		oh->opt_clks[i]._clk = c;
> +		oh->opt_clks[i].role = opt_clk_names[i];
> +		oh->opt_clks_cnt++;
> +		clk_prepare(oh->opt_clks[i]._clk);
> +	}
> +	return ret;
> +}

I don't like this. 

clock-names is used to represent the names of clocks as inputs to the
device. The driver must know the names of each and every one of the
clock inputs it intends to use -- there's a finite number, and if it
doesn't know about it it clearly has no idea how that clock's meant to
be used.

Consider a future revision of the hardware that has an additional clock
input. Some new feature may require that clock, but your driver won't
support that new feature, so you don't need it. Preparing that clock is
a waste of power, and could cause issues if for some reason the clock
was mutually exlcusive with another clock (so preparing it would make
the hardware unusable). If the new revision *requires* that clock to
provide the same interface otherwise, it's not backwards compatible and
needs a new binding, and the driver needs to be extended to support it.

Given that, preparing all the clocks you've been handed is a hack.
Simply request by name the ones you know you need, and attempt to
request the optional ones as necessary. Don't blindly go and prepare
everything.

Thanks,
Mark.