Stephen, thanks for the review...
On 21.05.2015 at 21:31 Stephen Boyd wrote:
+static int clk_gpio_mux_set_parent(struct clk_hw *hw, u8 index)
+{
+ struct clk_gpio_mux *clk = to_clk_gpio_mux(hw);
+
+ if (index > 1)
+ return -EINVAL;
Doesn't seem possible if num_parents is correct. Please drop.
Right, thanks.
+ if (err) {
+ pr_err("%s: %s: Error requesting gpio %u\n",
+ __func__, name, desc_to_gpio(gpiod_sel));
What if the error is probe defer? We should be silent in such a
situation. I see this is mostly copy/paste from gpio-gate.c so
perhaps we should fix that one too.
Agreed.
+ return ERR_PTR(err);
+ }
+ clk_gpio_mux->gpiod_sel = gpiod_sel;
+
+ if (gpiod_ena != NULL) {
Style nitpick:
if (gpiod_ena) {
is preferred.
Agreed.
+ return ERR_PTR(err);
+ }
+ clk_gpio_mux->gpiod_ena = gpiod_ena;
+ }
+
+ init.name = name;
+ init.ops = &clk_gpio_mux_ops;
+ init.flags = clk_flags | CLK_IS_BASIC;
+ init.parent_names = parent_names;
+ init.num_parents = num_parents;
Should we make sure num_parents is 2?
You are probably right.
+
+ clk_gpio_mux->hw.init = &init;
+
+ clk = clk_register(dev, &clk_gpio_mux->hw);
But no if (dev) devm_*() trick here?
Oh, right.
+
+ if (!IS_ERR(clk))
+ return clk;
+
+ if (!dev) {
+ kfree(clk_gpio_mux);
+ gpiod_put(gpiod_ena);
Isn't gpiod_ena optional? And so calling gpiod_put() here might
cause a warning?
Oops, right.
Actually I wonder why we wouldn't just make this a gpio-mux
without enable/disable support? If we want to do enable/disable,
we can parent the gpio gate to this mux. Alternatively, we could
combine the gpio-gate file and this new mux file into one file
and support both compatible strings. There's quite a bit of
duplicated code so this may be a better approach.
I agree, I am going to remove the enable/disable support.
+static struct clk *of_clk_gpio_mux_delayed_register_get(
+ struct of_phandle_args *clkspec,
+ void *_data)
+{
+ struct clk_gpio_mux_delayed_register_data *data = _data;
+ struct clk *clk = ERR_PTR(-EINVAL);
+ const char *clk_name = data->node->name;
+ int i, num_parents;
+ const char **parent_names;
+ struct gpio_desc *gpiod_sel, *gpiod_ena;
+ int gpio;
+ u32 flags = 0;
This is only used on place and never assigned otherwise, why not
just use 0 in place of flags?
Well, you are probably right, I thought it is better to define it here,
than to use a magic number in clk_register_gpio_mux().
+
+ num_parents = of_clk_get_parent_count(data->node);
+ if (num_parents < 2) {
Should that be num_parents != 2?
Oops, right.
+ pr_err("mux-clock %s must have 2 parents\n", data->node->name);
+ return clk;
+ }
+
+ parent_names = kzalloc((sizeof(char *) * num_parents), GFP_KERNEL);
kcalloc() please.
OK.
+err_gpio:
+ mutex_unlock(&data->lock);
+ if (gpio == -EPROBE_DEFER)
+ pr_warn("%s: %s: GPIOs not yet available, retry later\n",
+ __func__, clk_name);
pr_debug
OK.
+ else
+ pr_err("%s: %s: Can't get GPIOs\n",
+ __func__, clk_name);
+ return ERR_PTR(gpio);
+}
+
+/**
+ * of_gpio_mux_clk_setup() - Setup function for gpio controlled clock mux
+ */
+void __init of_gpio_mux_clk_setup(struct device_node *node)
+{
+ struct clk_gpio_mux_delayed_register_data *data;
+
+ data = kzalloc(sizeof(struct clk_gpio_mux_delayed_register_data),
please use kzalloc(sizeof(*data), GFP_KERNEL); style
OK.
Best regards,
Sergej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
