diff mbox series

[v5,7/8] clk: lochnagar: Add support for the Cirrus Logic Lochnagar

Message ID 20181120141631.18949-7-ckeepax@opensource.cirrus.com
State New
Headers show
Series [v5,1/8] regulator: lochnagar: Move driver to binding from DT | expand

Commit Message

Charles Keepax Nov. 20, 2018, 2:16 p.m. UTC
Lochnagar is an evaluation and development board for Cirrus
Logic Smart CODEC and Amp devices. It allows the connection of
most Cirrus Logic devices on mini-cards, as well as allowing
connection of various application processor systems to provide a
full evaluation platform. This driver supports the board
controller chip on the Lochnagar board.

The Lochnagar can take several input clocks from the host system,
provides several of its own clock sources, and provides extensive
routing options for those clocks to be supplied to the attached
CODEC/Amp device.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

No change since v4.

Thanks,
Charles

 drivers/clk/Kconfig         |   7 +
 drivers/clk/Makefile        |   1 +
 drivers/clk/clk-lochnagar.c | 360 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 368 insertions(+)
 create mode 100644 drivers/clk/clk-lochnagar.c

Comments

Stephen Boyd Nov. 30, 2018, 7:53 a.m. UTC | #1
Quoting Charles Keepax (2018-11-20 06:16:30)
> 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.

> +        */
> +       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.

> +               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.

> +};
> +
> +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.

> +               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?

> +       }
> +
> +       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.

> +       }
> +
> +       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;

> +}
> +
> +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.

> +};

Any MODULE_DEVICE_TABLE?

> +
> +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->clk_data = devm_kzalloc(dev, struct_size(priv->clk_data, hws,
> +                                                      ARRAY_SIZE(priv->lclks)),
> +                                     GFP_KERNEL);
> +       if (!priv->clk_data)
> +               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;
> +
> +       ret = lochnagar_init_clks(priv);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +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?

> +       },
> +

Nitpick: Why the extra newline?

> +       .probe = lochnagar_clk_probe,
> +};
> +module_platform_driver(lochnagar_clk_driver);
> +
> +MODULE_AUTHOR("Charles Keepax <ckeepax@opensource.cirrus.com>");
> +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?
Charles Keepax Dec. 21, 2018, 1:50 p.m. UTC | #2
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@opensource.cirrus.com>");
> > +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
Charles Keepax Dec. 21, 2018, 3:28 p.m. UTC | #3
On Fri, Dec 21, 2018 at 01:50:37PM +0000, Charles Keepax wrote:
> On Thu, Nov 29, 2018 at 11:53:51PM -0800, Stephen Boyd wrote:
> > Quoting Charles Keepax (2018-11-20 06:16:30)
> > > +MODULE_AUTHOR("Charles Keepax <ckeepax@opensource.cirrus.com>");
> > > +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.
> 

From what I can find out it looks like MODULE_ALIAS is indeed
redundant on DT only drivers, which these are at the moment so
will remove and give it a test.

Thanks,
Charles
Stephen Boyd Dec. 21, 2018, 8:44 p.m. UTC | #4
Quoting Charles Keepax (2018-12-21 05:50:37)
> On Thu, Nov 29, 2018 at 11:53:51PM -0800, Stephen Boyd wrote:
> > Quoting Charles Keepax (2018-11-20 06:16:30)
> 
> > > +               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.

No worries. I'm actively changing the parent_names design anyway so this
is fine.

> 
> 
> > > +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.

Ok. Then it's really not needed and it's better to drop the usage of
of_match_ptr() then.

>
diff mbox series

Patch

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 81cdb4eaca07..2c63c431879f 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -219,6 +219,13 @@  config COMMON_CLK_XGENE
 	---help---
 	  Sypport for the APM X-Gene SoC reference, PLL, and device clocks.
 
+config COMMON_CLK_LOCHNAGAR
+	tristate "Cirrus Logic Lochnagar clock driver"
+	depends on MFD_LOCHNAGAR
+	help
+	  This driver supports the clocking features of the Cirrus Logic
+	  Lochnagar audio development board.
+
 config COMMON_CLK_NXP
 	def_bool COMMON_CLK && (ARCH_LPC18XX || ARCH_LPC32XX)
 	select REGMAP_MMIO if ARCH_LPC32XX
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 72be7a38cff1..2354f0c8c8f1 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -30,6 +30,7 @@  obj-$(CONFIG_COMMON_CLK_GEMINI)		+= clk-gemini.o
 obj-$(CONFIG_COMMON_CLK_ASPEED)		+= clk-aspeed.o
 obj-$(CONFIG_ARCH_HIGHBANK)		+= clk-highbank.o
 obj-$(CONFIG_CLK_HSDK)			+= clk-hsdk-pll.o
