linux-kernel - Re: [PATCH v7 4/5] clk: RK808: Add clkout driver for RK808

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: <20140901215555.5251.33484@quantum>
Date:	Mon, 01 Sep 2014 14:55:55 -0700
From:	Mike Turquette <mturquette@...aro.org>
To:	Chris Zhong <zyw@...k-chips.com>, robh+dt@...nel.org,
	pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	sameo@...ux.intel.com, lee.jones@...aro.org, lgirdwood@...il.com,
	a.zummo@...ertech.it
Cc:	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	rtc-linux@...glegroups.com, grant.likely@...aro.org,
	hl@...k-chips.com, huangtao@...k-chips.com, cf@...k-chips.com,
	zhangqing@...k-chips.com, xxx@...k-chips.com,
	dianders@...omium.org, heiko@...ech.de, olof@...om.net,
	sonnyrao@...omium.org, dtor@...omium.org,
	javier.martinez@...labora.co.uk, kever.yang@...k-chips.com,
	"Chris Zhong" <zyw@...k-chips.com>
Subject: Re: [PATCH v7 4/5] clk: RK808: Add clkout driver for RK808

Quoting Chris Zhong (2014-09-01 02:46:40)
> Signed-off-by: Chris Zhong <zyw@...k-chips.com>
> 
> 
> Reviewed-by: Doug Anderson <dianders@...omium.org>
> Tested-by: Doug Anderson <dianders@...omium.org>

Hello Chris,

Thanks for submitting this patch. Could you fill in a proper changelog?
Also you should reorder the tags like so:

Reviewed-by: Doug Anderson <dianders@...omium.org>
Tested-by: Doug Anderson <dianders@...omium.org>
Signed-off-by: Chris Zhong <zyw@...k-chips.com>

<snip>

> +static int rk808_clkout2_control(struct clk_hw *hw, bool enable)
> +{
> +       struct rk808_clkout *rk808_clkout = container_of(hw,
> +                                                        struct rk808_clkout,
> +                                                        clkout2_hw);
> +       struct rk808 *rk808 = rk808_clkout->rk808;
> +
> +       return regmap_update_bits(rk808->regmap, RK808_CLK32OUT_REG,
> +                                 CLK32KOUT2_EN, enable ? CLK32KOUT2_EN : 0);
> +}

Nitpick: can you rename "control" to "enable" or "ungate"? That makes it
more obvious what the function is doing without having to inspect the
code in the function body.

<snip>

> +static int rk808_clkout_probe(struct platform_device *pdev)
> +{
> +       struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
> +       struct i2c_client *client = rk808->i2c;
> +       struct device_node *node = client->dev.of_node;
> +       struct clk_init_data init = {};
> +       struct clk **clk_table;
> +       struct rk808_clkout *rk808_clkout;
> +
> +       rk808_clkout = devm_kzalloc(&client->dev,
> +                                   sizeof(*rk808_clkout), GFP_KERNEL);
> +       if (!rk808_clkout)
> +               return -ENOMEM;
> +
> +       rk808_clkout->rk808 = rk808;
> +
> +       clk_table = devm_kzalloc(&client->dev,
> +                                2 * sizeof(struct clk *), GFP_KERNEL);

Better to use devm_kcalloc. Also good to define a constant like:

#define RK808_NR_OUTPUT 2

... and then use RK8808_NR_OUTPUT instead of hard-coding the value 2 in
the driver.

> +       if (!clk_table)
> +               return -ENOMEM;
> +
> +       init.flags = CLK_IS_ROOT;
> +       init.parent_names = NULL;
> +       init.num_parents = 0;
> +       init.name = "rk808-clkout1";
> +       init.ops = &rk808_clkout1_ops;
> +       rk808_clkout->clkout1_hw.init = &init;
> +
> +       /* optional override of the clockname */
> +       of_property_read_string_index(node, "clock-output-names",
> +                                     0, &init.name);
> +
> +       clk_table[0] = devm_clk_register(&client->dev,
> +                                        &rk808_clkout->clkout1_hw);
> +       if (IS_ERR(clk_table[0]))
> +               return PTR_ERR(clk_table[0]);
> +
> +       init.name = "rk808-clkout2";
> +       init.ops = &rk808_clkout2_ops;
> +       rk808_clkout->clkout2_hw.init = &init;
> +
> +       /* optional override of the clockname */
> +       of_property_read_string_index(node, "clock-output-names",
> +                                     1, &init.name);
> +
> +       clk_table[1] = devm_clk_register(&client->dev,
> +                                        &rk808_clkout->clkout2_hw);
> +       if (IS_ERR(clk_table[1]))
> +               return PTR_ERR(clk_table[1]);
> +
> +       rk808_clkout->clk_data.clks = clk_table;
> +       rk808_clkout->clk_data.clk_num = 2;

Again, here you can use RK808_NR_OUTPUT.

Otherwise the driver looks pretty good to me.

Thanks,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/