On Thu, Nov 29, 2018 at 11:53:51PM -0800, Stephen Boyd wrote:
> Quoting Charles Keepax (2018-11-20 06:16:30)
Apologies for the delay on this we have been very swamped at this
end lately.
> > diff --git a/drivers/clk/clk-lochnagar.c b/drivers/clk/clk-lochnagar.c
> > new file mode 100644
> > index 000000000000..8b2a78689715
> > --- /dev/null
> > +++ b/drivers/clk/clk-lochnagar.c
> > @@ -0,0 +1,360 @@
> [...]
> > +
> > +static int lochnagar_regmap_set_parent(struct clk_hw *hw, u8 index)
> > +{
> > + struct lochnagar_clk *lclk = lochnagar_hw_to_lclk(hw);
> > + struct lochnagar_clk_priv *priv = lclk->priv;
> > + struct regmap *regmap = priv->regmap;
> > + int ret;
> > +
> > + /*
> > + * Some clocks on Lochnagar can either generate a clock themselves
> > + * or accept an external clock, these default to generating the clock
> > + * themselves. If we set a parent however we should update the dir_mask
> > + * to indicate to the hardware that this clock will now be receiving an
> > + * external clock.
>
> Hmm ok. So the plan is to configure parents in DT or from driver code if
> the configuration is to accept an external clk? I guess this works.
>
Actually from further discussions on the hardware side it seems
this is handled automatically by the hardware so we no longer
need to set these direction bits. As such I will remove them in
the next spin.
> > + */
> > + if (lclk->dir_mask) {
> > + ret = regmap_update_bits(regmap, lclk->cfg_reg,
> > + lclk->dir_mask, lclk->dir_mask);
> > + if (ret < 0) {
> > + dev_err(priv->dev, "Failed to set %s direction: %d\n",
> > + lclk->name, ret);
> > + return ret;
> > + }
> > + }
> > +
> > + ret = regmap_update_bits(regmap, lclk->src_reg, lclk->src_mask, index);
> > + if (ret < 0)
> > + dev_err(priv->dev, "Failed to reparent %s: %d\n",
> > + lclk->name, ret);
> > +
> > + return ret;
> > +}
> > +
> > +static u8 lochnagar_regmap_get_parent(struct clk_hw *hw)
> > +{
> > + struct lochnagar_clk *lclk = lochnagar_hw_to_lclk(hw);
> > + struct lochnagar_clk_priv *priv = lclk->priv;
> > + struct regmap *regmap = priv->regmap;
> > + unsigned int val;
> > + int ret;
> > +
> > + ret = regmap_read(regmap, lclk->src_reg, &val);
> > + if (ret < 0) {
> > + dev_err(priv->dev, "Failed to read parent of %s: %d\n",
> > + lclk->name, ret);
>
> The error messages in the above functions could be spammy. Just let
> drivers who fail when using these clks ops print errors and maybe
> downgrade these to debug? If you don't agree with this it's fine, I'll
> just hope to never see these prints change to debug in the future.
>
Seems reasonable to me I will change them to debug prints.
> > + return priv->nparents;
> > + }
> > +
> > + val &= lclk->src_mask;
> > +
> > + return val;
> > +}
> > +
> > +static const struct clk_ops lochnagar_clk_regmap_ops = {
> > + .prepare = lochnagar_regmap_prepare,
> > + .unprepare = lochnagar_regmap_unprepare,
> > + .set_parent = lochnagar_regmap_set_parent,
> > + .get_parent = lochnagar_regmap_get_parent,
>
> Is regmap important to have in the name of these functions and struct?
> I'd prefer it was just clk instead of regmap.
>
Again no objection happy to rename.
> > +};
> > +
> > +static int lochnagar_init_parents(struct lochnagar_clk_priv *priv)
> > +{
> > + struct device_node *np = priv->dev->of_node;
> > + int i, j;
> > +
> > + switch (priv->type) {
> > + case LOCHNAGAR1:
> > + memcpy(priv->lclks, lochnagar1_clks, sizeof(lochnagar1_clks));
> > +
> > + priv->nparents = ARRAY_SIZE(lochnagar1_clk_parents);
> > + priv->parents = devm_kmemdup(priv->dev, lochnagar1_clk_parents,
> > + sizeof(lochnagar1_clk_parents),
> > + GFP_KERNEL);
> > + break;
> > + case LOCHNAGAR2:
> > + memcpy(priv->lclks, lochnagar2_clks, sizeof(lochnagar2_clks));
> > +
> > + priv->nparents = ARRAY_SIZE(lochnagar2_clk_parents);
> > + priv->parents = devm_kmemdup(priv->dev, lochnagar2_clk_parents,
> > + sizeof(lochnagar2_clk_parents),
> > + GFP_KERNEL);
>
> Why do we need to kmemdup it? The clk framework already deep copies
> everything from clk_init structure.
>
The copy is needed for the updates to the list down below.
> > + break;
> > + default:
> > + dev_err(priv->dev, "Unknown Lochnagar type: %d\n", priv->type);
> > + return -EINVAL;
> > + }
> > +
> > + if (!priv->parents)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < priv->nparents; i++) {
> > + j = of_property_match_string(np, "clock-names",
> > + priv->parents[i]);
> > + if (j >= 0)
> > + priv->parents[i] = of_clk_get_parent_name(np, j);
>
> Isn't this of_clk_parent_fill()? But there are holes or something?
>
I guess rather than a fill, this is perhaps more of a name and
replace. I could make this a core function if you prefer? I think
there are a couple of other drivers that could also use it,
although might be worth doing that as a separate series rather
than holding this one up.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int lochnagar_init_clks(struct lochnagar_clk_priv *priv)
> > +{
> > + struct clk_init_data clk_init = {
> > + .ops = &lochnagar_clk_regmap_ops,
> > + .parent_names = priv->parents,
> > + .num_parents = priv->nparents,
> > + };
> > + struct lochnagar_clk *lclk;
> > + int ret, i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(priv->lclks); i++) {
>
> We already have an array of clks.
>
> > + lclk = &priv->lclks[i];
> > +
> > + if (!lclk->name)
> > + continue;
> > +
> > + clk_init.name = lclk->name;
> > +
> > + lclk->priv = priv;
> > + lclk->hw.init = &clk_init;
> > +
> > + ret = devm_clk_hw_register(priv->dev, &lclk->hw);
> > + if (ret) {
> > + dev_err(priv->dev, "Failed to register %s: %d\n",
> > + lclk->name, ret);
> > + return ret;
> > + }
> > +
> > + priv->clk_data->hws[i] = &lclk->hw;
>
> But then we copy the pointers into here to use of_clk_hw_onecell_get().
> Can you just roll your own function to use your own array of clk
> structures? I know it's sort of sad, but it avoids a copy.
>
Apologies for not doing it this way the first time, looks much
clearer that way round.
> > + }
> > +
> > + priv->clk_data->num = ARRAY_SIZE(priv->lclks);
> > +
> > + ret = devm_of_clk_add_hw_provider(priv->dev, of_clk_hw_onecell_get,
> > + priv->clk_data);
> > + if (ret < 0) {
> > + dev_err(priv->dev, "Failed to register provider: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
>
> Simplify this to
>
> if (ret < 0)
> dev_err(...)
>
> return ret;
>
No problem.
> > +}
> > +
> > +static const struct of_device_id lochnagar_of_match[] = {
> > + { .compatible = "cirrus,lochnagar1-clk", .data = (void *)LOCHNAGAR1 },
> > + { .compatible = "cirrus,lochnagar2-clk", .data = (void *)LOCHNAGAR2 },
> > + {},
>
> Nitpick: Drop the comma, it's the sentinel so nothing should come after.
>
Again no problem.
> > +};
>
> Any MODULE_DEVICE_TABLE?
>
Yes oversight there, will add that.
> > +static struct platform_driver lochnagar_clk_driver = {
> > + .driver = {
> > + .name = "lochnagar-clk",
> > + .of_match_table = of_match_ptr(lochnagar_of_match),
>
> I suspect of_match_ptr() makes the build complain about unused match
> table when CONFIG_OF=N. Can you try building it that way?
>
The driver depends on the MFD which in turn depends on CONFIG_OF
so that shouldn't be a problem.
> > + },
> > +
>
> Nitpick: Why the extra newline?
>
Happy to remove.
> > + .probe = lochnagar_clk_probe,
> > +};
> > +module_platform_driver(lochnagar_clk_driver);
> > +
> > +MODULE_AUTHOR("Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Clock driver for Cirrus Logic Lochnagar Board");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:lochnagar-clk");
>
> I think MODULE_ALIAS is not needed if it's this simple?
>
Not actually sure on this one, to be honest its mostly cargo
culted from other drivers. I will investigate and see what I dig
up but if has any pointers I would greatly appreciate it.
Should be able to send another version very shortly.
Thanks,
Charles
