[v2,1/5] mfd: lochnagar: Add initial binding documentation

Message ID 20181008132542.19775-1-ckeepax@opensource.cirrus.com
State Not Applicable
Headers show
Series
  • [v2,1/5] mfd: lochnagar: Add initial binding documentation
Related show

Commit Message

Charles Keepax Oct. 8, 2018, 1:25 p.m.
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.

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

Changes since v1:
 - Move the binding includes into this patch.
 - Move patch to the start of the patch chain.
 - Update commit message a little

Thanks,
Charles

 .../devicetree/bindings/mfd/cirrus,lochnagar.txt   | 230 +++++++++++++++++++++
 include/dt-bindings/clk/lochnagar.h                |  33 +++
 include/dt-bindings/pinctrl/lochnagar.h            | 132 ++++++++++++
 3 files changed, 395 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
 create mode 100644 include/dt-bindings/clk/lochnagar.h
 create mode 100644 include/dt-bindings/pinctrl/lochnagar.h

Comments

Stephen Boyd Oct. 11, 2018, 7 a.m. | #1
Quoting Charles Keepax (2018-10-08 06:25:40)
> diff --git a/drivers/clk/clk-lochnagar.c b/drivers/clk/clk-lochnagar.c
> new file mode 100644
> index 0000000000000..fa54531dbf501
> --- /dev/null
> +++ b/drivers/clk/clk-lochnagar.c
> @@ -0,0 +1,449 @@
> +// 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.h>

Is this used?

> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>

Is this used?

> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>

Used?

> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/lochnagar.h>
> +#include <dt-bindings/clk/lochnagar.h>
> +
> +#define LOCHNAGAR_NUM_CLOCKS   (LOCHNAGAR_SPDIF_CLKOUT + 1)
> +
> +enum lochnagar_clk_type {
> +       LOCHNAGAR_CLK_TYPE_UNUSED,
> +       LOCHNAGAR_CLK_TYPE_FIXED,
> +       LOCHNAGAR_CLK_TYPE_REGMAP,
> +};
> +
> +struct lochnagar_regmap_clk {
> +       unsigned int cfg_reg;
> +       unsigned int ena_mask;
> +       unsigned int dir_mask;
> +
> +       unsigned int src_reg;
> +       unsigned int src_mask;

Are these 32 bits or 16 bits or 8 bits? Please use a u32/u16/u8 so we
know the register width.

> +};
> +
> +struct lochnagar_fixed_clk {
> +       unsigned int rate;
> +};
> +
> +struct lochnagar_clk {
> +       struct lochnagar_clk_priv *priv;
> +       struct clk_hw hw;
> +
> +       const char * const name;
> +
> +       enum lochnagar_clk_type type;
> +       union {
> +               struct lochnagar_fixed_clk fixed;
> +               struct lochnagar_regmap_clk regmap;
> +       };
> +};
> +
> +struct lochnagar_clk_priv {
> +       struct device *dev;
> +       struct lochnagar *lochnagar;

Is this used for anything besides getting the regmap? Can you get the
pointer to the parent in probe and use that to get the regmap pointer
from dev_get_remap() and also use the of_node of the parent to register
a clk provider? It would be nice to avoid including the mfd header file
unless it's providing something useful.

> +
> +       const char **parents;
> +       unsigned int nparents;
> +
> +       struct lochnagar_clk lclks[LOCHNAGAR_NUM_CLOCKS];
> +
> +       struct clk *clks[LOCHNAGAR_NUM_CLOCKS];
> +       struct clk_onecell_data of_clks;
> +};
> +
> +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 LN_CLK_FIXED(ID, NAME, RATE) \
> +       [LOCHNAGAR_##ID] = { \
> +               .name = NAME, .type = LOCHNAGAR_CLK_TYPE_FIXED, \
> +               { .fixed.rate = RATE, }, \
> +       }

Can all these fixed clks come from DT instead of being populated by this
driver? Unless they're being generated by this hardware block and not
actually a crystal or some other on-board clock that goes into this
hardware block and then right out of it?

