Re: [PATCH] clk: palmas: add clock driver for palmas

[Mike Turquette <mturquette@linaro.org>]

Quoting Laxman Dewangan (2013-10-03 05:30:07)
> +static struct clk_ops palmas_clks_ops = {
> +       .prepare        = palmas_clks_prepare,
> +       .unprepare      = palmas_clks_unprepare,
> +       .is_enabled     = palmas_clks_is_enabled,

Use .is_prepared if a call to clk_prepare for your clock will start
generating a signal. No need for .is_enabled since you do not define
your own .enable & .disable callbacks.

> +};
> +
> +static struct clk_init_data palmas_clks_hw_init[PALMAS_CLOCK32K_NR] = {
> +       [PALMAS_CLOCK32KG] = {
> +               .name = "clk32k_kg",
> +               .ops = &palmas_clks_ops,
> +               .flags = CLK_IS_ROOT | CLK_IGNORE_UNUSED,
> +       },
> +       [PALMAS_CLOCK32KG_AUDIO] = {
> +               .name = "clk32k_kg_audio",
> +               .ops = &palmas_clks_ops,
> +               .flags = CLK_IS_ROOT | CLK_IGNORE_UNUSED,
> +       },
> +};
> +
> +static int palmas_clks_get_clk_data(struct platform_device *pdev,
> +       struct palmas_clks *palmas_clks)
> +{
> +       struct device_node *node = pdev->dev.of_node;
> +       struct device_node *child;
> +       struct palmas_clock_info *cinfo;
> +       unsigned int prop;
> +       int ret;
> +       int i;
> +
> +       for (i = 0; i < PALMAS_CLOCK32K_NR; ++i) {
> +               child = of_get_child_by_name(node,
> +                               palmas_clk32k_descs[i].clk_name);
> +               if (!child)
> +                       continue;
> +
> +               cinfo = &palmas_clks->clk_info[i];
> +               cinfo->boot_enable = of_property_read_bool(child,
> +                                               "ti,clock-boot-enable");
> +               ret = of_property_read_u32(child, "ti,external-sleep-control",
> +                                       &prop);
> +               if (!ret) {
> +                       switch (prop) {
> +                       case 1:
> +                               prop = PALMAS_EXT_CONTROL_ENABLE1;
> +                               break;
> +                       case 2:
> +                               prop = PALMAS_EXT_CONTROL_ENABLE2;
> +                               break;
> +                       case 3:
> +                               prop = PALMAS_EXT_CONTROL_NSLEEP;
> +                               break;
> +                       default:
> +                               WARN_ON(1);
> +                               dev_warn(&pdev->dev,
> +                                       "%s: Invalid ext control option: %u\n",
> +                                       child->name, prop);
> +                               prop = 0;
> +                               break;
> +                       }
> +                       cinfo->ext_control_pin = prop;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static int palmas_clks_init_configure(struct palmas_clock_info *cinfo)
> +{
> +       struct palmas_clks *palmas_clks = cinfo->palmas_clk;
> +       int ret;
> +       unsigned int val = 0;
> +       unsigned int mask;
> +
> +       if (cinfo->boot_enable || cinfo->ext_control_pin)
> +               val |= cinfo->clk_desc->enable_mask;

So if the ti,clock-boot-enable property exists then a clock signal will
be generated by palmas but the Linux clock tree representation won't
know it. This is bad.

Better to call clk_get and clk_prepare_enable here instead of cheating
and directly writing to the register.

Regards,
Mike