Skip to contentSkip to conversation

clk: lochnagar: Use new parent_data approach to register clock parents

PATCH RESEND In review Charles Keepax 2 participants 4 messages Jun 25, 2019 – Jun 26, 2019

Subsystems (2)
Common clk Cirrus logic lochnagar driver
lore
Share & tools
lore

Charles Keepax

author maintainer
#0 lore raw

PATCH RESEND v3 1/1 clk: lochnagar: Use new parent_data approach to register clock parents

Switch over to the more modern style of registering parents and simplify the code in the process.

Suggested-by Stephen Boyd <[email protected]>
Signed-off-by Charles Keepax <[email protected]>
---
 drivers/clk/clk-lochnagar.c | 205 +++++++++++++++++++-------------------------
 1 file changed, 86 insertions(+), 119 deletions(-)
drivers/clk/clk-lochnagar.c +86 -119
index a2f31e58ee483..fa8c91758b1d7 100644
@@ -16,7 +16,6 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
-#include <linux/mfd/lochnagar.h>
 #include <linux/mfd/lochnagar1_regs.h>
 #include <linux/mfd/lochnagar2_regs.h>
 

@@ -40,48 +39,46 @@ struct lochnagar_clk {
 struct lochnagar_clk_priv {
 	struct device *dev;
 	struct regmap *regmap;
-	enum lochnagar_type type;
-
-	const char **parents;
-	unsigned int nparents;
 
 	struct lochnagar_clk lclks[LOCHNAGAR_NUM_CLOCKS];
 };
 
-static const char * const lochnagar1_clk_parents[] = {
-	"ln-none",
-	"ln-spdif-mclk",
-	"ln-psia1-mclk",
-	"ln-psia2-mclk",
-	"ln-cdc-clkout",
-	"ln-dsp-clkout",
-	"ln-pmic-32k",
-	"ln-gf-mclk1",
-	"ln-gf-mclk3",
-	"ln-gf-mclk2",
-	"ln-gf-mclk4",
+#define LN_PARENT(NAME) { .name = NAME, .fw_name = NAME }
+
+static const struct clk_parent_data lochnagar1_clk_parents[] = {
+	LN_PARENT("ln-none"),
+	LN_PARENT("ln-spdif-mclk"),
+	LN_PARENT("ln-psia1-mclk"),
+	LN_PARENT("ln-psia2-mclk"),
+	LN_PARENT("ln-cdc-clkout"),
+	LN_PARENT("ln-dsp-clkout"),
+	LN_PARENT("ln-pmic-32k"),
+	LN_PARENT("ln-gf-mclk1"),
+	LN_PARENT("ln-gf-mclk3"),
+	LN_PARENT("ln-gf-mclk2"),
+	LN_PARENT("ln-gf-mclk4"),
 };
 
-static const char * const lochnagar2_clk_parents[] = {
-	"ln-none",
-	"ln-cdc-clkout",
-	"ln-dsp-clkout",
-	"ln-pmic-32k",
-	"ln-spdif-mclk",
-	"ln-clk-12m",
-	"ln-clk-11m",
-	"ln-clk-24m",
-	"ln-clk-22m",
-	"ln-clk-8m",
-	"ln-usb-clk-24m",
-	"ln-gf-mclk1",
-	"ln-gf-mclk3",
-	"ln-gf-mclk2",
-	"ln-psia1-mclk",
-	"ln-psia2-mclk",
-	"ln-spdif-clkout",
-	"ln-adat-mclk",
-	"ln-usb-clk-12m",
+static const struct clk_parent_data lochnagar2_clk_parents[] = {
+	LN_PARENT("ln-none"),
+	LN_PARENT("ln-cdc-clkout"),
+	LN_PARENT("ln-dsp-clkout"),
+	LN_PARENT("ln-pmic-32k"),
+	LN_PARENT("ln-spdif-mclk"),
+	LN_PARENT("ln-clk-12m"),
+	LN_PARENT("ln-clk-11m"),
+	LN_PARENT("ln-clk-24m"),
+	LN_PARENT("ln-clk-22m"),
+	LN_PARENT("ln-clk-8m"),
+	LN_PARENT("ln-usb-clk-24m"),
+	LN_PARENT("ln-gf-mclk1"),
+	LN_PARENT("ln-gf-mclk3"),
+	LN_PARENT("ln-gf-mclk2"),
+	LN_PARENT("ln-psia1-mclk"),
+	LN_PARENT("ln-psia2-mclk"),
+	LN_PARENT("ln-spdif-clkout"),
+	LN_PARENT("ln-adat-mclk"),
+	LN_PARENT("ln-usb-clk-12m"),
 };
 
 #define LN1_CLK(ID, NAME, REG) \

@@ -122,6 +119,24 @@ static const struct lochnagar_clk lochnagar2_clks[LOCHNAGAR_NUM_CLOCKS] = {
 	LN2_CLK(SOUNDCARD_MCLK, "ln-soundcard-mclk"),
 };
 
+struct lochnagar_config {
+	const struct clk_parent_data *parents;
+	int nparents;
+	const struct lochnagar_clk *clks;
+};
+
+static const struct lochnagar_config lochnagar1_conf = {
+	.parents = lochnagar1_clk_parents,
+	.nparents = ARRAY_SIZE(lochnagar1_clk_parents),
+	.clks = lochnagar1_clks,
+};
+
+static const struct lochnagar_config lochnagar2_conf = {
+	.parents = lochnagar2_clk_parents,
+	.nparents = ARRAY_SIZE(lochnagar2_clk_parents),
+	.clks = lochnagar2_clks,
+};
+
 static inline struct lochnagar_clk *lochnagar_hw_to_lclk(struct clk_hw *hw)
 {
 	return container_of(hw, struct lochnagar_clk, hw);

@@ -183,7 +198,7 @@ static u8 lochnagar_clk_get_parent(struct clk_hw *hw)
 	if (ret < 0) {
 		dev_dbg(priv->dev, "Failed to read parent of %s: %d\n",
 			lclk->name, ret);
-		return priv->nparents;
+		return hw->init->num_parents;
 	}
 
 	val &= lclk->src_mask;

@@ -198,46 +213,6 @@ static const struct clk_ops lochnagar_clk_ops = {
 	.get_parent = lochnagar_clk_get_parent,
 };
 
-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);
-		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);
-	}
-
-	return 0;
-}
-
 static struct clk_hw *
 lochnagar_of_clk_hw_get(struct of_phandle_args *clkspec, void *data)
 {

@@ -252,16 +227,42 @@ lochnagar_of_clk_hw_get(struct of_phandle_args *clkspec, void *data)
 	return &priv->lclks[idx].hw;
 }
 