> +
> +#define LN1_CLK_REGMAP(ID, NAME, REG, ...) \
> +       [LOCHNAGAR_##ID] = { \
> +               .name = NAME, .type = LOCHNAGAR_CLK_TYPE_REGMAP, \
> +               { .regmap = { \
> +                       __VA_ARGS__ \
> +                       .cfg_reg = LOCHNAGAR1_##REG, \
> +                       .ena_mask = LOCHNAGAR1_##ID##_ENA_MASK, \
> +                       .src_reg = LOCHNAGAR1_##ID##_SEL, \
> +                       .src_mask = LOCHNAGAR1_SRC_MASK, \
> +               }, }, \
> +       }
> +
> +#define LN2_CLK_REGMAP(ID, NAME) \
> +       [LOCHNAGAR_##ID] = { \
> +               .name = NAME, .type = LOCHNAGAR_CLK_TYPE_REGMAP, \
> +               { .regmap = { \
> +                       .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_REGMAP(CDC_MCLK1,      "ln-cdc-mclk1",  CDC_AIF_CTRL2),
> +       LN1_CLK_REGMAP(CDC_MCLK2,      "ln-cdc-mclk2",  CDC_AIF_CTRL2),
> +       LN1_CLK_REGMAP(DSP_CLKIN,      "ln-dsp-clkin",  DSP_AIF),
> +       LN1_CLK_REGMAP(GF_CLKOUT1,     "ln-gf-clkout1", GF_AIF1),
> +
> +       LN_CLK_FIXED(PMIC_32K,         "ln-pmic-32k",   32768),
> +};
> +
> +static const struct lochnagar_clk lochnagar2_clks[LOCHNAGAR_NUM_CLOCKS] = {
> +       LN2_CLK_REGMAP(CDC_MCLK1,      "ln-cdc-mclk1"),
> +       LN2_CLK_REGMAP(CDC_MCLK2,      "ln-cdc-mclk2"),
> +       LN2_CLK_REGMAP(DSP_CLKIN,      "ln-dsp-clkin"),
> +       LN2_CLK_REGMAP(GF_CLKOUT1,     "ln-gf-clkout1"),
> +       LN2_CLK_REGMAP(GF_CLKOUT2,     "ln-gf-clkout2"),
> +       LN2_CLK_REGMAP(PSIA1_MCLK,     "ln-psia1-mclk"),
> +       LN2_CLK_REGMAP(PSIA2_MCLK,     "ln-psia2-mclk"),
> +       LN2_CLK_REGMAP(SPDIF_MCLK,     "ln-spdif-mclk"),
> +       LN2_CLK_REGMAP(ADAT_MCLK,      "ln-adat-mclk"),
> +       LN2_CLK_REGMAP(SOUNDCARD_MCLK, "ln-soundcard-mclk"),
> +
> +       LN_CLK_FIXED(PMIC_32K,         "ln-pmic-32k",    32768),
> +       LN_CLK_FIXED(CLK_12M,          "ln-clk-12m",     12288000),
> +       LN_CLK_FIXED(CLK_11M,          "ln-clk-11m",     11298600),
> +       LN_CLK_FIXED(CLK_24M,          "ln-clk-24m",     24576000),
> +       LN_CLK_FIXED(CLK_22M,          "ln-clk-22m",     22579200),
> +       LN_CLK_FIXED(USB_CLK_24M,      "ln-usb-clk-24m", 24000000),
> +       LN_CLK_FIXED(USB_CLK_12M,      "ln-usb-clk-12m", 12000000),

They sort of look like crystals or dividers on crystals. Let's move them
to DT?

> +};
> +
> +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->lochnagar->regmap;
> +       int ret;
> +
> +       dev_dbg(priv->dev, "Prepare %s\n", lclk->name);

Nitpick: Can you just rely on the clk tracepoints?

> +
> +       if (!lclk->regmap.ena_mask)

Why did we assign the ops to the clk if it can't be enabled? Please make
different ops for different types of clks so we don't have to check
something else and bail out early when the op is called.

> +               return 0;
> +
> +       ret = regmap_update_bits(regmap, lclk->regmap.cfg_reg,
> +                                lclk->regmap.ena_mask,
> +                                lclk->regmap.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->lochnagar->regmap;
> +       int ret;
> +
> +       dev_dbg(priv->dev, "Unprepare %s\n", lclk->name);
> +
> +       if (!lclk->regmap.ena_mask)
> +               return;
> +
> +       ret = regmap_update_bits(regmap, lclk->regmap.cfg_reg,
> +                                lclk->regmap.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->lochnagar->regmap;
> +       int ret;
> +
> +       dev_dbg(priv->dev, "Reparent %s to %s\n",
> +               lclk->name, priv->parents[index]);
> +
> +       if (lclk->regmap.dir_mask) {
> +               ret = regmap_update_bits(regmap, lclk->regmap.cfg_reg,
> +                                        lclk->regmap.dir_mask,
> +                                        lclk->regmap.dir_mask);
> +               if (ret < 0) {
> +                       dev_err(priv->dev, "Failed to set %s direction: %d\n",

What does direction mean?

> +                               lclk->name, ret);
> +                       return ret;
> +               }
> +       }
> +
> +       ret = regmap_update_bits(regmap, lclk->regmap.src_reg,
> +                                lclk->regmap.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->lochnagar->regmap;
> +       unsigned int val;
> +       int ret;
> +
> +       ret = regmap_read(regmap, lclk->regmap.src_reg, &val);
> +       if (ret < 0) {
> +               dev_err(priv->dev, "Failed to read parent of %s: %d\n",
> +                       lclk->name, ret);
> +               return 0;

Return a number greater than all possible parents of this clk?

> +       }
> +
> +       val &= lclk->regmap.src_mask;
> +
> +       dev_dbg(priv->dev, "Parent of %s is %s\n",
> +               lclk->name, priv->parents[val]);
> +
> +       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->lochnagar->dev->of_node;
> +       enum lochnagar_type type = priv->lochnagar->type;
> +       int i, j;
> +
> +       switch (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", 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) {
> +                       const char * const name = of_clk_get_parent_name(np, j);
> +
> +                       dev_dbg(priv->dev, "Set parent %s to %s\n",
> +                               priv->parents[i], name);
> +
> +                       priv->parents[i] = name;
> +               }
> +       }
> +
> +       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;
> +       struct clk *clk;
> +       int ret, i;
> +
> +       for (i = 0; i < ARRAY_SIZE(priv->lclks); i++) {
> +               lclk = &priv->lclks[i];
> +
> +               lclk->priv = priv;
> +
> +               switch (lclk->type) {
> +               case LOCHNAGAR_CLK_TYPE_FIXED:
> +                       clk = clk_register_fixed_rate(priv->dev, lclk->name,
> +                                                     NULL, 0,
> +                                                     lclk->fixed.rate);
> +                       break;
> +               case LOCHNAGAR_CLK_TYPE_REGMAP:
> +                       clk_init.name = lclk->name;
> +                       lclk->hw.init = &clk_init;
> +
> +                       clk = devm_clk_register(priv->dev, &lclk->hw);

Please use the clk_hw based registration APIs.

> +                       break;
> +               default:
> +                       continue;
> +               }
> +
> +               if (IS_ERR(clk)) {
> +                       ret = PTR_ERR(clk);
> +                       dev_err(priv->dev, "Failed to register %s: %d\n",
> +                               lclk->name, ret);
> +                       return ret;
> +               }
> +
> +               dev_dbg(priv->dev, "Registered %s\n", lclk->name);
> +
> +               priv->clks[i] = clk;
> +       }
> +
> +       return 0;
> +}
> +
> +static int lochnagar_init_of_providers(struct lochnagar_clk_priv *priv)
> +{
> +       struct device *dev = priv->dev;
> +       int ret;
> +
> +       priv->of_clks.clks = priv->clks;
> +       priv->of_clks.clk_num = ARRAY_SIZE(priv->clks);
> +
> +       ret = of_clk_add_provider(priv->lochnagar->dev->of_node,

Same here, clk_hw based provider APIs.

> +                                 of_clk_src_onecell_get,
> +                                 &priv->of_clks);
> +       if (ret < 0) {
> +               dev_err(dev, "Failed to register clock provider: %d\n", ret);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static int lochnagar_clk_probe(struct platform_device *pdev)
> +{
> +       struct lochnagar *lochnagar = dev_get_drvdata(pdev->dev.parent);
> +       struct device *dev = &pdev->dev;
> +       struct lochnagar_clk_priv *priv;
> +       int ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->dev = dev;
> +       priv->lochnagar = lochnagar;
> +
> +       ret = lochnagar_init_parents(priv);
> +       if (ret)
> +               return ret;
> +
> +       ret = lochnagar_init_clks(priv);
> +       if (ret)
> +               return ret;
> +
> +       ret = lochnagar_init_of_providers(priv);
> +       if (ret)
> +               return ret;
> +
> +       platform_set_drvdata(pdev, priv);
> +
> +       return 0;
> +}
> +
> +static int lochnagar_clk_remove(struct platform_device *pdev)
> +{
> +       struct lochnagar_clk_priv *priv = platform_get_drvdata(pdev);
> +       int i;
> +
> +       of_clk_del_provider(priv->lochnagar->dev->of_node);

Use devm variant of clk provider registration?

> +
> +       for (i = 0; i < ARRAY_SIZE(priv->lclks); i++) {
> +               switch (priv->lclks[i].type) {
> +               case LOCHNAGAR_CLK_TYPE_FIXED:

Hopefully fixed goes away so we don't need case statements and this code
here.

> +                       clk_unregister_fixed_rate(priv->clks[i]);
> +                       break;
> +               default:
> +                       break;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static struct platform_driver lochnagar_clk_driver = {
> +       .driver = {
> +               .name = "lochnagar-clk",
> +       },

Any id_table?

> +
> +       .probe = lochnagar_clk_probe,
> +       .remove = lochnagar_clk_remove,
> +};
> +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");
Charles Keepax Oct. 11, 2018, 1:26 p.m. | #2
On Thu, Oct 11, 2018 at 12:00:46AM -0700, Stephen Boyd wrote:
> Quoting Charles Keepax (2018-10-08 06:25:40)
> > +#include <linux/clk.h>
> 
> Is this used?
> 
> > +#include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> 
> Is this used?
> 
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> 
> Used?

Apparently not, will remove the three of them.

> > +struct lochnagar_regmap_clk {
> > +       unsigned int cfg_reg;
> > +       unsigned int ena_mask;
> > +       unsigned int dir_mask;
> > +
> > +       unsigned int src_reg;
> > +       unsigned int src_mask;
> 
> Are these 32 bits or 16 bits or 8 bits? Please use a u32/u16/u8 so we
> know the register width.
> 

Can do.

> > +struct lochnagar_clk_priv {
> > +       struct device *dev;
> > +       struct lochnagar *lochnagar;
> 
> Is this used for anything besides getting the regmap? Can you get the
> pointer to the parent in probe and use that to get the regmap pointer
> from dev_get_remap() and also use the of_node of the parent to register
> a clk provider? It would be nice to avoid including the mfd header file
> unless it's providing something useful.
> 

It is also used to find out which type of Lochnagar we have
connected, which determines which clocks we should register. I
could perhaps pass that using another mechanism but we would
still want to include the MFD stuff to get the register
definitions. So this approach seems simplest.

> > +#define LN_CLK_FIXED(ID, NAME, RATE) \
> > +       [LOCHNAGAR_##ID] = { \
> > +               .name = NAME, .type = LOCHNAGAR_CLK_TYPE_FIXED, \
> > +               { .fixed.rate = RATE, }, \
> > +       }
> 
> Can all these fixed clks come from DT instead of being populated by this
> driver? Unless they're being generated by this hardware block and not
> actually a crystal or some other on-board clock that goes into this
> hardware block and then right out of it?
> 

Technically they are supplied by a clock generator chip (bunch of
PLLs) which is controlled by the board controller chip. That said
I don't really ever see them changing so will move them in DT.

> > +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->lochnagar->regmap;
> > +       int ret;
> > +
> > +       dev_dbg(priv->dev, "Prepare %s\n", lclk->name);
> 
> Nitpick: Can you just rely on the clk tracepoints?
> 

Can do they are hardly essential.

> > +
> > +       if (!lclk->regmap.ena_mask)
> 
> Why did we assign the ops to the clk if it can't be enabled? Please make
> different ops for different types of clks so we don't have to check
> something else and bail out early when the op is called.
> 

This is a bit of a leak from earlier versions of the code I
can just remove them.

> > +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->lochnagar->regmap;
> > +       int ret;
> > +
> > +       dev_dbg(priv->dev, "Reparent %s to %s\n",
> > +               lclk->name, priv->parents[index]);
> > +
> > +       if (lclk->regmap.dir_mask) {
> > +               ret = regmap_update_bits(regmap, lclk->regmap.cfg_reg,
> > +                                        lclk->regmap.dir_mask,
> > +                                        lclk->regmap.dir_mask);
> > +               if (ret < 0) {
> > +                       dev_err(priv->dev, "Failed to set %s direction: %d\n",
> 
> What does direction mean?
> 

Some of the clocks can both generate and receive a clock. For
example the PSIA (external audio interface) MCLKs, the attached
device could be expecting or providing a master audio clock. If
the user assigns a parent to the clock we assume the attached
device is providing a clock to us, otherwise we assume we are
providing the clock.

> > +       ret = regmap_read(regmap, lclk->regmap.src_reg, &val);
> > +       if (ret < 0) {
> > +               dev_err(priv->dev, "Failed to read parent of %s: %d\n",
> > +                       lclk->name, ret);
> > +               return 0;
> 
> Return a number greater than all possible parents of this clk?
> 

Can do.

> > +       for (i = 0; i < ARRAY_SIZE(priv->lclks); i++) {
> > +               lclk = &priv->lclks[i];
> > +
> > +               lclk->priv = priv;
> > +
> > +               switch (lclk->type) {
> > +               case LOCHNAGAR_CLK_TYPE_FIXED:
> > +                       clk = clk_register_fixed_rate(priv->dev, lclk->name,
> > +                                                     NULL, 0,
> > +                                                     lclk->fixed.rate);
> > +                       break;
> > +               case LOCHNAGAR_CLK_TYPE_REGMAP:
> > +                       clk_init.name = lclk->name;
> > +                       lclk->hw.init = &clk_init;
> > +
> > +                       clk = devm_clk_register(priv->dev, &lclk->hw);
> 
> Please use the clk_hw based registration APIs.
> 

Can do.

> > +static int lochnagar_clk_remove(struct platform_device *pdev)
> > +{
> > +       struct lochnagar_clk_priv *priv = platform_get_drvdata(pdev);
> > +       int i;
> > +
> > +       of_clk_del_provider(priv->lochnagar->dev->of_node);
> 
> Use devm variant of clk provider registration?
> 

This isn't using the devm version as it makes the assumption
that the device that contains the DT is the same one we want
to devm against which isn't the case here.

I could update probe to copy the of_node over from the parent, if
you prefer? I think that would also work.

> > +static struct platform_driver lochnagar_clk_driver = {
> > +       .driver = {
> > +               .name = "lochnagar-clk",
> > +       },
> 
> Any id_table?
> 

Doesn't really need one since the driver will only ever be bound
in by the MFD, so matching on the driver name is fine.

Thanks,
Charles
Mark Brown Oct. 11, 2018, 2:54 p.m. | #3
On Thu, Oct 11, 2018 at 12:00:46AM -0700, Stephen Boyd wrote:
> Quoting Charles Keepax (2018-10-08 06:25:40)

> > +struct lochnagar_regmap_clk {
> > +       unsigned int cfg_reg;
> > +       unsigned int ena_mask;
> > +       unsigned int dir_mask;
> > +
> > +       unsigned int src_reg;
> > +       unsigned int src_mask;

> Are these 32 bits or 16 bits or 8 bits? Please use a u32/u16/u8 so we
> know the register width.

Note that regmap always uses unsigned int to represent registers
regardless of the underlying physical register width, this can be an
issue on reads since we pass the destination for the read in by address.

> > +struct lochnagar_clk_priv {
> > +       struct device *dev;
> > +       struct lochnagar *lochnagar;

> Is this used for anything besides getting the regmap? Can you get the
> pointer to the parent in probe and use that to get the regmap pointer
> from dev_get_remap() and also use the of_node of the parent to register

dev_get_regmap() is pretty expensive, I'd not advise using it in
anything that might approximate a fast path.  It's kind of debatable
when I2C gets involved but still feels wrong.
Stephen Boyd Oct. 11, 2018, 7:36 p.m. | #4
Quoting Mark Brown (2018-10-11 07:54:22)
> On Thu, Oct 11, 2018 at 12:00:46AM -0700, Stephen Boyd wrote:
> > Quoting Charles Keepax (2018-10-08 06:25:40)
> 
> > > +struct lochnagar_regmap_clk {
> > > +       unsigned int cfg_reg;
> > > +       unsigned int ena_mask;
> > > +       unsigned int dir_mask;
> > > +
> > > +       unsigned int src_reg;
> > > +       unsigned int src_mask;
> 
> > Are these 32 bits or 16 bits or 8 bits? Please use a u32/u16/u8 so we
> > know the register width.
> 
> Note that regmap always uses unsigned int to represent registers
> regardless of the underlying physical register width, this can be an
> issue on reads since we pass the destination for the read in by address.
> 
> > > +struct lochnagar_clk_priv {
> > > +       struct device *dev;
> > > +       struct lochnagar *lochnagar;
> 
> > Is this used for anything besides getting the regmap? Can you get the
> > pointer to the parent in probe and use that to get the regmap pointer
> > from dev_get_remap() and also use the of_node of the parent to register
> 
> dev_get_regmap() is pretty expensive, I'd not advise using it in
> anything that might approximate a fast path.  It's kind of debatable
> when I2C gets involved but still feels wrong.

I'm suggesting the regmap is acquired in probe and a pointer is stored
here in this structure. That is not a fastpath as far as I know.
Stephen Boyd Oct. 12, 2018, 3:59 p.m. | #5
Quoting Charles Keepax (2018-10-11 06:26:02)
> On Thu, Oct 11, 2018 at 12:00:46AM -0700, Stephen Boyd wrote:
> > Quoting Charles Keepax (2018-10-08 06:25:40)
> 
> > > +struct lochnagar_regmap_clk {
> > > +       unsigned int cfg_reg;
> > > +       unsigned int ena_mask;
> > > +       unsigned int dir_mask;
> > > +
> > > +       unsigned int src_reg;
> > > +       unsigned int src_mask;
> > 
> > Are these 32 bits or 16 bits or 8 bits? Please use a u32/u16/u8 so we
> > know the register width.
> > 
> 
> Can do.

Well Mark may be opposed so it doesn't really matter to me. Just
would be nice to have some more code documentation on the expected
register width.

> 
> > > +struct lochnagar_clk_priv {
> > > +       struct device *dev;
> > > +       struct lochnagar *lochnagar;
> > 
> > Is this used for anything besides getting the regmap? Can you get the
> > pointer to the parent in probe and use that to get the regmap pointer
> > from dev_get_remap() and also use the of_node of the parent to register
> > a clk provider? It would be nice to avoid including the mfd header file
> > unless it's providing something useful.
> > 
> 
> It is also used to find out which type of Lochnagar we have
> connected, which determines which clocks we should register. I

Can that be done through some device ID? So the driver can be untangled
from the MFD part.

> could perhaps pass that using another mechanism but we would
> still want to include the MFD stuff to get the register
> definitions. So this approach seems simplest.

Can the register definitions be moved to this clk driver?

Maybe you now get the hint, but I'd really like to be able to merge and
compile the clk driver all by itself without relying on the parent MFD
device to provide anything at compile time.

> 
> > > +#define LN_CLK_FIXED(ID, NAME, RATE) \
> > > +       [LOCHNAGAR_##ID] = { \
> > > +               .name = NAME, .type = LOCHNAGAR_CLK_TYPE_FIXED, \
> > > +               { .fixed.rate = RATE, }, \
> > > +       }
> > 
> > Can all these fixed clks come from DT instead of being populated by this
> > driver? Unless they're being generated by this hardware block and not
> > actually a crystal or some other on-board clock that goes into this
> > hardware block and then right out of it?
> > 
> 
> Technically they are supplied by a clock generator chip (bunch of
> PLLs) which is controlled by the board controller chip. That said
> I don't really ever see them changing so will move them in DT.
> 
> > > +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->lochnagar->regmap;
> > > +       int ret;
> > > +
> > > +       dev_dbg(priv->dev, "Prepare %s\n", lclk->name);
> > 
> > Nitpick: Can you just rely on the clk tracepoints?
> > 
> 
> Can do they are hardly essential.
> 
> > > +
> > > +       if (!lclk->regmap.ena_mask)
> > 
> > Why did we assign the ops to the clk if it can't be enabled? Please make
> > different ops for different types of clks so we don't have to check
> > something else and bail out early when the op is called.
> > 
> 
> This is a bit of a leak from earlier versions of the code I
> can just remove them.
> 
> > > +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->lochnagar->regmap;
> > > +       int ret;
> > > +
> > > +       dev_dbg(priv->dev, "Reparent %s to %s\n",
> > > +               lclk->name, priv->parents[index]);
> > > +
> > > +       if (lclk->regmap.dir_mask) {
> > > +               ret = regmap_update_bits(regmap, lclk->regmap.cfg_reg,
> > > +                                        lclk->regmap.dir_mask,
> > > +                                        lclk->regmap.dir_mask);
> > > +               if (ret < 0) {
> > > +                       dev_err(priv->dev, "Failed to set %s direction: %d\n",
> > 
> > What does direction mean?
> > 
> 
> Some of the clocks can both generate and receive a clock. For
> example the PSIA (external audio interface) MCLKs, the attached
> device could be expecting or providing a master audio clock. If
> the user assigns a parent to the clock we assume the attached
> device is providing a clock to us, otherwise we assume we are
> providing the clock.

And this directionality is determined by dir_mask? It would be great if
this sort of information was in the commit text or in a comment in the
driver so we know what's going on here.

> 
> > > +static int lochnagar_clk_remove(struct platform_device *pdev)
> > > +{
> > > +       struct lochnagar_clk_priv *priv = platform_get_drvdata(pdev);
> > > +       int i;
> > > +
> > > +       of_clk_del_provider(priv->lochnagar->dev->of_node);
> > 
> > Use devm variant of clk provider registration?
> > 
> 
> This isn't using the devm version as it makes the assumption
> that the device that contains the DT is the same one we want
> to devm against which isn't the case here.
> 
> I could update probe to copy the of_node over from the parent, if
> you prefer? I think that would also work.

I don't really mind either way. When we get these subdevice drivers of a
larger OF node for the whole MFD it becomes sort of awkward to make devm
work.
Mark Brown Oct. 12, 2018, 4:52 p.m. | #6
On Thu, Oct 11, 2018 at 12:36:22PM -0700, Stephen Boyd wrote:
> Quoting Mark Brown (2018-10-11 07:54:22)
> > On Thu, Oct 11, 2018 at 12:00:46AM -0700, Stephen Boyd wrote:

> > > Is this used for anything besides getting the regmap? Can you get the
> > > pointer to the parent in probe and use that to get the regmap pointer
> > > from dev_get_remap() and also use the of_node of the parent to register

> > dev_get_regmap() is pretty expensive, I'd not advise using it in
> > anything that might approximate a fast path.  It's kind of debatable
> > when I2C gets involved but still feels wrong.

> I'm suggesting the regmap is acquired in probe and a pointer is stored
> here in this structure. That is not a fastpath as far as I know.

Ah, great - that's the intended usage.
Rob Herring Oct. 12, 2018, 10:08 p.m. | #7
On Mon, Oct 08, 2018 at 02:25:38PM +0100, Charles Keepax wrote:
> 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.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> Changes since v1:
>  - Move the binding includes into this patch.
>  - Move patch to the start of the patch chain.
>  - Update commit message a little
> 
> Thanks,
> Charles
> 
>  .../devicetree/bindings/mfd/cirrus,lochnagar.txt   | 230 +++++++++++++++++++++
>  include/dt-bindings/clk/lochnagar.h                |  33 +++
>  include/dt-bindings/pinctrl/lochnagar.h            | 132 ++++++++++++
>  3 files changed, 395 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
>  create mode 100644 include/dt-bindings/clk/lochnagar.h
>  create mode 100644 include/dt-bindings/pinctrl/lochnagar.h

A few nits, otherwise:

Acked-by: Rob Herring <robh@kernel.org>

> 
> diff --git a/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt b/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
> new file mode 100644
> index 0000000000000..3382a14c19fe4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
> @@ -0,0 +1,230 @@
> +Cirrus Logic Lochnagar Audio Development Board
> +
> +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.  Audio system topology, clocking and power can all be
> +controlled through the Lochnagar, allowing the device under test
> +to be used in a variety of possible use cases.
> +
> +Also see these documents for generic binding information:
> +  [1] GPIO : ../gpio/gpio.txt
> +  [2] Pinctrl: ../pinctrl/pinctrl-bindings.txt
> +  [3] Clock : ../clock/clock-bindings.txt
> +  [4] Regulator: ../regulator/regulator.txt
> +
> +And these for relevant defines:
> +  [5] include/dt-bindings/pinctrl/lochnagar.h
> +  [6] include/dt-bindings/clock/lochnagar.h
> +
> +Required properties:
> +
> +  - compatible : One of the following strings:
> +                 "cirrus,lochnagar1"
> +                 "cirrus,lochnagar2"
> +
> +  - reg : I2C slave address
> +
> +  - gpio-controller : Indicates this device is a GPIO controller.
> +  - #gpio-cells : Must be 2. The first cell is the pin number, see
> +    [5] for available pins and the second cell is used to specify
> +    optional parameters, see [1].
> +  - gpio-ranges : Range of pins managed by the GPIO controller, see
> +    [1]. Both the GPIO and Pinctrl base should be set to zero and the
> +    count to the appropriate of the LOCHNAGARx_PIN_NUM_GPIOS define,
> +    see [5].
> +
> +  - reset-gpios : Reset line to the Lochnagar, see [1].
> +
> +  - #clock-cells : Must be 1. The first cell indicates the clock
> +    number, see [6] for available clocks and [3].
> +
> +  - pinctrl-names : A pinctrl state named "default" must be defined.
> +  - pinctrl-0 : A phandle to the default pinctrl state.
> +
> +  - SYSVDD-supply: Primary power supply for the Lochnagar.
> +
> +Optional properties:
> +
> +  - present-gpios : Host present line, indicating the presence of a
> +    host system, see [1]. This can be omitted if the present line is
> +    tied in hardware.
> +
> +  - clocks : Must contain an entry for each clock in clock-names.
> +  - clock-names : May contain entries for each of the following
> +    clocks:
> +     - ln-cdc-clkout : Output clock from CODEC card.
> +     - ln-dsp-clkout : Output clock from DSP card.
> +     - ln-gf-mclk1,ln-gf-mclk2,ln-gf-mclk3,ln-gf-mclk4 : Optional
> +       input audio clocks from host system.
> +     - ln-psia1-mclk, ln-psia2-mclk : Optional input audio clocks from
> +       external connector.
> +     - ln-spdif-clkout : Optional input audio clock from SPDIF.
> +     - ln-adat-clkout : Optional input audio clock from ADAT.
> +
> +  - assigned-clocks : A list of Lochnagar clocks to be reparented, see
> +    [6] for available clocks.
> +  - assigned-clock-parents : Parents to be assigned to the clocks
> +    listed in "assigned-clocks".
> +
> +  - MICBIAS1-supply, MICBIAS2-supply: Regulator supplies for the
> +    MICxVDD outputs, supplying the digital microphones, normally
> +    supplied from the attached CODEC.
> +
> +Required sub-nodes:
> +
> +The pin configurations are defined as a child of the pinctrl states
> +node, see [2]. Each sub-node can have the following properties:
> +  - groups : A list of groups to select (either this or "pins" must be
> +    specified), available groups:
> +      codec-aif1, codec-aif2, codec-aif3, dsp-aif1, dsp-aif2, psia1,
> +      psia2, gf-aif1, gf-aif2, gf-aif3, gf-aif4, spdif-aif, usb-aif1,
> +      usb-aif2, adat-aif, soundcard-aif
> +  - pins : A list of pin names to select (either this or "groups" must
> +    be specified), available pins:
> +      fgpa-gpio1, fgpa-gpio2, fgpa-gpio3, fgpa-gpio4, fgpa-gpio5,
> +      fgpa-gpio6, codec-gpio1, codec-gpio2, codec-gpio3, codec-gpio4,
> +      codec-gpio5, codec-gpio6, codec-gpio7, codec-gpio8, dsp-gpio1,
> +      dsp-gpio2, dsp-gpio3, dsp-gpio4, dsp-gpio5, dsp-gpio6, gf-gpio2,
> +      gf-gpio3, gf-gpio7, codec-aif1-bclk, codec-aif1-rxdat,
> +      codec-aif1-lrclk, codec-aif1-txdat, codec-aif2-bclk,
> +      codec-aif2-rxdat, codec-aif2-lrclk, codec-aif2-txdat,
> +      codec-aif3-bclk, codec-aif3-rxdat, codec-aif3-lrclk,
> +      codec-aif3-txdat, dsp-aif1-bclk, dsp-aif1-rxdat, dsp-aif1-lrclk,
> +      dsp-aif1-txdat, dsp-aif2-bclk, dsp-aif2-rxdat,
> +      dsp-aif2-lrclk, dsp-aif2-txdat, psia1-bclk, psia1-rxdat,
> +      psia1-lrclk, psia1-txdat, psia2-bclk, psia2-rxdat, psia2-lrclk,
> +      psia2-txdat, gf-aif3-bclk, gf-aif3-rxdat, gf-aif3-lrclk,
> +      gf-aif3-txdat, gf-aif4-bclk, gf-aif4-rxdat, gf-aif4-lrclk,
> +      gf-aif4-txdat, gf-aif1-bclk, gf-aif1-rxdat, gf-aif1-lrclk,
> +      gf-aif1-txdat, gf-aif2-bclk, gf-aif2-rxdat, gf-aif2-lrclk,
> +      gf-aif2-txdat, dsp-uart1-rx, dsp-uart1-tx, dsp-uart2-rx,
> +      dsp-uart2-tx, gf-uart2-rx, gf-uart2-tx, usb-uart-rx,
> +      codec-pdmclk1, codec-pdmdat1, codec-pdmclk2, codec-pdmdat2,
> +      codec-dmicclk1, codec-dmicdat1, codec-dmicclk2, codec-dmicdat2,
> +      codec-dmicclk3, codec-dmicdat3, codec-dmicclk4, codec-dmicdat4,
> +      dsp-dmicclk1, dsp-dmicdat1, dsp-dmicclk2, dsp-dmicdat2, i2c2-scl,
> +      i2c2-sda, i2c3-scl, i2c3-sda, i2c4-scl, i2c4-sda, dsp-standby,
> +      codec-mclk1, codec-mclk2, dsp-clkin, psia1-mclk, psia2-mclk,
> +      gf-gpio1, gf-gpio5, dsp-gpio20, led1, led2
> +  - function : The mux function to select, available functions:
> +      aif, fgpa-gpio1, fgpa-gpio2, fgpa-gpio3, fgpa-gpio4, fgpa-gpio5,
> +      fgpa-gpio6, codec-gpio1, codec-gpio2, codec-gpio3, codec-gpio4,
> +      codec-gpio5, codec-gpio6, codec-gpio7, codec-gpio8, dsp-gpio1,
> +      dsp-gpio2, dsp-gpio3, dsp-gpio4, dsp-gpio5, dsp-gpio6, gf-gpio2,
> +      gf-gpio3, gf-gpio7, gf-gpio1, gf-gpio5, dsp-gpio20, codec-clkout,
> +      dsp-clkout, pmic-32k, spdif-clkout, clk-12m288, clk-11m2986,
> +      clk-24m576, clk-22m5792, xmos-mclk, gf-clkout1, gf-mclk1,
> +      gf-mclk3, gf-mclk2, gf-clkout2, codec-mclk1, codec-mclk2,
> +      dsp-clkin, psia1-mclk, psia2-mclk, spdif-mclk, codec-irq,
> +      codec-reset, dsp-reset, dsp-irq, dsp-standby, codec-pdmclk1,
> +      codec-pdmdat1, codec-pdmclk2, codec-pdmdat2, codec-dmicclk1,
> +      codec-dmicdat1, codec-dmicclk2, codec-dmicdat2, codec-dmicclk3,
> +      codec-dmicdat3, codec-dmicclk4, codec-dmicdat4, dsp-dmicclk1,
> +      dsp-dmicdat1, dsp-dmicclk2, dsp-dmicdat2, dsp-uart1-rx,
> +      dsp-uart1-tx, dsp-uart2-rx, dsp-uart2-tx, gf-uart2-rx,
> +      gf-uart2-tx, usb-uart-rx, usb-uart-tx, i2c2-scl, i2c2-sda,
> +      i2c3-scl, i2c3-sda, i2c4-scl, i2c4-sda, gpio, spdif-aif, psia1,
> +      psia1-bclk, psia1-lrclk, psia1-rxdat, psia1-txdat, psia2,
> +      psia2-bclk, psia2-lrclk, psia2-rxdat, psia2-txdat, codec-aif1,
> +      codec-aif1-bclk, codec-aif1-lrclk, codec-aif1-rxdat,
> +      codec-aif1-txdat, codec-aif2, codec-aif2-bclk, codec-aif2-lrclk,
> +      codec-aif2-rxdat, codec-aif2-txdat, codec-aif3, codec-aif3-bclk,
> +      codec-aif3-lrclk, codec-aif3-rxdat, codec-aif3-txdat, dsp-aif1,
> +      dsp-aif1-bclk, dsp-aif1-lrclk, dsp-aif1-rxdat, dsp-aif1-txdat,
> +      dsp-aif2, dsp-aif2-bclk, dsp-aif2-lrclk, dsp-aif2-rxdat,
> +      dsp-aif2-txdat, gf-aif3, gf-aif3-bclk, gf-aif3-lrclk,
> +      gf-aif3-rxdat, gf-aif3-txdat, gf-aif4, gf-aif4-bclk,
> +      gf-aif4-lrclk, gf-aif4-rxdat, gf-aif4-txdat, gf-aif1,
> +      gf-aif1-bclk, gf-aif1-lrclk, gf-aif1-rxdat, gf-aif1-txdat,
> +      gf-aif2, gf-aif2-bclk, gf-aif2-lrclk, gf-aif2-rxdat,
> +      gf-aif2-txdat, usb-aif1, usb-aif2, adat-aif, soundcard-aif,
> +
> +  - aif-master : Specifies that an AIF group will be used as a master
> +    interface (either this or aif-slave is required if a group is
> +    being muxed to an AIF)
> +  - aif-slave : Specifies that an AIF group will be used as a slave
> +    interface (either this or aif-master is required if a group is
> +    being muxed to an AIF)

Vendor prefix needed on these.

> +
> +Optional sub-nodes:
> +
> +  - VDDCORE : Initialisation data for the VDDCORE regulator, which
> +    supplies the CODECs digital core if it has no build regulator for that
> +    purpose.
> +
> +  - MICVDD : Initialisation data for the MICVDD regulator, which
> +    supplies the CODECs MICVDD.
> +  - MIC1VDD, MIC2VDD : Initialisation data for the MICxVDD supplies.
> +      Optional Properties:
> +      - cirrus,micbias-input : A property selecting which of the CODEC
> +        minicard micbias outputs should be used, valid values are 1 - 4.
> +
> +  - VDD1V8 : Recommended fixed regulator for the VDD1V8 regulator, which supplies the
> +    CODECs analog and 1.8V digital supplies.
> +      Required Properties:
> +      - compatible : Should be set to "regulator-fixed"
> +      - regulator-min-microvolt : Should be set to 1.8V
> +      - regulator-max-microvolt : Should be set to 1.8V
> +      - regulator-boot-on
> +      - regulator-always-on
> +      - vin-supply : Should be set to same supply as SYSVDD
> +
> +Example:
> +lochnagar: lochnagar@22 {
> +	compatible = "cirrus,lochnagar2";
> +	reg = <0x22>;
> +
> +	gpio-controller;
> +	#gpio-cells = <2>;
> +	gpio-ranges = <&lochnagar 0 0 110>;
> +
> +	reset-gpio = <&gpio0 55 0>;
> +	present-gpio = <&gpio0 60 0>;

-gpios

> +
> +	#clock-cells = <1>;
> +
> +	clocks = <&clk_audio>;
> +	clock-names = "ln-gf-mclk2";
> +
> +	assigned-clocks = <&lochnagar LOCHNAGAR_CDC_MCLK1>,
> +			  <&lochnagar LOCHNAGAR_CDC_MCLK2>;
> +	assigned-clock-parents = <&clk_audio>,
> +				 <&lochnagar LOCHNAGAR_PMIC_32K>;
> +
> +	SYSVDD-supply = <&wallvdd>;
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pin_settings>;
> +
> +	pin_settings: pin_settings {
> +		ap_aif {
> +			aif-slave;

Use '-' rather than '_' on node names.

> +			groups = "gf-aif1";
> +			function = "codec-aif3";
> +		};
> +		codec_aif {
> +			aif-master;
> +			groups = "codec-aif3";
> +			function = "gf-aif1";
> +		};
> +	};
> +
> +	lochnagar_micvdd: MICVDD {
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +	};
> +
> +	lochnagar_vdd1v8: VDD1V8 {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VDD1V8";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		regulator-boot-on;
> +		regulator-always-on;
> +
> +		vin-supply = <&wallvdd>;
> +	};
> +};
> diff --git a/include/dt-bindings/clk/lochnagar.h b/include/dt-bindings/clk/lochnagar.h
> new file mode 100644
> index 0000000000000..caeeec7dfbf6d
> --- /dev/null
> +++ b/include/dt-bindings/clk/lochnagar.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Device Tree defines for Lochnagar clocking
> + *
> + * Copyright (c) 2017-2018 Cirrus Logic, Inc. and
> + *                         Cirrus Logic International Semiconductor Ltd.
> + *
> + * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
> + */
> +
> +#ifndef DT_BINDINGS_CLK_LOCHNAGAR_H
> +#define DT_BINDINGS_CLK_LOCHNAGAR_H
> +
> +#define LOCHNAGAR_CDC_MCLK1		0
> +#define LOCHNAGAR_CDC_MCLK2		1
> +#define LOCHNAGAR_DSP_CLKIN		2
> +#define LOCHNAGAR_GF_CLKOUT1		3
> +#define LOCHNAGAR_GF_CLKOUT2		4
> +#define LOCHNAGAR_PSIA1_MCLK		5
> +#define LOCHNAGAR_PSIA2_MCLK		6
> +#define LOCHNAGAR_SPDIF_MCLK		7
> +#define LOCHNAGAR_ADAT_MCLK		8
> +#define LOCHNAGAR_SOUNDCARD_MCLK	9
> +#define LOCHNAGAR_PMIC_32K		10
> +#define LOCHNAGAR_CLK_12M		11
> +#define LOCHNAGAR_CLK_11M		12
> +#define LOCHNAGAR_CLK_24M		13
> +#define LOCHNAGAR_CLK_22M		14
> +#define LOCHNAGAR_USB_CLK_24M		15
> +#define LOCHNAGAR_USB_CLK_12M		16
> +#define LOCHNAGAR_SPDIF_CLKOUT		17
> +
> +#endif
> diff --git a/include/dt-bindings/pinctrl/lochnagar.h b/include/dt-bindings/pinctrl/lochnagar.h
> new file mode 100644
> index 0000000000000..644760bf5725b
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/lochnagar.h
> @@ -0,0 +1,132 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Device Tree defines for Lochnagar pinctrl
> + *
> + * Copyright (c) 2018 Cirrus Logic, Inc. and
> + *                    Cirrus Logic International Semiconductor Ltd.
> + *
> + * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
> + */
> +
> +#ifndef DT_BINDINGS_PINCTRL_LOCHNAGAR_H
> +#define DT_BINDINGS_PINCTRL_LOCHNAGAR_H
> +
> +#define LOCHNAGAR1_PIN_CDC_RESET		0
> +#define LOCHNAGAR1_PIN_DSP_RESET		1
> +#define LOCHNAGAR1_PIN_CDC_CIF1MODE		2
> +#define LOCHNAGAR1_PIN_NUM_GPIOS		3
> +
> +#define LOCHNAGAR2_PIN_CDC_RESET		0
> +#define LOCHNAGAR2_PIN_DSP_RESET		1
> +#define LOCHNAGAR2_PIN_CDC_CIF1MODE		2
> +#define LOCHNAGAR2_PIN_CDC_LDOENA		3
> +#define LOCHNAGAR2_PIN_SPDIF_HWMODE		4
> +#define LOCHNAGAR2_PIN_SPDIF_RESET		5
> +#define LOCHNAGAR2_PIN_FPGA_GPIO1		6
> +#define LOCHNAGAR2_PIN_FPGA_GPIO2		7
> +#define LOCHNAGAR2_PIN_FPGA_GPIO3		8
> +#define LOCHNAGAR2_PIN_FPGA_GPIO4		9
> +#define LOCHNAGAR2_PIN_FPGA_GPIO5		10
> +#define LOCHNAGAR2_PIN_FPGA_GPIO6		11
> +#define LOCHNAGAR2_PIN_CDC_GPIO1		12
> +#define LOCHNAGAR2_PIN_CDC_GPIO2		13
> +#define LOCHNAGAR2_PIN_CDC_GPIO3		14
> +#define LOCHNAGAR2_PIN_CDC_GPIO4		15
> +#define LOCHNAGAR2_PIN_CDC_GPIO5		16
> +#define LOCHNAGAR2_PIN_CDC_GPIO6		17
> +#define LOCHNAGAR2_PIN_CDC_GPIO7		18
> +#define LOCHNAGAR2_PIN_CDC_GPIO8		19
> +#define LOCHNAGAR2_PIN_DSP_GPIO1		20
> +#define LOCHNAGAR2_PIN_DSP_GPIO2		21
> +#define LOCHNAGAR2_PIN_DSP_GPIO3		22
> +#define LOCHNAGAR2_PIN_DSP_GPIO4		23
> +#define LOCHNAGAR2_PIN_DSP_GPIO5		24
> +#define LOCHNAGAR2_PIN_DSP_GPIO6		25
> +#define LOCHNAGAR2_PIN_GF_GPIO2			26
> +#define LOCHNAGAR2_PIN_GF_GPIO3			27
> +#define LOCHNAGAR2_PIN_GF_GPIO7			28
> +#define LOCHNAGAR2_PIN_CDC_AIF1_BCLK		29
> +#define LOCHNAGAR2_PIN_CDC_AIF1_RXDAT		30
> +#define LOCHNAGAR2_PIN_CDC_AIF1_LRCLK		31
> +#define LOCHNAGAR2_PIN_CDC_AIF1_TXDAT		32
> +#define LOCHNAGAR2_PIN_CDC_AIF2_BCLK		33
> +#define LOCHNAGAR2_PIN_CDC_AIF2_RXDAT		34
> +#define LOCHNAGAR2_PIN_CDC_AIF2_LRCLK		35
> +#define LOCHNAGAR2_PIN_CDC_AIF2_TXDAT		36
> +#define LOCHNAGAR2_PIN_CDC_AIF3_BCLK		37
> +#define LOCHNAGAR2_PIN_CDC_AIF3_RXDAT		38
> +#define LOCHNAGAR2_PIN_CDC_AIF3_LRCLK		39
> +#define LOCHNAGAR2_PIN_CDC_AIF3_TXDAT		40
> +#define LOCHNAGAR2_PIN_DSP_AIF1_BCLK		41
> +#define LOCHNAGAR2_PIN_DSP_AIF1_RXDAT		42
> +#define LOCHNAGAR2_PIN_DSP_AIF1_LRCLK		43
> +#define LOCHNAGAR2_PIN_DSP_AIF1_TXDAT		44
> +#define LOCHNAGAR2_PIN_DSP_AIF2_BCLK		45
> +#define LOCHNAGAR2_PIN_DSP_AIF2_RXDAT		46
> +#define LOCHNAGAR2_PIN_DSP_AIF2_LRCLK		47
> +#define LOCHNAGAR2_PIN_DSP_AIF2_TXDAT		48
> +#define LOCHNAGAR2_PIN_PSIA1_BCLK		49
> +#define LOCHNAGAR2_PIN_PSIA1_RXDAT		50
> +#define LOCHNAGAR2_PIN_PSIA1_LRCLK		51
> +#define LOCHNAGAR2_PIN_PSIA1_TXDAT		52
> +#define LOCHNAGAR2_PIN_PSIA2_BCLK		53
> +#define LOCHNAGAR2_PIN_PSIA2_RXDAT		54
> +#define LOCHNAGAR2_PIN_PSIA2_LRCLK		55
> +#define LOCHNAGAR2_PIN_PSIA2_TXDAT		56
> +#define LOCHNAGAR2_PIN_GF_AIF3_BCLK		57
> +#define LOCHNAGAR2_PIN_GF_AIF3_RXDAT		58
> +#define LOCHNAGAR2_PIN_GF_AIF3_LRCLK		59
> +#define LOCHNAGAR2_PIN_GF_AIF3_TXDAT		60
> +#define LOCHNAGAR2_PIN_GF_AIF4_BCLK		61
> +#define LOCHNAGAR2_PIN_GF_AIF4_RXDAT		62
> +#define LOCHNAGAR2_PIN_GF_AIF4_LRCLK		63
> +#define LOCHNAGAR2_PIN_GF_AIF4_TXDAT		64
> +#define LOCHNAGAR2_PIN_GF_AIF1_BCLK		65
> +#define LOCHNAGAR2_PIN_GF_AIF1_RXDAT		66
> +#define LOCHNAGAR2_PIN_GF_AIF1_LRCLK		67
> +#define LOCHNAGAR2_PIN_GF_AIF1_TXDAT		68
> +#define LOCHNAGAR2_PIN_GF_AIF2_BCLK		69
> +#define LOCHNAGAR2_PIN_GF_AIF2_RXDAT		70
> +#define LOCHNAGAR2_PIN_GF_AIF2_LRCLK		71
> +#define LOCHNAGAR2_PIN_GF_AIF2_TXDAT		72
> +#define LOCHNAGAR2_PIN_DSP_UART1_RX		73
> +#define LOCHNAGAR2_PIN_DSP_UART1_TX		74
> +#define LOCHNAGAR2_PIN_DSP_UART2_RX		75
> +#define LOCHNAGAR2_PIN_DSP_UART2_TX		76
> +#define LOCHNAGAR2_PIN_GF_UART2_RX		77
> +#define LOCHNAGAR2_PIN_GF_UART2_TX		78
> +#define LOCHNAGAR2_PIN_USB_UART_RX		79
> +#define LOCHNAGAR2_PIN_CDC_PDMCLK1		80
> +#define LOCHNAGAR2_PIN_CDC_PDMDAT1		81
> +#define LOCHNAGAR2_PIN_CDC_PDMCLK2		82
> +#define LOCHNAGAR2_PIN_CDC_PDMDAT2		83
> +#define LOCHNAGAR2_PIN_CDC_DMICCLK1		84
> +#define LOCHNAGAR2_PIN_CDC_DMICDAT1		85
> +#define LOCHNAGAR2_PIN_CDC_DMICCLK2		86
> +#define LOCHNAGAR2_PIN_CDC_DMICDAT2		87
> +#define LOCHNAGAR2_PIN_CDC_DMICCLK3		88
> +#define LOCHNAGAR2_PIN_CDC_DMICDAT3		89
> +#define LOCHNAGAR2_PIN_CDC_DMICCLK4		90
> +#define LOCHNAGAR2_PIN_CDC_DMICDAT4		91
> +#define LOCHNAGAR2_PIN_DSP_DMICCLK1		92
> +#define LOCHNAGAR2_PIN_DSP_DMICDAT1		93
> +#define LOCHNAGAR2_PIN_DSP_DMICCLK2		94
> +#define LOCHNAGAR2_PIN_DSP_DMICDAT2		95
> +#define LOCHNAGAR2_PIN_I2C2_SCL			96
> +#define LOCHNAGAR2_PIN_I2C2_SDA			97
> +#define LOCHNAGAR2_PIN_I2C3_SCL			98
> +#define LOCHNAGAR2_PIN_I2C3_SDA			99
> +#define LOCHNAGAR2_PIN_I2C4_SCL			100
> +#define LOCHNAGAR2_PIN_I2C4_SDA			101
> +#define LOCHNAGAR2_PIN_DSP_STANDBY		102
> +#define LOCHNAGAR2_PIN_CDC_MCLK1		103
> +#define LOCHNAGAR2_PIN_CDC_MCLK2		104
> +#define LOCHNAGAR2_PIN_DSP_CLKIN		105
> +#define LOCHNAGAR2_PIN_PSIA1_MCLK		106
> +#define LOCHNAGAR2_PIN_PSIA2_MCLK		107
> +#define LOCHNAGAR2_PIN_GF_GPIO1			108
> +#define LOCHNAGAR2_PIN_GF_GPIO5			109
> +#define LOCHNAGAR2_PIN_DSP_GPIO20		110
> +#define LOCHNAGAR2_PIN_NUM_GPIOS		111
> +
> +#endif
> -- 
> 2.11.0
>
Charles Keepax Oct. 15, 2018, 10:49 a.m. | #8
On Fri, Oct 12, 2018 at 08:59:56AM -0700, Stephen Boyd wrote:
> Quoting Charles Keepax (2018-10-11 06:26:02)
> > On Thu, Oct 11, 2018 at 12:00:46AM -0700, Stephen Boyd wrote:
> > > Quoting Charles Keepax (2018-10-08 06:25:40)
> > > > +struct lochnagar_clk_priv {
> > > > +       struct device *dev;
> > > > +       struct lochnagar *lochnagar;
> > > 
> > > Is this used for anything besides getting the regmap? Can you get the
> > > pointer to the parent in probe and use that to get the regmap pointer
> > > from dev_get_remap() and also use the of_node of the parent to register
> > > a clk provider? It would be nice to avoid including the mfd header file
> > > unless it's providing something useful.
> > > 
> > 
> > It is also used to find out which type of Lochnagar we have
> > connected, which determines which clocks we should register. I
> 
> Can that be done through some device ID? So the driver can be untangled
> from the MFD part.
> 
> > could perhaps pass that using another mechanism but we would
> > still want to include the MFD stuff to get the register
> > definitions. So this approach seems simplest.
> 
> Can the register definitions be moved to this clk driver?
> 
> Maybe you now get the hint, but I'd really like to be able to merge and
> compile the clk driver all by itself without relying on the parent MFD
> device to provide anything at compile time.
> 

If you feel strongly but since the MFD needs to hold the regmap
(which needs to define the read/volatile regs and defaults)
these will need to be duplicate defines and personally i would
rather only have one copy as it makes updating things much less
error prone.

> > > > +       if (lclk->regmap.dir_mask) {
> > > > +               ret = regmap_update_bits(regmap, lclk->regmap.cfg_reg,
> > > > +                                        lclk->regmap.dir_mask,
> > > > +                                        lclk->regmap.dir_mask);
> > > > +               if (ret < 0) {
> > > > +                       dev_err(priv->dev, "Failed to set %s direction: %d\n",
> > > 
> > > What does direction mean?
> > > 
> > 
> > Some of the clocks can both generate and receive a clock. For
> > example the PSIA (external audio interface) MCLKs, the attached
> > device could be expecting or providing a master audio clock. If
> > the user assigns a parent to the clock we assume the attached
> > device is providing a clock to us, otherwise we assume we are
> > providing the clock.
> 
> And this directionality is determined by dir_mask? It would be great if
> this sort of information was in the commit text or in a comment in the
> driver so we know what's going on here.
> 

No problem will make this more clear.

Thanks,
Charles

Patch

diff --git a/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt b/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
new file mode 100644
index 0000000000000..3382a14c19fe4
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
@@ -0,0 +1,230 @@ 
+Cirrus Logic Lochnagar Audio Development Board
+
+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.  Audio system topology, clocking and power can all be
+controlled through the Lochnagar, allowing the device under test
+to be used in a variety of possible use cases.
+
+Also see these documents for generic binding information:
+  [1] GPIO : ../gpio/gpio.txt
+  [2] Pinctrl: ../pinctrl/pinctrl-bindings.txt
+  [3] Clock : ../clock/clock-bindings.txt
+  [4] Regulator: ../regulator/regulator.txt
+
+And these for relevant defines:
+  [5] include/dt-bindings/pinctrl/lochnagar.h
+  [6] include/dt-bindings/clock/lochnagar.h
+
+Required properties:
+
+  - compatible : One of the following strings:
+                 "cirrus,lochnagar1"
+                 "cirrus,lochnagar2"
+
+  - reg : I2C slave address
+
+  - gpio-controller : Indicates this device is a GPIO controller.
+  - #gpio-cells : Must be 2. The first cell is the pin number, see
+    [5] for available pins and the second cell is used to specify
+    optional parameters, see [1].
+  - gpio-ranges : Range of pins managed by the GPIO controller, see
+    [1]. Both the GPIO and Pinctrl base should be set to zero and the
+    count to the appropriate of the LOCHNAGARx_PIN_NUM_GPIOS define,
+    see [5].
+
+  - reset-gpios : Reset line to the Lochnagar, see [1].
+
+  - #clock-cells : Must be 1. The first cell indicates the clock
+    number, see [6] for available clocks and [3].
+
+  - pinctrl-names : A pinctrl state named "default" must be defined.
+  - pinctrl-0 : A phandle to the default pinctrl state.
+
+  - SYSVDD-supply: Primary power supply for the Lochnagar.
+
+Optional properties:
+
+  - present-gpios : Host present line, indicating the presence of a
+    host system, see [1]. This can be omitted if the present line is
+    tied in hardware.
+
+  - clocks : Must contain an entry for each clock in clock-names.
+  - clock-names : May contain entries for each of the following
+    clocks:
+     - ln-cdc-clkout : Output clock from CODEC card.
+     - ln-dsp-clkout : Output clock from DSP card.
+     - ln-gf-mclk1,ln-gf-mclk2,ln-gf-mclk3,ln-gf-mclk4 : Optional
+       input audio clocks from host system.
+     - ln-psia1-mclk, ln-psia2-mclk : Optional input audio clocks from
+       external connector.
+     - ln-spdif-clkout : Optional input audio clock from SPDIF.
+     - ln-adat-clkout : Optional input audio clock from ADAT.
+
+  - assigned-clocks : A list of Lochnagar clocks to be reparented, see
+    [6] for available clocks.
+  - assigned-clock-parents : Parents to be assigned to the clocks
+    listed in "assigned-clocks".
+
+  - MICBIAS1-supply, MICBIAS2-supply: Regulator supplies for the
+    MICxVDD outputs, supplying the digital microphones, normally
+    supplied from the attached CODEC.
+
+Required sub-nodes:
+
+The pin configurations are defined as a child of the pinctrl states
+node, see [2]. Each sub-node can have the following properties:
+  - groups : A list of groups to select (either this or "pins" must be
+    specified), available groups:
+      codec-aif1, codec-aif2, codec-aif3, dsp-aif1, dsp-aif2, psia1,
+      psia2, gf-aif1, gf-aif2, gf-aif3, gf-aif4, spdif-aif, usb-aif1,
+      usb-aif2, adat-aif, soundcard-aif
+  - pins : A list of pin names to select (either this or "groups" must
+    be specified), available pins:
+      fgpa-gpio1, fgpa-gpio2, fgpa-gpio3, fgpa-gpio4, fgpa-gpio5,
+      fgpa-gpio6, codec-gpio1, codec-gpio2, codec-gpio3, codec-gpio4,
+      codec-gpio5, codec-gpio6, codec-gpio7, codec-gpio8, dsp-gpio1,
+      dsp-gpio2, dsp-gpio3, dsp-gpio4, dsp-gpio5, dsp-gpio6, gf-gpio2,
+      gf-gpio3, gf-gpio7, codec-aif1-bclk, codec-aif1-rxdat,
+      codec-aif1-lrclk, codec-aif1-txdat, codec-aif2-bclk,
+      codec-aif2-rxdat, codec-aif2-lrclk, codec-aif2-txdat,
+      codec-aif3-bclk, codec-aif3-rxdat, codec-aif3-lrclk,
+      codec-aif3-txdat, dsp-aif1-bclk, dsp-aif1-rxdat, dsp-aif1-lrclk,
+      dsp-aif1-txdat, dsp-aif2-bclk, dsp-aif2-rxdat,
+      dsp-aif2-lrclk, dsp-aif2-txdat, psia1-bclk, psia1-rxdat,
+      psia1-lrclk, psia1-txdat, psia2-bclk, psia2-rxdat, psia2-lrclk,
+      psia2-txdat, gf-aif3-bclk, gf-aif3-rxdat, gf-aif3-lrclk,
+      gf-aif3-txdat, gf-aif4-bclk, gf-aif4-rxdat, gf-aif4-lrclk,
+      gf-aif4-txdat, gf-aif1-bclk, gf-aif1-rxdat, gf-aif1-lrclk,
+      gf-aif1-txdat, gf-aif2-bclk, gf-aif2-rxdat, gf-aif2-lrclk,
+      gf-aif2-txdat, dsp-uart1-rx, dsp-uart1-tx, dsp-uart2-rx,
+      dsp-uart2-tx, gf-uart2-rx, gf-uart2-tx, usb-uart-rx,
+      codec-pdmclk1, codec-pdmdat1, codec-pdmclk2, codec-pdmdat2,
+      codec-dmicclk1, codec-dmicdat1, codec-dmicclk2, codec-dmicdat2,
+      codec-dmicclk3, codec-dmicdat3, codec-dmicclk4, codec-dmicdat4,
+      dsp-dmicclk1, dsp-dmicdat1, dsp-dmicclk2, dsp-dmicdat2, i2c2-scl,
+      i2c2-sda, i2c3-scl, i2c3-sda, i2c4-scl, i2c4-sda, dsp-standby,
+      codec-mclk1, codec-mclk2, dsp-clkin, psia1-mclk, psia2-mclk,
+      gf-gpio1, gf-gpio5, dsp-gpio20, led1, led2
+  - function : The mux function to select, available functions:
+      aif, fgpa-gpio1, fgpa-gpio2, fgpa-gpio3, fgpa-gpio4, fgpa-gpio5,
+      fgpa-gpio6, codec-gpio1, codec-gpio2, codec-gpio3, codec-gpio4,
+      codec-gpio5, codec-gpio6, codec-gpio7, codec-gpio8, dsp-gpio1,
+      dsp-gpio2, dsp-gpio3, dsp-gpio4, dsp-gpio5, dsp-gpio6, gf-gpio2,
+      gf-gpio3, gf-gpio7, gf-gpio1, gf-gpio5, dsp-gpio20, codec-clkout,
+      dsp-clkout, pmic-32k, spdif-clkout, clk-12m288, clk-11m2986,
+      clk-24m576, clk-22m5792, xmos-mclk, gf-clkout1, gf-mclk1,
+      gf-mclk3, gf-mclk2, gf-clkout2, codec-mclk1, codec-mclk2,
+      dsp-clkin, psia1-mclk, psia2-mclk, spdif-mclk, codec-irq,
+      codec-reset, dsp-reset, dsp-irq, dsp-standby, codec-pdmclk1,
+      codec-pdmdat1, codec-pdmclk2, codec-pdmdat2, codec-dmicclk1,
+      codec-dmicdat1, codec-dmicclk2, codec-dmicdat2, codec-dmicclk3,
+      codec-dmicdat3, codec-dmicclk4, codec-dmicdat4, dsp-dmicclk1,
+      dsp-dmicdat1, dsp-dmicclk2, dsp-dmicdat2, dsp-uart1-rx,
+      dsp-uart1-tx, dsp-uart2-rx, dsp-uart2-tx, gf-uart2-rx,
+      gf-uart2-tx, usb-uart-rx, usb-uart-tx, i2c2-scl, i2c2-sda,
+      i2c3-scl, i2c3-sda, i2c4-scl, i2c4-sda, gpio, spdif-aif, psia1,
+      psia1-bclk, psia1-lrclk, psia1-rxdat, psia1-txdat, psia2,
+      psia2-bclk, psia2-lrclk, psia2-rxdat, psia2-txdat, codec-aif1,
+      codec-aif1-bclk, codec-aif1-lrclk, codec-aif1-rxdat,
+      codec-aif1-txdat, codec-aif2, codec-aif2-bclk, codec-aif2-lrclk,
+      codec-aif2-rxdat, codec-aif2-txdat, codec-aif3, codec-aif3-bclk,
+      codec-aif3-lrclk, codec-aif3-rxdat, codec-aif3-txdat, dsp-aif1,
+      dsp-aif1-bclk, dsp-aif1-lrclk, dsp-aif1-rxdat, dsp-aif1-txdat,
+      dsp-aif2, dsp-aif2-bclk, dsp-aif2-lrclk, dsp-aif2-rxdat,
+      dsp-aif2-txdat, gf-aif3, gf-aif3-bclk, gf-aif3-lrclk,
+      gf-aif3-rxdat, gf-aif3-txdat, gf-aif4, gf-aif4-bclk,
+      gf-aif4-lrclk, gf-aif4-rxdat, gf-aif4-txdat, gf-aif1,
+      gf-aif1-bclk, gf-aif1-lrclk, gf-aif1-rxdat, gf-aif1-txdat,
+      gf-aif2, gf-aif2-bclk, gf-aif2-lrclk, gf-aif2-rxdat,
+      gf-aif2-txdat, usb-aif1, usb-aif2, adat-aif, soundcard-aif,
+
+  - aif-master : Specifies that an AIF group will be used as a master
+    interface (either this or aif-slave is required if a group is
+    being muxed to an AIF)
+  - aif-slave : Specifies that an AIF group will be used as a slave
+    interface (either this or aif-master is required if a group is
+    being muxed to an AIF)
+
+Optional sub-nodes:
+
+  - VDDCORE : Initialisation data for the VDDCORE regulator, which
+    supplies the CODECs digital core if it has no build regulator for that
+    purpose.
+
+  - MICVDD : Initialisation data for the MICVDD regulator, which
+    supplies the CODECs MICVDD.
+  - MIC1VDD, MIC2VDD : Initialisation data for the MICxVDD supplies.
+      Optional Properties:
+      - cirrus,micbias-input : A property selecting which of the CODEC
+        minicard micbias outputs should be used, valid values are 1 - 4.
+
+  - VDD1V8 : Recommended fixed regulator for the VDD1V8 regulator, which supplies the
+    CODECs analog and 1.8V digital supplies.
+      Required Properties:
+      - compatible : Should be set to "regulator-fixed"
+      - regulator-min-microvolt : Should be set to 1.8V
+      - regulator-max-microvolt : Should be set to 1.8V
+      - regulator-boot-on
+      - regulator-always-on
+      - vin-supply : Should be set to same supply as SYSVDD
+
+Example:
+lochnagar: lochnagar@22 {
+	compatible = "cirrus,lochnagar2";
+	reg = <0x22>;
+
+	gpio-controller;
+	#gpio-cells = <2>;
+	gpio-ranges = <&lochnagar 0 0 110>;
+
+	reset-gpio = <&gpio0 55 0>;
+	present-gpio = <&gpio0 60 0>;
+
+	#clock-cells = <1>;
+
+	clocks = <&clk_audio>;
+	clock-names = "ln-gf-mclk2";
+
+	assigned-clocks = <&lochnagar LOCHNAGAR_CDC_MCLK1>,
+			  <&lochnagar LOCHNAGAR_CDC_MCLK2>;
+	assigned-clock-parents = <&clk_audio>,
+				 <&lochnagar LOCHNAGAR_PMIC_32K>;
+
+	SYSVDD-supply = <&wallvdd>;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pin_settings>;
+
+	pin_settings: pin_settings {
+		ap_aif {
+			aif-slave;
+			groups = "gf-aif1";
+			function = "codec-aif3";
+		};
+		codec_aif {
+			aif-master;
+			groups = "codec-aif3";
+			function = "gf-aif1";
+		};
+	};
+
+	lochnagar_micvdd: MICVDD {
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+	};
+
+	lochnagar_vdd1v8: VDD1V8 {
+		compatible = "regulator-fixed";
+
+		regulator-name = "VDD1V8";
+		regulator-min-microvolt = <1800000>;
+		regulator-max-microvolt = <1800000>;
+		regulator-boot-on;
+		regulator-always-on;
+
+		vin-supply = <&wallvdd>;
+	};
+};
diff --git a/include/dt-bindings/clk/lochnagar.h b/include/dt-bindings/clk/lochnagar.h
new file mode 100644
index 0000000000000..caeeec7dfbf6d
--- /dev/null
+++ b/include/dt-bindings/clk/lochnagar.h
@@ -0,0 +1,33 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Device Tree defines for Lochnagar clocking
+ *
+ * Copyright (c) 2017-2018 Cirrus Logic, Inc. and
+ *                         Cirrus Logic International Semiconductor Ltd.
+ *
+ * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
+ */
+
+#ifndef DT_BINDINGS_CLK_LOCHNAGAR_H
+#define DT_BINDINGS_CLK_LOCHNAGAR_H
+
+#define LOCHNAGAR_CDC_MCLK1		0
+#define LOCHNAGAR_CDC_MCLK2		1
+#define LOCHNAGAR_DSP_CLKIN		2
+#define LOCHNAGAR_GF_CLKOUT1		3
+#define LOCHNAGAR_GF_CLKOUT2		4
+#define LOCHNAGAR_PSIA1_MCLK		5
+#define LOCHNAGAR_PSIA2_MCLK		6
+#define LOCHNAGAR_SPDIF_MCLK		7
+#define LOCHNAGAR_ADAT_MCLK		8
+#define LOCHNAGAR_SOUNDCARD_MCLK	9
+#define LOCHNAGAR_PMIC_32K		10
+#define LOCHNAGAR_CLK_12M		11
+#define LOCHNAGAR_CLK_11M		12
+#define LOCHNAGAR_CLK_24M		13
+#define LOCHNAGAR_CLK_22M		14
+#define LOCHNAGAR_USB_CLK_24M		15
+#define LOCHNAGAR_USB_CLK_12M		16
+#define LOCHNAGAR_SPDIF_CLKOUT		17
+
+#endif
diff --git a/include/dt-bindings/pinctrl/lochnagar.h b/include/dt-bindings/pinctrl/lochnagar.h
new file mode 100644
index 0000000000000..644760bf5725b
--- /dev/null
+++ b/include/dt-bindings/pinctrl/lochnagar.h
@@ -0,0 +1,132 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Device Tree defines for Lochnagar pinctrl
+ *
+ * Copyright (c) 2018 Cirrus Logic, Inc. and
+ *                    Cirrus Logic International Semiconductor Ltd.
+ *
+ * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
+ */
+
+#ifndef DT_BINDINGS_PINCTRL_LOCHNAGAR_H
+#define DT_BINDINGS_PINCTRL_LOCHNAGAR_H
+
+#define LOCHNAGAR1_PIN_CDC_RESET		0
+#define LOCHNAGAR1_PIN_DSP_RESET		1
+#define LOCHNAGAR1_PIN_CDC_CIF1MODE		2
+#define LOCHNAGAR1_PIN_NUM_GPIOS		3
+
+#define LOCHNAGAR2_PIN_CDC_RESET		0
+#define LOCHNAGAR2_PIN_DSP_RESET		1
+#define LOCHNAGAR2_PIN_CDC_CIF1MODE		2
+#define LOCHNAGAR2_PIN_CDC_LDOENA		3
+#define LOCHNAGAR2_PIN_SPDIF_HWMODE		4
+#define LOCHNAGAR2_PIN_SPDIF_RESET		5
+#define LOCHNAGAR2_PIN_FPGA_GPIO1		6
+#define LOCHNAGAR2_PIN_FPGA_GPIO2		7
+#define LOCHNAGAR2_PIN_FPGA_GPIO3		8
+#define LOCHNAGAR2_PIN_FPGA_GPIO4		9
+#define LOCHNAGAR2_PIN_FPGA_GPIO5		10
+#define LOCHNAGAR2_PIN_FPGA_GPIO6		11
+#define LOCHNAGAR2_PIN_CDC_GPIO1		12
+#define LOCHNAGAR2_PIN_CDC_GPIO2		13
+#define LOCHNAGAR2_PIN_CDC_GPIO3		14
+#define LOCHNAGAR2_PIN_CDC_GPIO4		15
+#define LOCHNAGAR2_PIN_CDC_GPIO5		16
+#define LOCHNAGAR2_PIN_CDC_GPIO6		17
+#define LOCHNAGAR2_PIN_CDC_GPIO7		18
+#define LOCHNAGAR2_PIN_CDC_GPIO8		19
+#define LOCHNAGAR2_PIN_DSP_GPIO1		20
+#define LOCHNAGAR2_PIN_DSP_GPIO2		21
+#define LOCHNAGAR2_PIN_DSP_GPIO3		22
+#define LOCHNAGAR2_PIN_DSP_GPIO4		23
+#define LOCHNAGAR2_PIN_DSP_GPIO5		24
+#define LOCHNAGAR2_PIN_DSP_GPIO6		25
+#define LOCHNAGAR2_PIN_GF_GPIO2			26
+#define LOCHNAGAR2_PIN_GF_GPIO3			27
+#define LOCHNAGAR2_PIN_GF_GPIO7			28
+#define LOCHNAGAR2_PIN_CDC_AIF1_BCLK		29
+#define LOCHNAGAR2_PIN_CDC_AIF1_RXDAT		30
+#define LOCHNAGAR2_PIN_CDC_AIF1_LRCLK		31
+#define LOCHNAGAR2_PIN_CDC_AIF1_TXDAT		32
+#define LOCHNAGAR2_PIN_CDC_AIF2_BCLK		33
+#define LOCHNAGAR2_PIN_CDC_AIF2_RXDAT		34
+#define LOCHNAGAR2_PIN_CDC_AIF2_LRCLK		35
+#define LOCHNAGAR2_PIN_CDC_AIF2_TXDAT		36
+#define LOCHNAGAR2_PIN_CDC_AIF3_BCLK		37
+#define LOCHNAGAR2_PIN_CDC_AIF3_RXDAT		38
+#define LOCHNAGAR2_PIN_CDC_AIF3_LRCLK		39
+#define LOCHNAGAR2_PIN_CDC_AIF3_TXDAT		40
+#define LOCHNAGAR2_PIN_DSP_AIF1_BCLK		41
+#define LOCHNAGAR2_PIN_DSP_AIF1_RXDAT		42
+#define LOCHNAGAR2_PIN_DSP_AIF1_LRCLK		43
+#define LOCHNAGAR2_PIN_DSP_AIF1_TXDAT		44
+#define LOCHNAGAR2_PIN_DSP_AIF2_BCLK		45
+#define LOCHNAGAR2_PIN_DSP_AIF2_RXDAT		46
+#define LOCHNAGAR2_PIN_DSP_AIF2_LRCLK		47
+#define LOCHNAGAR2_PIN_DSP_AIF2_TXDAT		48
+#define LOCHNAGAR2_PIN_PSIA1_BCLK		49
+#define LOCHNAGAR2_PIN_PSIA1_RXDAT		50
+#define LOCHNAGAR2_PIN_PSIA1_LRCLK		51
+#define LOCHNAGAR2_PIN_PSIA1_TXDAT		52
+#define LOCHNAGAR2_PIN_PSIA2_BCLK		53
+#define LOCHNAGAR2_PIN_PSIA2_RXDAT		54
+#define LOCHNAGAR2_PIN_PSIA2_LRCLK		55
+#define LOCHNAGAR2_PIN_PSIA2_TXDAT		56
+#define LOCHNAGAR2_PIN_GF_AIF3_BCLK		57
+#define LOCHNAGAR2_PIN_GF_AIF3_RXDAT		58
+#define LOCHNAGAR2_PIN_GF_AIF3_LRCLK		59
+#define LOCHNAGAR2_PIN_GF_AIF3_TXDAT		60
+#define LOCHNAGAR2_PIN_GF_AIF4_BCLK		61
+#define LOCHNAGAR2_PIN_GF_AIF4_RXDAT		62
+#define LOCHNAGAR2_PIN_GF_AIF4_LRCLK		63
+#define LOCHNAGAR2_PIN_GF_AIF4_TXDAT		64
+#define LOCHNAGAR2_PIN_GF_AIF1_BCLK		65
+#define LOCHNAGAR2_PIN_GF_AIF1_RXDAT		66
+#define LOCHNAGAR2_PIN_GF_AIF1_LRCLK		67
+#define LOCHNAGAR2_PIN_GF_AIF1_TXDAT		68
+#define LOCHNAGAR2_PIN_GF_AIF2_BCLK		69
+#define LOCHNAGAR2_PIN_GF_AIF2_RXDAT		70
+#define LOCHNAGAR2_PIN_GF_AIF2_LRCLK		71
+#define LOCHNAGAR2_PIN_GF_AIF2_TXDAT		72
+#define LOCHNAGAR2_PIN_DSP_UART1_RX		73
+#define LOCHNAGAR2_PIN_DSP_UART1_TX		74
+#define LOCHNAGAR2_PIN_DSP_UART2_RX		75
+#define LOCHNAGAR2_PIN_DSP_UART2_TX		76
+#define LOCHNAGAR2_PIN_GF_UART2_RX		77
+#define LOCHNAGAR2_PIN_GF_UART2_TX		78
+#define LOCHNAGAR2_PIN_USB_UART_RX		79
+#define LOCHNAGAR2_PIN_CDC_PDMCLK1		80
+#define LOCHNAGAR2_PIN_CDC_PDMDAT1		81
+#define LOCHNAGAR2_PIN_CDC_PDMCLK2		82
+#define LOCHNAGAR2_PIN_CDC_PDMDAT2		83
+#define LOCHNAGAR2_PIN_CDC_DMICCLK1		84
+#define LOCHNAGAR2_PIN_CDC_DMICDAT1		85
+#define LOCHNAGAR2_PIN_CDC_DMICCLK2		86
+#define LOCHNAGAR2_PIN_CDC_DMICDAT2		87
+#define LOCHNAGAR2_PIN_CDC_DMICCLK3		88
+#define LOCHNAGAR2_PIN_CDC_DMICDAT3		89
+#define LOCHNAGAR2_PIN_CDC_DMICCLK4		90
+#define LOCHNAGAR2_PIN_CDC_DMICDAT4		91
+#define LOCHNAGAR2_PIN_DSP_DMICCLK1		92
+#define LOCHNAGAR2_PIN_DSP_DMICDAT1		93
+#define LOCHNAGAR2_PIN_DSP_DMICCLK2		94
+#define LOCHNAGAR2_PIN_DSP_DMICDAT2		95
+#define LOCHNAGAR2_PIN_I2C2_SCL			96
+#define LOCHNAGAR2_PIN_I2C2_SDA			97
+#define LOCHNAGAR2_PIN_I2C3_SCL			98
+#define LOCHNAGAR2_PIN_I2C3_SDA			99
+#define LOCHNAGAR2_PIN_I2C4_SCL			100
+#define LOCHNAGAR2_PIN_I2C4_SDA			101
+#define LOCHNAGAR2_PIN_DSP_STANDBY		102
+#define LOCHNAGAR2_PIN_CDC_MCLK1		103
+#define LOCHNAGAR2_PIN_CDC_MCLK2		104
+#define LOCHNAGAR2_PIN_DSP_CLKIN		105
+#define LOCHNAGAR2_PIN_PSIA1_MCLK		106
+#define LOCHNAGAR2_PIN_PSIA2_MCLK		107
+#define LOCHNAGAR2_PIN_GF_GPIO1			108
+#define LOCHNAGAR2_PIN_GF_GPIO5			109
+#define LOCHNAGAR2_PIN_DSP_GPIO20		110
+#define LOCHNAGAR2_PIN_NUM_GPIOS		111
+
+#endif