linux-kernel - Re: [PATCH v5 19/44] clk: davinci: New driver for TI DA8XX CFGCHIP clocks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <19f2830e-6d57-ebd9-1873-f79bea3e8439@lechnology.com>
Date:   Wed, 17 Jan 2018 11:35:54 -0600
From:   David Lechner <david@...hnology.com>
To:     Sekhar Nori <nsekhar@...com>, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Cc:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Kevin Hilman <khilman@...nel.org>,
        Adam Ford <aford173@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 19/44] clk: davinci: New driver for TI DA8XX CFGCHIP
 clocks

On 01/17/2018 09:31 AM, Sekhar Nori wrote:
> On Monday 08 January 2018 07:47 AM, David Lechner wrote:
>> This adds a new driver for the gate and multiplexer clocks in the
>> CFGCHIPn syscon registers on TI DA8XX-type SoCs.
>>
>> Signed-off-by: David Lechner <david@...hnology.com>
>> ---
>>   drivers/clk/davinci/Makefile        |   2 +
>>   drivers/clk/davinci/da8xx-cfgchip.c | 203 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 205 insertions(+)
>>   create mode 100644 drivers/clk/davinci/da8xx-cfgchip.c
>>
>> diff --git a/drivers/clk/davinci/Makefile b/drivers/clk/davinci/Makefile
>> index 6c388d4..11178b7 100644
>> --- a/drivers/clk/davinci/Makefile
>> +++ b/drivers/clk/davinci/Makefile
>> @@ -1,6 +1,8 @@
>>   # SPDX-License-Identifier: GPL-2.0
>>   
>>   ifeq ($(CONFIG_COMMON_CLK), y)
>> +obj-$(CONFIG_ARCH_DAVINCI_DA8XX)	+= da8xx-cfgchip.o
>> +
>>   obj-y += pll.o
>>   obj-$(CONFIG_ARCH_DAVINCI_DA830)	+= pll-da830.o
>>   obj-$(CONFIG_ARCH_DAVINCI_DA850)	+= pll-da850.o
>> diff --git a/drivers/clk/davinci/da8xx-cfgchip.c b/drivers/clk/davinci/da8xx-cfgchip.c
>> new file mode 100644
>> index 0000000..772e09a
>> --- /dev/null
>> +++ b/drivers/clk/davinci/da8xx-cfgchip.c
>> @@ -0,0 +1,203 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Clock driver for DA8xx/AM17xx/AM18xx/OMAP-L13x CFGCHIP
>> + *
>> + * Copyright (C) 2017 David Lechner <david@...hnology.com>
> 
> 2018
> 
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/mfd/da8xx-cfgchip.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/regmap.h>
>> +#include <linux/slab.h>
>> +
>> +#ifdef CONFIG_OF
> 
> Is this ifdef really needed, or included to save space for non-OF
> builds? I think it can be removed if not really needed.
> 
>> +static void da8xx_cfgchip_gate_clk_init(struct device_node *np, u32 reg,
>> +					u32 mask)
>> +{
>> +	struct da8xx_cfgchip_gate_clk *clk;
>> +	struct clk_init_data init;
>> +	const char *name = np->name;
>> +	const char *parent_name;
>> +	struct regmap *regmap;
>> +	int ret;
>> +
>> +	of_property_read_string(np, "clock-output-names", &name);
>> +	parent_name = of_clk_get_parent_name(np, 0);
>> +
>> +	regmap = syscon_node_to_regmap(of_get_parent(np));
>> +	if (IS_ERR(regmap)) {
>> +		pr_err("%s: no regmap for syscon parent of %s (%ld)\n",
>> +		       __func__, np->full_name, PTR_ERR(regmap));
> 
> please use pr_fmt for this driver too.
> 
>> +static void da8xx_cfgchip_mux_clk_init(struct device_node *np, u32 reg,
>> +				       u32 mask)
>> +{
>> +	struct da8xx_cfgchip_mux_clk *clk;
>> +	struct clk_init_data init;
>> +	const char *name = np->name;
>> +	const char *parent_names[2];
>> +	struct regmap *regmap;
>> +	int ret;
>> +
>> +	ret = of_property_match_string(np, "clock-names", "pll0_sysclk2");
>> +	parent_names[0] = of_clk_get_parent_name(np, ret);
>> +	if (!parent_names[0]) {
>> +		pr_err("%s: missing pll0_sysclk2 clock\n", __func__);
>> +		return;
>> +	}
>> +
>> +	ret = of_property_match_string(np, "clock-names", "pll1_sysclk2");
>> +	parent_names[1] = of_clk_get_parent_name(np, ret);
>> +	if (!parent_names[1]) {
>> +		pr_err("%s: missing pll1_sysclk2 clock\n", __func__);
>> +		return;
>> +	}
> 
> The fact that you are looking specifically for pll0_sysclk2 and
> pll1_sysclk2 makes it really specific to async3 and the same function
> cannot be used for something like EMIFA clock source. Can this part of
> the function be factored out so rest of the function can still be reused
> for another clock?
> 

Already fixed this stuff a couple days ago. ;-)

And I went ahead and added the EMIFA clocks (the mux and the DIV4.5)