-static int lochnagar_init_clks(struct lochnagar_clk_priv *priv)
+static const struct of_device_id lochnagar_of_match[] = {
+	{ .compatible = "cirrus,lochnagar1-clk", .data = &lochnagar1_conf },
+	{ .compatible = "cirrus,lochnagar2-clk", .data = &lochnagar2_conf },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lochnagar_of_match);
+
+static int lochnagar_clk_probe(struct platform_device *pdev)
 {
 	struct clk_init_data clk_init = {
 		.ops = &lochnagar_clk_ops,
-		.parent_names = priv->parents,
-		.num_parents = priv->nparents,
 	};
+	struct device *dev = &pdev->dev;
+	struct lochnagar_clk_priv *priv;
+	const struct of_device_id *of_id;
 	struct lochnagar_clk *lclk;
+	struct lochnagar_config *conf;
 	int ret, i;
 
+	of_id = of_match_device(lochnagar_of_match, dev);
+	if (!of_id)
+		return -EINVAL;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->regmap = dev_get_regmap(dev->parent, NULL);
+	conf = (struct lochnagar_config *)of_id->data;
+
+	memcpy(priv->lclks, conf->clks, sizeof(priv->lclks));
+
+	clk_init.parent_data = conf->parents;
+	clk_init.num_parents = conf->nparents;
+
 	for (i = 0; i < ARRAY_SIZE(priv->lclks); i++) {
 		lclk = &priv->lclks[i];
 

@@ -273,55 +274,21 @@ static int lochnagar_init_clks(struct lochnagar_clk_priv *priv)
 		lclk->priv = priv;
 		lclk->hw.init = &clk_init;
 
-		ret = devm_clk_hw_register(priv->dev, &lclk->hw);
+		ret = devm_clk_hw_register(dev, &lclk->hw);
 		if (ret) {
-			dev_err(priv->dev, "Failed to register %s: %d\n",
+			dev_err(dev, "Failed to register %s: %d\n",
 				lclk->name, ret);
 			return ret;
 		}
 	}
 
-	ret = devm_of_clk_add_hw_provider(priv->dev, lochnagar_of_clk_hw_get,
-					  priv);
+	ret = devm_of_clk_add_hw_provider(dev, lochnagar_of_clk_hw_get, priv);
 	if (ret < 0)
-		dev_err(priv->dev, "Failed to register provider: %d\n", ret);
+		dev_err(dev, "Failed to register provider: %d\n", ret);
 
 	return ret;
 }
 
-static const struct of_device_id lochnagar_of_match[] = {
-	{ .compatible = "cirrus,lochnagar1-clk", .data = (void *)LOCHNAGAR1 },
-	{ .compatible = "cirrus,lochnagar2-clk", .data = (void *)LOCHNAGAR2 },
-	{}
-};
-MODULE_DEVICE_TABLE(of, lochnagar_of_match);
-
-static int lochnagar_clk_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct lochnagar_clk_priv *priv;
-	const struct of_device_id *of_id;
-	int ret;
-
-	of_id = of_match_device(lochnagar_of_match, dev);
-	if (!of_id)
-		return -EINVAL;
-
-	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	priv->dev = dev;
-	priv->regmap = dev_get_regmap(dev->parent, NULL);
-	priv->type = (enum lochnagar_type)of_id->data;
-
-	ret = lochnagar_init_parents(priv);
-	if (ret)
-		return ret;
-
-	return lochnagar_init_clks(priv);
-}
-
 static struct platform_driver lochnagar_clk_driver = {
 	.driver = {
 		.name = "lochnagar-clk",
Signature · 1 line
2.11.0
maintainer
#1 lore raw

Quoting Charles Keepax (2019-06-25 06:10:53)

Quoted 7 lines
drivers/clk/clk-lochnagar.c +0 -7
-static const char * const lochnagar1_clk_parents[] = {
-       "ln-none",
-       "ln-spdif-mclk",
-       "ln-psia1-mclk",
-       "ln-psia2-mclk",
-       "ln-cdc-clkout",
-       "ln-dsp-clkout",

drivers/clk/clk-lochnagar.c struct lochnagar_clk { · line 50

Quoted 10 lines
drivers/clk/clk-lochnagar.c +5 -5
-       "ln-pmic-32k",
-       "ln-gf-mclk1",
-       "ln-gf-mclk3",
-       "ln-gf-mclk2",
-       "ln-gf-mclk4",
+#define LN_PARENT(NAME) { .name = NAME, .fw_name = NAME }
+
+static const struct clk_parent_data lochnagar1_clk_parents[] = {
+       LN_PARENT("ln-none"),
+       LN_PARENT("ln-spdif-mclk"),

The above two aren't documented in the binding. Is it intentional? I'd like to apply this patch, but I don't want it to use undocumented bindings.

Quoted 11 lines
drivers/clk/clk-lochnagar.c +9 -0
+       LN_PARENT("ln-psia1-mclk"),
+       LN_PARENT("ln-psia2-mclk"),
+       LN_PARENT("ln-cdc-clkout"),
+       LN_PARENT("ln-dsp-clkout"),
+       LN_PARENT("ln-pmic-32k"),
+       LN_PARENT("ln-gf-mclk1"),
+       LN_PARENT("ln-gf-mclk3"),
+       LN_PARENT("ln-gf-mclk2"),
+       LN_PARENT("ln-gf-mclk4"),
 };
author maintainer
#2 lore raw
Quoted 14 lines · Stephen Boyd

Quoting Charles Keepax (2019-06-25 06:10:53)

Quoted 13 lines
drivers/clk/clk-lochnagar.c +1 -12
-static const char * const lochnagar1_clk_parents[] = {
-       "ln-none",
-       "ln-spdif-mclk",
-       "ln-psia1-mclk",
-       "ln-psia2-mclk",
-       "ln-cdc-clkout",
-       "ln-dsp-clkout",
-       "ln-pmic-32k",
-       "ln-gf-mclk1",
-       "ln-gf-mclk3",
-       "ln-gf-mclk2",
-       "ln-gf-mclk4",
+#define LN_PARENT(NAME) { .name = NAME, .fw_name = NAME }
Quoted 9 lines

drivers/clk/clk-lochnagar.c struct lochnagar_clk { · line 50

Quoted 4 lines
drivers/clk/clk-lochnagar.c +4 -0
+
+static const struct clk_parent_data lochnagar1_clk_parents[] = {
+       LN_PARENT("ln-none"),
+       LN_PARENT("ln-spdif-mclk"),

The above two aren't documented in the binding. Is it intentional? I'd like to apply this patch, but I don't want it to use undocumented bindings.

ln-none is intentionally missing from the binding, it isn't really used at the moment, it is really more of a place holder for when the clocks are not parented to anything as many will generate a clock in that case. Maybe in the future it might be used to allow clocks to be put back into that state after being parented but that probably requires more thought.

ln-spdif-mclk should be in the binding though I will do a patch to add it. Do you want me to resend this patch as well? Feels like this stuff is orthogonal to what this patch is doing.

Thanks,
Charles

Quoted 11 lines
Quoted 11 lines
drivers/clk/clk-lochnagar.c +9 -0
+       LN_PARENT("ln-psia1-mclk"),
+       LN_PARENT("ln-psia2-mclk"),
+       LN_PARENT("ln-cdc-clkout"),
+       LN_PARENT("ln-dsp-clkout"),
+       LN_PARENT("ln-pmic-32k"),
+       LN_PARENT("ln-gf-mclk1"),
+       LN_PARENT("ln-gf-mclk3"),
+       LN_PARENT("ln-gf-mclk2"),
+       LN_PARENT("ln-gf-mclk4"),
 };
maintainer
#3 lore raw

Quoting Charles Keepax (2019-06-26 06:18:02)

Quoted 25 lines · Stephen Boyd
Quoted 14 lines

Quoting Charles Keepax (2019-06-25 06:10:53)

Quoted 13 lines
drivers/clk/clk-lochnagar.c +1 -12
-static const char * const lochnagar1_clk_parents[] = {
-       "ln-none",
-       "ln-spdif-mclk",
-       "ln-psia1-mclk",
-       "ln-psia2-mclk",
-       "ln-cdc-clkout",
-       "ln-dsp-clkout",
-       "ln-pmic-32k",
-       "ln-gf-mclk1",
-       "ln-gf-mclk3",
-       "ln-gf-mclk2",
-       "ln-gf-mclk4",
+#define LN_PARENT(NAME) { .name = NAME, .fw_name = NAME }
Quoted 9 lines

drivers/clk/clk-lochnagar.c struct lochnagar_clk { · line 50

Quoted 4 lines
drivers/clk/clk-lochnagar.c +4 -0
+
+static const struct clk_parent_data lochnagar1_clk_parents[] = {
+       LN_PARENT("ln-none"),
+       LN_PARENT("ln-spdif-mclk"),

The above two aren't documented in the binding. Is it intentional? I'd like to apply this patch, but I don't want it to use undocumented bindings.

Quoted 6 lines

ln-none is intentionally missing from the binding, it isn't really used at the moment, it is really more of a place holder for when the clocks are not parented to anything as many will generate a clock in that case. Maybe in the future it might be used to allow clocks to be put back into that state after being parented but that probably requires more thought.

Hmm.. So basically it's a way to configure the clk to have no parent anymore and not be orphaned but output a clk signal? I'll have to think about this more but maybe this means we should let clks parent to themselves to indicate that they're not using anything else to output a clk signal, i.e. clk_set_parent(clk, clk) would make it a root clk if it's possible.

Quoted 5 lines

ln-spdif-mclk should be in the binding though I will do a patch to add it. Do you want me to resend this patch as well? Feels like this stuff is orthogonal to what this patch is doing.

A follow-up patch is fine for this. I'll stack it on top and merge it up to clk-next.