Re: [PATCH linux-next (v2) 2/2] clk: bcm6345: Add BCM6345 gated clock support

[Michael Turquette <mturquette@baylibre.com>]

Hi Simon,

Quoting Simon Arlott (2015-12-10 13:50:59)
> +#define to_clk_bcm6345(_hw) container_of(_hw, struct clk_bcm6345, hw)
> +
> +static int clk_bcm6345_enable(struct clk_hw *hw)
> +{
> +       struct clk_bcm6345 *gate = to_clk_bcm6345(hw);
> +
> +       return regmap_write_bits(gate->map, gate->offset,
> +                                       gate->mask, gate->mask);

Does your regmap hold a spinlock or mutex? If a mutex then this should
be a .prepare callback instead of .enable. If it's a spinlock then
nothing to see here, move along.

> +}
> +
> +static void clk_bcm6345_disable(struct clk_hw *hw)
> +{
> +       struct clk_bcm6345 *gate = to_clk_bcm6345(hw);
> +
> +       regmap_write_bits(gate->map, gate->offset,
> +                                       gate->mask, 0);
> +}
> +
> +static int clk_bcm6345_is_enabled(struct clk_hw *hw)
> +{
> +       struct clk_bcm6345 *gate = to_clk_bcm6345(hw);
> +       unsigned int val;
> +       int ret;
> +
> +       ret = regmap_read(gate->map, gate->offset, &val);
> +       if (ret)
> +               return ret;
> +
> +       val &= gate->mask;
> +
> +       return val ? 1 : 0;
> +}
> +
> +const struct clk_ops clk_bcm6345_ops = {
> +       .enable = clk_bcm6345_enable,
> +       .disable = clk_bcm6345_disable,
> +       .is_enabled = clk_bcm6345_is_enabled,
> +};
> +
> +static struct clk * __init of_bcm6345_clk_register(const char *parent_name,
> +       const char *clk_name, struct regmap *map, u32 offset, u32 mask)
> +{
> +       struct clk_bcm6345 *gate;
> +       struct clk_init_data init;
> +       struct clk *ret;
> +
> +       gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> +       if (!gate)
> +               return ERR_PTR(-ENOMEM);
> +
> +       init.name = clk_name;
> +       init.ops = &clk_bcm6345_ops;
> +       init.flags = CLK_IS_BASIC;

Why is CLK_IS_BASIC set?

> +       init.parent_names = (parent_name ? &parent_name : NULL);
> +       init.num_parents = (parent_name ? 1 : 0);
> +       gate->hw.init = &init;
> +       gate->map = map;
> +       gate->offset = offset;
> +       gate->mask = mask;
> +
> +       ret = clk_register(NULL, &gate->hw);
> +       if (IS_ERR(ret))
> +               kfree(gate);
> +
> +       return ret;
> +}
> +
> +static void __init of_bcm6345_clk_setup(struct device_node *node)
> +{
> +       struct clk_onecell_data *clk_data;
> +       const char *parent_name = NULL;
> +       struct regmap *map;
> +       u32 offset;
> +       unsigned int clocks = 0;
> +       int i;
> +
> +       clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
> +       if (!clk_data)
> +               return;
> +
> +       clk_data->clk_num = 32;
> +       clk_data->clks = kmalloc_array(clk_data->clk_num,
> +               sizeof(*clk_data->clks), GFP_KERNEL);
> +
> +       for (i = 0; i < clk_data->clk_num; i++)
> +               clk_data->clks[i] = ERR_PTR(-ENODEV);
> +
> +       if (of_clk_get_parent_count(node) > 0)
> +               parent_name = of_clk_get_parent_name(node, 0);
> +
> +       map = syscon_regmap_lookup_by_phandle(node, "regmap");
> +       if (IS_ERR(map)) {
> +               pr_err("%s: regmap lookup error %ld\n",
> +                       node->full_name, PTR_ERR(map));
> +               goto out;
> +       }
> +
> +       if (of_property_read_u32(node, "offset", &offset)) {
> +               pr_err("%s: offset not specified\n", node->full_name);
> +               goto out;
> +       }
> +
> +       /* clks[] is sparse, indexed by bit, maximum clocks checked using i */
> +       for (i = 0; i < clk_data->clk_num; i++) {
> +               u32 bit;
> +               const char *clk_name;
> +
> +               if (of_property_read_u32_index(node, "clock-indices",
> +                               i, &bit))
> +                       goto out;
> +
> +               if (of_property_read_string_index(node, "clock-output-names",
> +                               i, &clk_name))
> +                       goto out;
> +
> +               if (bit >= clk_data->clk_num) {
> +                       pr_err("%s: clock bit %u out of range\n",
> +                               node->full_name, bit);
> +                       continue;
> +               }
> +
> +               if (!IS_ERR(clk_data->clks[bit])) {
> +                       pr_err("%s: clock bit %u already exists\n",
> +                               node->full_name, bit);
> +                       continue;
> +               }
> +
> +               clk_data->clks[bit] = of_bcm6345_clk_register(parent_name,
> +                                       clk_name, map, offset, BIT(bit));
> +               if (!IS_ERR(clk_data->clks[bit]))
> +                       clocks++;
> +       }
> +
> +out:
> +       if (clocks) {
> +               of_clk_add_provider(node, of_clk_src_onecell_get, clk_data);
> +               pr_info("%s: registered %u clocks\n", node->name, clocks);
> +       } else {
> +               kfree(clk_data->clks);
> +               kfree(clk_data);
> +       }
> +}
> +
> +CLK_OF_DECLARE(bcm6345_clk, "brcm,bcm6345-gate-clk", of_bcm6345_clk_setup);

Please do not use CLK_OF_DECLARE unless there is a good reason. Ideally
this should be a platform_driver.

Regards,
Mike

> -- 
> 2.1.4
> 
> -- 
> Simon Arlott