+obj-$(CONFIG_COMMON_CLK_LOCHNAGAR)	+= clk-lochnagar.o
 obj-$(CONFIG_COMMON_CLK_MAX77686)	+= clk-max77686.o
 obj-$(CONFIG_COMMON_CLK_MAX9485)	+= clk-max9485.o
 obj-$(CONFIG_ARCH_MOXART)		+= clk-moxart.o
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 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Lochnagar clock control
+ *
+ * Copyright (c) 2017-2018 Cirrus Logic, Inc. and
+ *                         Cirrus Logic International Semiconductor Ltd.
+ *
+ * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#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>
+
+#include <dt-bindings/clk/lochnagar.h>
+
+#define LOCHNAGAR_NUM_CLOCKS	(LOCHNAGAR_SPDIF_CLKOUT + 1)
+
+struct lochnagar_clk {
+	const char * const name;
+	struct clk_hw hw;
+
+	struct lochnagar_clk_priv *priv;
+
+	u16 cfg_reg;
+	u16 ena_mask;
+	u16 dir_mask; /* Control bit to set clock as a source or sink */
+
+	u16 src_reg;
+	u16 src_mask;
+};
+
+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];
+
+	struct clk_hw_onecell_data *clk_data;
+};
+
+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",
+};
+
+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-reserved",
+	"ln-usb-clk-24m",
+	"ln-gf-mclk1",
+	"ln-gf-mclk3",
+	"ln-gf-mclk2",
+	"ln-psia1-mclk",
+	"ln-psia2-mclk",
+	"ln-spdif-clkout",
+	"ln-adat-clkout",
+	"ln-usb-clk-12m",
+};
+
+#define LN1_CLK(ID, NAME, REG) \
+	[LOCHNAGAR_##ID] = { \
+		.name = NAME, \
+		.cfg_reg = LOCHNAGAR1_##REG, \
+		.ena_mask = LOCHNAGAR1_##ID##_ENA_MASK, \
+		.src_reg = LOCHNAGAR1_##ID##_SEL, \
+		.src_mask = LOCHNAGAR1_SRC_MASK, \
+	}
+
+#define LN2_CLK(ID, NAME) \
+	[LOCHNAGAR_##ID] = { \
+		.name = NAME, \
+		.cfg_reg = LOCHNAGAR2_##ID##_CTRL, \
+		.src_reg = LOCHNAGAR2_##ID##_CTRL, \
+		.ena_mask = LOCHNAGAR2_CLK_ENA_MASK, \
+		.dir_mask = LOCHNAGAR2_CLK_DIR_MASK, \
+		.src_mask = LOCHNAGAR2_CLK_SRC_MASK, \
+	}
+
+static const struct lochnagar_clk lochnagar1_clks[LOCHNAGAR_NUM_CLOCKS] = {
+	LN1_CLK(CDC_MCLK1,      "ln-cdc-mclk1",  CDC_AIF_CTRL2),
+	LN1_CLK(CDC_MCLK2,      "ln-cdc-mclk2",  CDC_AIF_CTRL2),
+	LN1_CLK(DSP_CLKIN,      "ln-dsp-clkin",  DSP_AIF),
+	LN1_CLK(GF_CLKOUT1,     "ln-gf-clkout1", GF_AIF1),
+};
+
+static const struct lochnagar_clk lochnagar2_clks[LOCHNAGAR_NUM_CLOCKS] = {
+	LN2_CLK(CDC_MCLK1,      "ln-cdc-mclk1"),
+	LN2_CLK(CDC_MCLK2,      "ln-cdc-mclk2"),
+	LN2_CLK(DSP_CLKIN,      "ln-dsp-clkin"),
+	LN2_CLK(GF_CLKOUT1,     "ln-gf-clkout1"),
+	LN2_CLK(GF_CLKOUT2,     "ln-gf-clkout2"),
+	LN2_CLK(PSIA1_MCLK,     "ln-psia1-mclk"),
+	LN2_CLK(PSIA2_MCLK,     "ln-psia2-mclk"),
+	LN2_CLK(SPDIF_MCLK,     "ln-spdif-mclk"),
+	LN2_CLK(ADAT_MCLK,      "ln-adat-mclk"),
+	LN2_CLK(SOUNDCARD_MCLK, "ln-soundcard-mclk"),
+};
+
+static inline struct lochnagar_clk *lochnagar_hw_to_lclk(struct clk_hw *hw)
+{
+	return container_of(hw, struct lochnagar_clk, hw);
+}
+
+static int lochnagar_regmap_prepare(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;
+	int ret;
+
+	ret = regmap_update_bits(regmap, lclk->cfg_reg,
+				 lclk->ena_mask, lclk->ena_mask);
+	if (ret < 0)
+		dev_err(priv->dev, "Failed to prepare %s: %d\n",
+			lclk->name, ret);
+
+	return ret;
+}
+
+static void lochnagar_regmap_unprepare(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;
+	int ret;
+
+	ret = regmap_update_bits(regmap, lclk->cfg_reg, lclk->ena_mask, 0);
+	if (ret < 0)
+		dev_err(priv->dev, "Failed to unprepare %s: %d\n",
+			lclk->name, ret);
+}
+
+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.
+	 */
+	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);
+		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,
+};
+
+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 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++) {
+		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;
+	}
+
+	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;
+}
+
+static const struct of_device_id lochnagar_of_match[] = {
+	{ .compatible = "cirrus,lochnagar1-clk", .data = (void *)LOCHNAGAR1 },
+	{ .compatible = "cirrus,lochnagar2-clk", .data = (void *)LOCHNAGAR2 },
+	{},
+};
+
+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->clk_data = devm_kzalloc(dev, struct_size(priv->clk_data, hws,
+						       ARRAY_SIZE(priv->lclks)),
+				      GFP_KERNEL);
+	if (!priv->clk_data)
+		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;
+
+	ret = lochnagar_init_clks(priv);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct platform_driver lochnagar_clk_driver = {
+	.driver = {
+		.name = "lochnagar-clk",
+		.of_match_table = of_match_ptr(lochnagar_of_match),
+	},
+
+	.probe = lochnagar_clk_probe,
+};
+module_platform_driver(lochnagar_clk_driver);
+
+MODULE_AUTHOR("Charles Keepax <ckeepax@opensource.cirrus.com>");
+MODULE_DESCRIPTION("Clock driver for Cirrus Logic Lochnagar Board");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:lochnagar-clk");