Quoting Chris Zhong (2014-09-01 02:46:40)
> Signed-off-by: Chris Zhong <zyw@xxxxxxxxxxxxxx>
>
>
> Reviewed-by: Doug Anderson <dianders@xxxxxxxxxxxx>
> Tested-by: Doug Anderson <dianders@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
Tested-by: Doug Anderson <dianders@xxxxxxxxxxxx>
Signed-off-by: Chris Zhong <zyw@xxxxxxxxxxxxxx>
<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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
