[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
Stephen Boyd Oct. 15, 2018, 4:39 p.m. | #9
Quoting Charles Keepax (2018-10-15 03:49:05)
> 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.

Ok if there's going to be read/volatile regs and defaults then it makes
sense to leave the defines in some shared header file, which is
unfortunate for the independent merge of driver bits. Either way, I
would prefer we don't use struct lochnagar in this driver and move to
more generic structures like struct regmap and express the type of MFD
to this device driver some other way.

> 
> > > > > +       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!
Mark Brown Oct. 15, 2018, 4:55 p.m. | #10
On Mon, Oct 15, 2018 at 09:39:59AM -0700, Stephen Boyd wrote:

> Ok if there's going to be read/volatile regs and defaults then it makes
> sense to leave the defines in some shared header file, which is
> unfortunate for the independent merge of driver bits. Either way, I

Kconfig dependencies take care of that pretty well - if there's a
dependency on the MFD then the function driver just won't get built
until the MFD gets merged.
Stephen Boyd Oct. 15, 2018, 9:53 p.m. | #11
Quoting Mark Brown (2018-10-15 09:55:28)
> On Mon, Oct 15, 2018 at 09:39:59AM -0700, Stephen Boyd wrote:
> 
> > Ok if there's going to be read/volatile regs and defaults then it makes
> > sense to leave the defines in some shared header file, which is
> > unfortunate for the independent merge of driver bits. Either way, I
> 
> Kconfig dependencies take care of that pretty well - if there's a
> dependency on the MFD then the function driver just won't get built
> until the MFD gets merged.  

Yep. I'm just saying that I can't merge the clk driver part by itself
when there's a shared header dependency, unless someone provides a
stable tree with the header file patch applied to it.
Lee Jones Oct. 25, 2018, 7:44 a.m. | #12
On Mon, 08 Oct 2018, Charles Keepax wrote:

> From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> 
> 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. Audio system topology,
> clocking and power can all be controlled through the Lochnagar
> controller chip, allowing the device under test to be used in
> a variety of possible use cases.
> 
> As the Lochnagar is a fairly complex device this MFD driver
> allows the drivers for the various features to be bound
> in. Initially clocking, regulator and pinctrl will be added as
> these are necessary to configure the system. But in time at least
> audio and voltage/current monitoring will also be added.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
> 
> Changes since v1:
>  - Update Kconfig to correctly specify dependencies
>  - Update commit message a little
> 
> Thanks,
> Charles
> 
>  MAINTAINERS                         |  13 +
>  drivers/mfd/Kconfig                 |   8 +
>  drivers/mfd/Makefile                |   2 +
>  drivers/mfd/lochnagar-i2c.c         | 732 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/lochnagar.h       |  48 +++
>  include/linux/mfd/lochnagar1_regs.h | 157 ++++++++
>  include/linux/mfd/lochnagar2_regs.h | 253 +++++++++++++
>  7 files changed, 1213 insertions(+)
>  create mode 100644 drivers/mfd/lochnagar-i2c.c
>  create mode 100644 include/linux/mfd/lochnagar.h
>  create mode 100644 include/linux/mfd/lochnagar1_regs.h
>  create mode 100644 include/linux/mfd/lochnagar2_regs.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d9e6d86488df4..f1f3494ac5cd8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3557,6 +3557,19 @@ L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	drivers/net/ethernet/cirrus/ep93xx_eth.c
>  
> +CIRRUS LOGIC LOCHNAGAR DRIVER
> +M:	Charles Keepax <ckeepax@opensource.cirrus.com>
> +M:	Richard Fitzgerald <rf@opensource.cirrus.com>
> +L:	patches@opensource.cirrus.com
> +S:	Supported
> +F:	drivers/clk/clk-lochnagar.c
> +F:	drivers/mfd/lochnagar-i2c.c
> +F:	drivers/pinctrl/cirrus/pinctrl-lochnagar.c
> +F:	drivers/regulator/lochnagar-regulator.c
> +F:	include/dt-bindings/clk/lochnagar.h
> +F:	include/dt-bindings/pinctrl/lochnagar.h
> +F:	include/linux/mfd/lochnagar*
> +
>  CISCO FCOE HBA DRIVER
>  M:	Satish Kharat <satishkh@cisco.com>
>  M:	Sesidhar Baddela <sebaddel@cisco.com>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index f3a5f8d027906..dc64151373714 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1676,6 +1676,14 @@ config MFD_VX855
>  	  VIA VX855/VX875 south bridge. You will need to enable the vx855_spi
>  	  and/or vx855_gpio drivers for this to do anything useful.
>  
> +config MFD_LOCHNAGAR
> +	bool "Cirrus Logic Lochnagar Audio Development Board"
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	depends on I2C=y && OF
> +	help
> +	  Support for Cirrus Logic Lochnagar audio development board.
> +
>  config MFD_ARIZONA
>  	select REGMAP
>  	select REGMAP_IRQ
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 5856a9489cbd8..f16773cb887ff 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -37,6 +37,8 @@ obj-$(CONFIG_MFD_T7L66XB)	+= t7l66xb.o tmio_core.o
>  obj-$(CONFIG_MFD_TC6387XB)	+= tc6387xb.o tmio_core.o
>  obj-$(CONFIG_MFD_TC6393XB)	+= tc6393xb.o tmio_core.o
>  
> +obj-$(CONFIG_MFD_LOCHNAGAR)	+= lochnagar-i2c.o
> +
>  obj-$(CONFIG_MFD_ARIZONA)	+= arizona-core.o
>  obj-$(CONFIG_MFD_ARIZONA)	+= arizona-irq.o
>  obj-$(CONFIG_MFD_ARIZONA_I2C)	+= arizona-i2c.o
> diff --git a/drivers/mfd/lochnagar-i2c.c b/drivers/mfd/lochnagar-i2c.c
> new file mode 100644
> index 0000000000000..7ac7af9e3130b
> --- /dev/null
> +++ b/drivers/mfd/lochnagar-i2c.c
> @@ -0,0 +1,732 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Lochnagar I2C bus interface
> + *
> + * Copyright (c) 2012-2018 Cirrus Logic, Inc. and
> + *                         Cirrus Logic International Semiconductor Ltd.
> + *
> + * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/lochnagar.h>
> +
> +#define LOCHNAGAR_BOOT_RETRIES		10
> +#define LOCHNAGAR_BOOT_DELAY_MS		350
> +
> +#define LOCHNAGAR_CONFIG_POLL_US	10000
> +
> +static bool lochnagar1_readable_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case LOCHNAGAR_SOFTWARE_RESET:
> +	case LOCHNAGAR_FIRMWARE_ID1:
> +	case LOCHNAGAR_FIRMWARE_ID2:
> +	case LOCHNAGAR1_CDC_AIF1_SEL:
> +	case LOCHNAGAR1_CDC_AIF2_SEL:
> +	case LOCHNAGAR1_CDC_AIF3_SEL:
> +	case LOCHNAGAR1_CDC_MCLK1_SEL:
> +	case LOCHNAGAR1_CDC_MCLK2_SEL:
> +	case LOCHNAGAR1_CDC_AIF_CTRL1:
> +	case LOCHNAGAR1_CDC_AIF_CTRL2:
> +	case LOCHNAGAR1_EXT_AIF_CTRL:
> +	case LOCHNAGAR1_DSP_AIF1_SEL:
> +	case LOCHNAGAR1_DSP_AIF2_SEL:
> +	case LOCHNAGAR1_DSP_CLKIN_SEL:
> +	case LOCHNAGAR1_DSP_AIF:
> +	case LOCHNAGAR1_GF_AIF1:
> +	case LOCHNAGAR1_GF_AIF2:
> +	case LOCHNAGAR1_PSIA_AIF:
> +	case LOCHNAGAR1_PSIA1_SEL:
> +	case LOCHNAGAR1_PSIA2_SEL:
> +	case LOCHNAGAR1_SPDIF_AIF_SEL:
> +	case LOCHNAGAR1_GF_AIF3_SEL:
> +	case LOCHNAGAR1_GF_AIF4_SEL:
> +	case LOCHNAGAR1_GF_CLKOUT1_SEL:
> +	case LOCHNAGAR1_GF_AIF1_SEL:
> +	case LOCHNAGAR1_GF_AIF2_SEL:
> +	case LOCHNAGAR1_GF_GPIO2:
> +	case LOCHNAGAR1_GF_GPIO3:
> +	case LOCHNAGAR1_GF_GPIO7:
> +	case LOCHNAGAR1_RST:
> +	case LOCHNAGAR1_LED1:
> +	case LOCHNAGAR1_LED2:
> +	case LOCHNAGAR1_I2C_CTRL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct reg_default lochnagar1_reg_defaults[] = {
> +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> +	{ LOCHNAGAR1_CDC_AIF3_SEL,    0x00 },
> +	{ LOCHNAGAR1_CDC_MCLK1_SEL,   0x00 },
> +	{ LOCHNAGAR1_CDC_MCLK2_SEL,   0x00 },
> +	{ LOCHNAGAR1_CDC_AIF_CTRL1,   0x00 },
> +	{ LOCHNAGAR1_CDC_AIF_CTRL2,   0x00 },
> +	{ LOCHNAGAR1_EXT_AIF_CTRL,    0x00 },
> +	{ LOCHNAGAR1_DSP_AIF1_SEL,    0x00 },
> +	{ LOCHNAGAR1_DSP_AIF2_SEL,    0x00 },
> +	{ LOCHNAGAR1_DSP_CLKIN_SEL,   0x01 },
> +	{ LOCHNAGAR1_DSP_AIF,         0x08 },
> +	{ LOCHNAGAR1_GF_AIF1,         0x00 },
> +	{ LOCHNAGAR1_GF_AIF2,         0x00 },
> +	{ LOCHNAGAR1_PSIA_AIF,        0x00 },
> +	{ LOCHNAGAR1_PSIA1_SEL,       0x00 },
> +	{ LOCHNAGAR1_PSIA2_SEL,       0x00 },
> +	{ LOCHNAGAR1_SPDIF_AIF_SEL,   0x00 },
> +	{ LOCHNAGAR1_GF_AIF3_SEL,     0x00 },
> +	{ LOCHNAGAR1_GF_AIF4_SEL,     0x00 },
> +	{ LOCHNAGAR1_GF_CLKOUT1_SEL,  0x00 },
> +	{ LOCHNAGAR1_GF_AIF1_SEL,     0x00 },
> +	{ LOCHNAGAR1_GF_AIF2_SEL,     0x00 },
> +	{ LOCHNAGAR1_GF_GPIO2,        0x00 },
> +	{ LOCHNAGAR1_GF_GPIO3,        0x00 },
> +	{ LOCHNAGAR1_GF_GPIO7,        0x00 },
> +	{ LOCHNAGAR1_RST,             0x00 },
> +	{ LOCHNAGAR1_LED1,            0x00 },
> +	{ LOCHNAGAR1_LED2,            0x00 },
> +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> +};

Why do you need to specify each register value?

> +static const struct regmap_config lochnagar1_i2c_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +
> +	.max_register = 0x50,
> +	.readable_reg = lochnagar1_readable_register,
> +
> +	.use_single_rw = true,
> +
> +	.cache_type = REGCACHE_RBTREE,
> +
> +	.reg_defaults = lochnagar1_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(lochnagar1_reg_defaults),
> +};
> +
> +static const struct reg_sequence lochnagar1_patch[] = {
> +	{ 0x40, 0x0083 },
> +	{ 0x46, 0x0001 },
> +	{ 0x47, 0x0018 },
> +	{ 0x50, 0x0000 },
> +};

I'm really not a fan of these so call 'patches'.

Can't you set the registers up proper way?

> +static struct mfd_cell lochnagar1_devs[] = {
> +	{
> +		.name = "lochnagar-pinctrl"
> +	},
> +	{
> +		.name = "lochnagar-clk"
> +	},

This should each be on a single line.

> +};
> +
> +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case LOCHNAGAR_SOFTWARE_RESET:
> +	case LOCHNAGAR_FIRMWARE_ID1:
> +	case LOCHNAGAR_FIRMWARE_ID2:
> +	case LOCHNAGAR2_CDC_AIF1_CTRL:
> +	case LOCHNAGAR2_CDC_AIF2_CTRL:
> +	case LOCHNAGAR2_CDC_AIF3_CTRL:
> +	case LOCHNAGAR2_DSP_AIF1_CTRL:
> +	case LOCHNAGAR2_DSP_AIF2_CTRL:
> +	case LOCHNAGAR2_PSIA1_CTRL:
> +	case LOCHNAGAR2_PSIA2_CTRL:
> +	case LOCHNAGAR2_GF_AIF3_CTRL:
> +	case LOCHNAGAR2_GF_AIF4_CTRL:
> +	case LOCHNAGAR2_GF_AIF1_CTRL:
> +	case LOCHNAGAR2_GF_AIF2_CTRL:
> +	case LOCHNAGAR2_SPDIF_AIF_CTRL:
> +	case LOCHNAGAR2_USB_AIF1_CTRL:
> +	case LOCHNAGAR2_USB_AIF2_CTRL:
> +	case LOCHNAGAR2_ADAT_AIF_CTRL:
> +	case LOCHNAGAR2_CDC_MCLK1_CTRL:
> +	case LOCHNAGAR2_CDC_MCLK2_CTRL:
> +	case LOCHNAGAR2_DSP_CLKIN_CTRL:
> +	case LOCHNAGAR2_PSIA1_MCLK_CTRL:
> +	case LOCHNAGAR2_PSIA2_MCLK_CTRL:
> +	case LOCHNAGAR2_SPDIF_MCLK_CTRL:
> +	case LOCHNAGAR2_GF_CLKOUT1_CTRL:
> +	case LOCHNAGAR2_GF_CLKOUT2_CTRL:
> +	case LOCHNAGAR2_ADAT_MCLK_CTRL:
> +	case LOCHNAGAR2_SOUNDCARD_MCLK_CTRL:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO1:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO2:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO3:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO4:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO5:
> +	case LOCHNAGAR2_GPIO_FPGA_GPIO6:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO1:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO2:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO3:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO4:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO5:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO6:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO7:
> +	case LOCHNAGAR2_GPIO_CDC_GPIO8:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO1:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO2:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO3:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO4:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO5:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO6:
> +	case LOCHNAGAR2_GPIO_GF_GPIO2:
> +	case LOCHNAGAR2_GPIO_GF_GPIO3:
> +	case LOCHNAGAR2_GPIO_GF_GPIO7:
> +	case LOCHNAGAR2_GPIO_CDC_AIF1_BCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF1_RXDAT:
> +	case LOCHNAGAR2_GPIO_CDC_AIF1_LRCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF1_TXDAT:
> +	case LOCHNAGAR2_GPIO_CDC_AIF2_BCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF2_RXDAT:
> +	case LOCHNAGAR2_GPIO_CDC_AIF2_LRCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF2_TXDAT:
> +	case LOCHNAGAR2_GPIO_CDC_AIF3_BCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF3_RXDAT:
> +	case LOCHNAGAR2_GPIO_CDC_AIF3_LRCLK:
> +	case LOCHNAGAR2_GPIO_CDC_AIF3_TXDAT:
> +	case LOCHNAGAR2_GPIO_DSP_AIF1_BCLK:
> +	case LOCHNAGAR2_GPIO_DSP_AIF1_RXDAT:
> +	case LOCHNAGAR2_GPIO_DSP_AIF1_LRCLK:
> +	case LOCHNAGAR2_GPIO_DSP_AIF1_TXDAT:
> +	case LOCHNAGAR2_GPIO_DSP_AIF2_BCLK:
> +	case LOCHNAGAR2_GPIO_DSP_AIF2_RXDAT:
> +	case LOCHNAGAR2_GPIO_DSP_AIF2_LRCLK:
> +	case LOCHNAGAR2_GPIO_DSP_AIF2_TXDAT:
> +	case LOCHNAGAR2_GPIO_PSIA1_BCLK:
> +	case LOCHNAGAR2_GPIO_PSIA1_RXDAT:
> +	case LOCHNAGAR2_GPIO_PSIA1_LRCLK:
> +	case LOCHNAGAR2_GPIO_PSIA1_TXDAT:
> +	case LOCHNAGAR2_GPIO_PSIA2_BCLK:
> +	case LOCHNAGAR2_GPIO_PSIA2_RXDAT:
> +	case LOCHNAGAR2_GPIO_PSIA2_LRCLK:
> +	case LOCHNAGAR2_GPIO_PSIA2_TXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF3_BCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF3_RXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF3_LRCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF3_TXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF4_BCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF4_RXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF4_LRCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF4_TXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF1_BCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF1_RXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF1_LRCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF1_TXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF2_BCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF2_RXDAT:
> +	case LOCHNAGAR2_GPIO_GF_AIF2_LRCLK:
> +	case LOCHNAGAR2_GPIO_GF_AIF2_TXDAT:
> +	case LOCHNAGAR2_GPIO_DSP_UART1_RX:
> +	case LOCHNAGAR2_GPIO_DSP_UART1_TX:
> +	case LOCHNAGAR2_GPIO_DSP_UART2_RX:
> +	case LOCHNAGAR2_GPIO_DSP_UART2_TX:
> +	case LOCHNAGAR2_GPIO_GF_UART2_RX:
> +	case LOCHNAGAR2_GPIO_GF_UART2_TX:
> +	case LOCHNAGAR2_GPIO_USB_UART_RX:
> +	case LOCHNAGAR2_GPIO_CDC_PDMCLK1:
> +	case LOCHNAGAR2_GPIO_CDC_PDMDAT1:
> +	case LOCHNAGAR2_GPIO_CDC_PDMCLK2:
> +	case LOCHNAGAR2_GPIO_CDC_PDMDAT2:
> +	case LOCHNAGAR2_GPIO_CDC_DMICCLK1:
> +	case LOCHNAGAR2_GPIO_CDC_DMICDAT1:
> +	case LOCHNAGAR2_GPIO_CDC_DMICCLK2:
> +	case LOCHNAGAR2_GPIO_CDC_DMICDAT2:
> +	case LOCHNAGAR2_GPIO_CDC_DMICCLK3:
> +	case LOCHNAGAR2_GPIO_CDC_DMICDAT3:
> +	case LOCHNAGAR2_GPIO_CDC_DMICCLK4:
> +	case LOCHNAGAR2_GPIO_CDC_DMICDAT4:
> +	case LOCHNAGAR2_GPIO_DSP_DMICCLK1:
> +	case LOCHNAGAR2_GPIO_DSP_DMICDAT1:
> +	case LOCHNAGAR2_GPIO_DSP_DMICCLK2:
> +	case LOCHNAGAR2_GPIO_DSP_DMICDAT2:
> +	case LOCHNAGAR2_GPIO_I2C2_SCL:
> +	case LOCHNAGAR2_GPIO_I2C2_SDA:
> +	case LOCHNAGAR2_GPIO_I2C3_SCL:
> +	case LOCHNAGAR2_GPIO_I2C3_SDA:
> +	case LOCHNAGAR2_GPIO_I2C4_SCL:
> +	case LOCHNAGAR2_GPIO_I2C4_SDA:
> +	case LOCHNAGAR2_GPIO_DSP_STANDBY:
> +	case LOCHNAGAR2_GPIO_CDC_MCLK1:
> +	case LOCHNAGAR2_GPIO_CDC_MCLK2:
> +	case LOCHNAGAR2_GPIO_DSP_CLKIN:
> +	case LOCHNAGAR2_GPIO_PSIA1_MCLK:
> +	case LOCHNAGAR2_GPIO_PSIA2_MCLK:
> +	case LOCHNAGAR2_GPIO_GF_GPIO1:
> +	case LOCHNAGAR2_GPIO_GF_GPIO5:
> +	case LOCHNAGAR2_GPIO_DSP_GPIO20:
> +	case LOCHNAGAR2_GPIO_CHANNEL1:
> +	case LOCHNAGAR2_GPIO_CHANNEL2:
> +	case LOCHNAGAR2_GPIO_CHANNEL3:
> +	case LOCHNAGAR2_GPIO_CHANNEL4:
> +	case LOCHNAGAR2_GPIO_CHANNEL5:
> +	case LOCHNAGAR2_GPIO_CHANNEL6:
> +	case LOCHNAGAR2_GPIO_CHANNEL7:
> +	case LOCHNAGAR2_GPIO_CHANNEL8:
> +	case LOCHNAGAR2_GPIO_CHANNEL9:
> +	case LOCHNAGAR2_GPIO_CHANNEL10:
> +	case LOCHNAGAR2_GPIO_CHANNEL11:
> +	case LOCHNAGAR2_GPIO_CHANNEL12:
> +	case LOCHNAGAR2_GPIO_CHANNEL13:
> +	case LOCHNAGAR2_GPIO_CHANNEL14:
> +	case LOCHNAGAR2_GPIO_CHANNEL15:
> +	case LOCHNAGAR2_GPIO_CHANNEL16:
> +	case LOCHNAGAR2_MINICARD_RESETS:
> +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL2:
> +	case LOCHNAGAR2_COMMS_CTRL4:
> +	case LOCHNAGAR2_SPDIF_CTRL:
> +	case LOCHNAGAR2_POWER_CTRL:
> +	case LOCHNAGAR2_MICVDD_CTRL1:
> +	case LOCHNAGAR2_MICVDD_CTRL2:
> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case LOCHNAGAR2_GPIO_CHANNEL1:
> +	case LOCHNAGAR2_GPIO_CHANNEL2:
> +	case LOCHNAGAR2_GPIO_CHANNEL3:
> +	case LOCHNAGAR2_GPIO_CHANNEL4:
> +	case LOCHNAGAR2_GPIO_CHANNEL5:
> +	case LOCHNAGAR2_GPIO_CHANNEL6:
> +	case LOCHNAGAR2_GPIO_CHANNEL7:
> +	case LOCHNAGAR2_GPIO_CHANNEL8:
> +	case LOCHNAGAR2_GPIO_CHANNEL9:
> +	case LOCHNAGAR2_GPIO_CHANNEL10:
> +	case LOCHNAGAR2_GPIO_CHANNEL11:
> +	case LOCHNAGAR2_GPIO_CHANNEL12:
> +	case LOCHNAGAR2_GPIO_CHANNEL13:
> +	case LOCHNAGAR2_GPIO_CHANNEL14:
> +	case LOCHNAGAR2_GPIO_CHANNEL15:
> +	case LOCHNAGAR2_GPIO_CHANNEL16:
> +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}

This is getting silly now.  Can't you use ranges?

> +static const struct reg_default lochnagar2_reg_defaults[] = {
> +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_DSP_AIF2_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_PSIA1_CTRL,            0x0000 },
> +	{ LOCHNAGAR2_PSIA2_CTRL,            0x0000 },
> +	{ LOCHNAGAR2_GF_AIF3_CTRL,          0x0000 },
> +	{ LOCHNAGAR2_GF_AIF4_CTRL,          0x0000 },
> +	{ LOCHNAGAR2_GF_AIF1_CTRL,          0x0000 },
> +	{ LOCHNAGAR2_GF_AIF2_CTRL,          0x0000 },
> +	{ LOCHNAGAR2_SPDIF_AIF_CTRL,        0x0000 },
> +	{ LOCHNAGAR2_USB_AIF1_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_USB_AIF2_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_ADAT_AIF_CTRL,         0x0000 },
> +	{ LOCHNAGAR2_CDC_MCLK1_CTRL,        0x0000 },
> +	{ LOCHNAGAR2_CDC_MCLK2_CTRL,        0x0000 },
> +	{ LOCHNAGAR2_DSP_CLKIN_CTRL,        0x0000 },
> +	{ LOCHNAGAR2_PSIA1_MCLK_CTRL,       0x0000 },
> +	{ LOCHNAGAR2_PSIA2_MCLK_CTRL,       0x0000 },
> +	{ LOCHNAGAR2_SPDIF_MCLK_CTRL,       0x0000 },
> +	{ LOCHNAGAR2_GF_CLKOUT1_CTRL,       0x0000 },
> +	{ LOCHNAGAR2_GF_CLKOUT2_CTRL,       0x0000 },
> +	{ LOCHNAGAR2_ADAT_MCLK_CTRL,        0x0000 },
> +	{ LOCHNAGAR2_SOUNDCARD_MCLK_CTRL,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO1,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO2,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO3,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO4,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO5,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_FPGA_GPIO6,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO1,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO2,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO3,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO4,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO5,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO6,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO7,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_GPIO8,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO1,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO2,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO3,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO4,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO5,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO6,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_GPIO2,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_GPIO3,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_GPIO7,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF1_BCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF1_RXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF1_LRCLK,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF1_TXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF2_BCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF2_RXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF2_LRCLK,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF2_TXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF3_BCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF3_RXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF3_LRCLK,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_AIF3_TXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF1_BCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF1_RXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF1_LRCLK,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF1_TXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF2_BCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF2_RXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF2_LRCLK,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_AIF2_TXDAT,   0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA1_BCLK,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA1_RXDAT,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA1_LRCLK,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA1_TXDAT,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA2_BCLK,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA2_RXDAT,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA2_LRCLK,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA2_TXDAT,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF3_BCLK,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF3_RXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF3_LRCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF3_TXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF4_BCLK,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF4_RXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF4_LRCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF4_TXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF1_BCLK,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF1_RXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF1_LRCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF1_TXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF2_BCLK,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF2_RXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF2_LRCLK,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_AIF2_TXDAT,    0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_UART1_RX,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_UART1_TX,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_UART2_RX,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_UART2_TX,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_UART2_RX,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_UART2_TX,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_USB_UART_RX,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_PDMCLK1,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_PDMDAT1,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_PDMCLK2,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_PDMDAT2,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICCLK1,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICDAT1,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICCLK2,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICDAT2,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICCLK3,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICDAT3,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICCLK4,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_DMICDAT4,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_DMICCLK1,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_DMICDAT1,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_DMICCLK2,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_DMICDAT2,     0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C2_SCL,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C2_SDA,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C3_SCL,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C3_SDA,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C4_SCL,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_I2C4_SDA,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_STANDBY,      0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_MCLK1,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_CDC_MCLK2,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_CLKIN,        0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA1_MCLK,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_PSIA2_MCLK,       0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_GPIO1,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_GF_GPIO5,         0x0000 },
> +	{ LOCHNAGAR2_GPIO_DSP_GPIO20,       0x0000 },
> +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> +};

OMG!  Vile, vile vile!

> +static const struct regmap_config lochnagar2_i2c_regmap = {
> +	.reg_bits = 16,
> +	.val_bits = 16,
> +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +
> +	.max_register = 0x1F1F,
> +	.readable_reg = lochnagar2_readable_register,
> +	.volatile_reg = lochnagar2_volatile_register,
> +
> +	.cache_type = REGCACHE_RBTREE,
> +
> +	.reg_defaults = lochnagar2_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(lochnagar2_reg_defaults),
> +};
> +
> +static const struct reg_sequence lochnagar2_patch[] = {
> +	{ 0x00EE, 0x0000 },
> +	{ 0x00F0, 0x0001 },
> +};
> +
> +static struct mfd_cell lochnagar2_devs[] = {
> +	{
> +		.name = "lochnagar-pinctrl"
> +	},
> +	{
> +		.name = "lochnagar-clk"
> +	},
> +	{
> +		.name = "lochnagar-regulator"
> +	},
> +	{
> +		.name = "lochnagar2-sound-card",
> +	},
> +};

All the same as above.

> +static struct lochnagar_config {
> +	int id;
> +	const char * const name;
> +	enum lochnagar_type type;
> +	const struct regmap_config *regmap;
> +	struct mfd_cell *devs;
> +	int ndevs;
> +	const struct reg_sequence *patch;
> +	int npatch;
> +} lochnagar_configs[] = {

Not a fan of this syntax.

Please separate the struct declaration and its use.

> +	{
> +		.id = 0x50,
> +		.name = "lochnagar1",
> +		.type = LOCHNAGAR1,
> +		.regmap = &lochnagar1_i2c_regmap,
> +		.devs = lochnagar1_devs,
> +		.ndevs = ARRAY_SIZE(lochnagar1_devs),
> +		.patch = lochnagar1_patch,
> +		.npatch = ARRAY_SIZE(lochnagar1_patch),
> +	},
> +	{
> +		.id = 0xCB58,
> +		.name = "lochnagar2",
> +		.type = LOCHNAGAR2,
> +		.regmap = &lochnagar2_i2c_regmap,
> +		.devs = lochnagar2_devs,
> +		.ndevs = ARRAY_SIZE(lochnagar2_devs),
> +		.patch = lochnagar2_patch,
> +		.npatch = ARRAY_SIZE(lochnagar2_patch),
> +	},
> +};
> +
> +static const struct of_device_id lochnagar_of_match[] = {
> +	{ .compatible = "cirrus,lochnagar1", .data = &lochnagar_configs[0] },
> +	{ .compatible = "cirrus,lochnagar2", .data = &lochnagar_configs[1] },
> +	{},
> +};
> +
> +static int lochnagar_wait_for_boot(struct regmap *regmap, unsigned int *id)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < LOCHNAGAR_BOOT_RETRIES; ++i) {
> +		msleep(LOCHNAGAR_BOOT_DELAY_MS);
> +
> +		ret = regmap_read(regmap, LOCHNAGAR_SOFTWARE_RESET, id);
> +		if (!ret)
> +			return ret;
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +int lochnagar_update_config(struct lochnagar *lochnagar)
> +{
> +	struct regmap *regmap = lochnagar->regmap;
> +	unsigned int done = LOCHNAGAR2_ANALOGUE_PATH_UPDATE_STS_MASK;
> +	int timeout_ms = LOCHNAGAR_BOOT_DELAY_MS * LOCHNAGAR_BOOT_RETRIES;
> +	unsigned int val = 0;
> +	int ret;
> +
> +	switch (lochnagar->type) {
> +	case LOCHNAGAR1:
> +		return 0;
> +	case LOCHNAGAR2:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

This can be done with a simple if () statement:

if (lochnagar->type != LOCHNAGAR2)
   return 0;

> +	ret = regmap_write(regmap, LOCHNAGAR2_ANALOGUE_PATH_CTRL1, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, LOCHNAGAR2_ANALOGUE_PATH_CTRL1,
> +			   LOCHNAGAR2_ANALOGUE_PATH_UPDATE_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_read_poll_timeout(regmap,
> +				       LOCHNAGAR2_ANALOGUE_PATH_CTRL1, val,
> +				       (val & done), LOCHNAGAR_CONFIG_POLL_US,
> +				       timeout_ms * 1000);
> +	if (ret < 0)
> +		return ret;

What does all this do then?  You need a comment.

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(lochnagar_update_config);
> +
> +static int lochnagar_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	const struct lochnagar_config *config = NULL;
> +	const struct of_device_id *of_id;
> +	struct lochnagar *lochnagar;
> +	unsigned int val;
> +	struct gpio_desc *reset, *present;

Put this up higher in the list to keep the simple 'int's together.

> +	unsigned int firmwareid;
> +	int devid, rev;

Why do these need to be signed?

> +	int ret;
> +
> +	lochnagar = devm_kzalloc(dev, sizeof(*lochnagar), GFP_KERNEL);
> +	if (!lochnagar)
> +		return -ENOMEM;
> +
> +	of_id = of_match_device(lochnagar_of_match, dev);
> +	if (!of_id)
> +		return -EINVAL;
> +
> +	config = of_id->data;
> +
> +	lochnagar->dev = dev;
> +	mutex_init(&lochnagar->analogue_config_lock);
> +
> +	dev_set_drvdata(dev, lochnagar);
> +
> +	reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(reset)) {
> +		ret = PTR_ERR(reset);
> +		dev_err(dev, "Failed to get reset GPIO: %d\n", ret);
> +		return ret;
> +	}
> +
> +	present = devm_gpiod_get_optional(dev, "present", GPIOD_OUT_HIGH);
> +	if (IS_ERR(present)) {
> +		ret = PTR_ERR(present);
> +		dev_err(dev, "Failed to get present GPIO: %d\n", ret);
> +		return ret;
> +	}
> +
> +	msleep(20);

What's this for?

> +	/* Bring Lochnagar out of reset */
> +	gpiod_set_value_cansleep(reset, 1);
> +
> +	/* Identify Lochnagar */
> +	lochnagar->type = config->type;

'\n'

> +	lochnagar->regmap = devm_regmap_init_i2c(i2c, config->regmap);
> +	if (IS_ERR(lochnagar->regmap)) {
> +		ret = PTR_ERR(lochnagar->regmap);
> +		dev_err(dev, "Failed to allocate register map: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Wait for Lochnagar to boot */
> +	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read device ID: %d\n", ret);

Eh?

So you read the LOCHNAGAR_SOFTWARE_RESET register and out pops the
device/revision IDs?  That's just random!

> +		return ret;
> +	}
> +
> +	devid = val & LOCHNAGAR_DEVICE_ID_MASK;
> +	rev = val & LOCHNAGAR_REV_ID_MASK;
> +
> +	if (devid != config->id) {
> +		dev_err(dev,
> +			"ID does not match %s (expected 0x%x got 0x%x)\n",
> +			config->name, config->id, devid);
> +		return -ENODEV;
> +	}
> +
> +	/* Identify firmware */
> +	ret = regmap_read(lochnagar->regmap, LOCHNAGAR_FIRMWARE_ID1, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read firmware id 1: %d\n", ret);
> +		return ret;
> +	}
> +
> +	firmwareid = val;
> +
> +	ret = regmap_read(lochnagar->regmap, LOCHNAGAR_FIRMWARE_ID2, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read firmware id 2: %d\n", ret);
> +		return ret;
> +	}
> +
> +	firmwareid |= (val << config->regmap->val_bits);
> +
> +	dev_info(dev, "Found %s (0x%x) revision %d firmware 0x%.6x\n",
> +		 config->name, devid, rev + 1, firmwareid);
> +
> +	ret = regmap_register_patch(lochnagar->regmap, config->patch,
> +				    config->npatch);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register patch: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, config->devs,
> +				   config->ndevs, NULL, 0, NULL);
> +	if (ret != 0) {

if (ret)

> +		dev_err(dev, "Failed to add subdevices: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_of_platform_populate(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> +		return ret;
> +	}

Please do not mix OF and MFD device registration strategies.

Pick one and register all devices through your chosen method.

> +	return ret;
> +}
> +
> +static struct i2c_driver lochnagar_i2c_driver = {
> +	.driver = {
> +		.name = "lochnagar",
> +		.of_match_table = of_match_ptr(lochnagar_of_match),
> +		.suppress_bind_attrs = true,
> +	},
> +

No need for '\n'.

> +	.probe_new = lochnagar_i2c_probe,

Hasn't this been replaced yet?

> +};
> +
> +static int __init lochnagar_i2c_init(void)
> +{
> +	int ret;
> +
> +	ret = i2c_add_driver(&lochnagar_i2c_driver);
> +	if (ret != 0)

if (ret)

> +		pr_err("Failed to register Lochnagar driver: %d\n", ret);
> +
> +	return ret;
> +}
> +subsys_initcall(lochnagar_i2c_init);
> diff --git a/include/linux/mfd/lochnagar.h b/include/linux/mfd/lochnagar.h
> new file mode 100644
> index 0000000000000..be485d6714f7c
> --- /dev/null
> +++ b/include/linux/mfd/lochnagar.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Lochnagar internals
> + *
> + * Copyright (c) 2013-2018 Cirrus Logic, Inc. and
> + *                         Cirrus Logic International Semiconductor Ltd.
> + *
> + * Author: Charles Keepax <ckeepax@opensource.cirrus.com>
> + */
> +
> +#ifndef CIRRUS_LOCHNAGAR_H
> +#define CIRRUS_LOCHNAGAR_H
> +
> +#include "lochnagar1_regs.h"
> +#include "lochnagar2_regs.h"

Why are you including these here?

> +struct device;
> +struct regmap;
> +struct mutex;

Just add the include files.

> +enum lochnagar_type {
> +	LOCHNAGAR1,
> +	LOCHNAGAR2,
> +};
> +
> +struct lochnagar {
> +	enum lochnagar_type type;
> +	struct device *dev;
> +	struct regmap *regmap;
> +
> +	/* Lock to protect updates to the analogue configuration */
> +	struct mutex analogue_config_lock;
> +};

Kerneldoc header please.

> +/* Register Addresses */
> +#define LOCHNAGAR_SOFTWARE_RESET                             0x00
> +#define LOCHNAGAR_FIRMWARE_ID1                               0x01
> +#define LOCHNAGAR_FIRMWARE_ID2                               0x02
> +
> +/* (0x0000)  Software Reset */
> +#define LOCHNAGAR_DEVICE_ID_MASK                           0xFFFC
> +#define LOCHNAGAR_DEVICE_ID_SHIFT                               2
> +#define LOCHNAGAR_REV_ID_MASK                              0x0003
> +#define LOCHNAGAR_REV_ID_SHIFT                                  0
> +
> +int lochnagar_update_config(struct lochnagar *lochnagar);
> +
> +#endif

> diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
> diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h

So Lochnagar 1 and 2 are completely different devices?

What do they do?
Charles Keepax Oct. 25, 2018, 8:26 a.m. | #13
On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> On Mon, 08 Oct 2018, Charles Keepax wrote:
> > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
...
> > +	{ LOCHNAGAR1_LED1,            0x00 },
> > +	{ LOCHNAGAR1_LED2,            0x00 },
> > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > +};
> 
> Why do you need to specify each register value?
> 

The way regmap operates it needs to know the starting value of
each register. It will use this to initialise the cache and to
determine if writes need to actually update the hardware on
cache_syncs after devices have been powered back up.

> > +static const struct reg_sequence lochnagar1_patch[] = {
> > +	{ 0x40, 0x0083 },
> > +	{ 0x46, 0x0001 },
> > +	{ 0x47, 0x0018 },
> > +	{ 0x50, 0x0000 },
> > +};
> 
> I'm really not a fan of these so call 'patches'.
> 
> Can't you set the registers up proper way?
> 

I will see if we could move any out of here or define any of the
registers but as we have discussed before it is not always possible.

> > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case LOCHNAGAR_SOFTWARE_RESET:
> > +	case LOCHNAGAR_FIRMWARE_ID1:
> > +	case LOCHNAGAR_FIRMWARE_ID2:
...
> > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > +	case LOCHNAGAR2_GPIO_CHANNEL3:
...
> > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> 
> This is getting silly now.  Can't you use ranges?
> 

I can if you feel strongly about it? But it does make the drivers
much more error prone and significantly more annoying to work
with. I find it is really common to be checking that a register
is handled correctly through the regmap callbacks and it is nice
to just be able to grep for that. Obviously this won't work for
all devices/regmaps as well since many will not have consecutive
addresses on registers, for example having multi-byte registers
that are byte addressed.

How far would you like me to take this as well? Is it just the
numeric registers you want ranges for ie.

LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16

Or is it all consecutive registers even if they are unrelated
(exmaple is probably not accurate as I haven't checked the
addresses):

LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1

I don't mind the first at all but the second is getting really
horrible in my opinion.

> > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
...
> > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > +};
> 
> OMG!  Vile, vile vile!
> 

I really feel this isn't the driver you are objecting to as such
but the way regmap operates and also we seem to always have the same
discussions around regmap every time we push a driver. Is there
any way me, you and Mark could hash this out and find out a way to
handle regmaps that is acceptable to you? I don't suppose you are
in Edinburgh at the moment for ELCE?

> > +	/* Wait for Lochnagar to boot */
> > +	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to read device ID: %d\n", ret);
> 
> Eh?
> 
> So you read the LOCHNAGAR_SOFTWARE_RESET register and out pops the
> device/revision IDs?  That's just random!

I shall let the hardware guys know you don't approve of their
life choices :-) and add some comments to the code.

> > +	ret = devm_of_platform_populate(dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> > +		return ret;
> > +	}
> 
> Please do not mix OF and MFD device registration strategies.
> 
> Pick one and register all devices through your chosen method.
> 

Hmmm we use this to do things like register some fixed regulators
and clocks that don't need any control but do need to be associated
with the device. I could do that through the MFD although it is in
direct conflict with the feedback on the clock patches I received
to move the fixed clocks into devicetree rather than registering
them manually (see v2 of the patch chain).

I will have a look see if I can find any ideas that will make
everyone happy but we might need to discuss with Mark and the
clock guys.

> > +	.probe_new = lochnagar_i2c_probe,
> 
> Hasn't this been replaced yet?
> 

I will check, the patchset has been around internally for a while
so it is possible this is no longer needed.

> > +#ifndef CIRRUS_LOCHNAGAR_H
> > +#define CIRRUS_LOCHNAGAR_H
> > +
> > +#include "lochnagar1_regs.h"
> > +#include "lochnagar2_regs.h"
> 
> Why are you including these here?
> 

It is just a convenience so the drivers only need to include
lochnagar.h rather than including all three headers manually.

> > diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
> > diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h
> 
> So Lochnagar 1 and 2 are completely different devices?
> 
> What do they do?
> 

Completely different devices is a bit strong, they are different
versions of the same system. They have quite different register
maps but provide very similar functionality.

All the other comments I will get fixed up for the next spin of
the patches.

Thanks,
Charles
Richard Fitzgerald Oct. 25, 2018, 9:28 a.m. | #14
On 25/10/18 09:26, Charles Keepax wrote:
> On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
>> On Mon, 08 Oct 2018, Charles Keepax wrote:
>>> From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>> +static const struct reg_default lochnagar1_reg_defaults[] = {
>>> +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
>>> +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> ...
>>> +	{ LOCHNAGAR1_LED1,            0x00 },
>>> +	{ LOCHNAGAR1_LED2,            0x00 },
>>> +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
>>> +};
>>
>> Why do you need to specify each register value?
>>
> 
> The way regmap operates it needs to know the starting value of
> each register. It will use this to initialise the cache and to
> determine if writes need to actually update the hardware on
> cache_syncs after devices have been powered back up.
> 
>>> +static const struct reg_sequence lochnagar1_patch[] = {
>>> +	{ 0x40, 0x0083 },
>>> +	{ 0x46, 0x0001 },
>>> +	{ 0x47, 0x0018 },
>>> +	{ 0x50, 0x0000 },
>>> +};
>>
>> I'm really not a fan of these so call 'patches'.
>>
>> Can't you set the registers up proper way?
>>
> 
> I will see if we could move any out of here or define any of the
> registers but as we have discussed before it is not always possible.
> 

Also patches generally come out of hardware tuning/qualification/tools
as this list of address,value. So it's easy for people to dump an update
into the driver as a trivial copy-paste but more work if they have to
reverse-engineer the patch list from hardware/datasheet into what each
line "means" and then find the relevant lines of code to change. It's also
much easier to answer the question "Have these hardware patches been
applied to the driver?" if we have them in the original documented format.
It just makes people's lives more difficult if they have to search around
the code to try to find something that looks like the originally specified
patch list. We don't use them just as a lazy way to setup some registers.

>>> +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
>>> +{
>>> +	switch (reg) {
>>> +	case LOCHNAGAR_SOFTWARE_RESET:
>>> +	case LOCHNAGAR_FIRMWARE_ID1:
>>> +	case LOCHNAGAR_FIRMWARE_ID2:
> ...
>>> +	case LOCHNAGAR2_MICVDD_CTRL2:
>>> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
>>> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
>>> +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
>>> +		return true;
>>> +	default:
>>> +		return false;
>>> +	}
>>> +}
>>> +
>>> +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
>>> +{
>>> +	switch (reg) {
>>> +	case LOCHNAGAR2_GPIO_CHANNEL1:
>>> +	case LOCHNAGAR2_GPIO_CHANNEL2:
>>> +	case LOCHNAGAR2_GPIO_CHANNEL3:
> ...
>>> +	case LOCHNAGAR2_GPIO_CHANNEL13:
>>> +	case LOCHNAGAR2_GPIO_CHANNEL14:
>>> +	case LOCHNAGAR2_GPIO_CHANNEL15:
>>> +	case LOCHNAGAR2_GPIO_CHANNEL16:
>>> +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
>>> +		return true;
>>> +	default:
>>> +		return false;
>>> +	}
>>> +}
>>
>> This is getting silly now.  Can't you use ranges?
>>
> 
> I can if you feel strongly about it? But it does make the drivers
> much more error prone and significantly more annoying to work
> with. I find it is really common to be checking that a register
> is handled correctly through the regmap callbacks and it is nice
> to just be able to grep for that. Obviously this won't work for
> all devices/regmaps as well since many will not have consecutive
> addresses on registers, for example having multi-byte registers
> that are byte addressed.
> 
> How far would you like me to take this as well? Is it just the
> numeric registers you want ranges for ie.
> 
> LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> 
> Or is it all consecutive registers even if they are unrelated
> (exmaple is probably not accurate as I haven't checked the
> addresses):
> 
> LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> 
> I don't mind the first at all but the second is getting really
> horrible in my opinion.
> 
>>> +static const struct reg_default lochnagar2_reg_defaults[] = {
>>> +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
>>> +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
>>> +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
>>> +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> ...
>>> +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
>>> +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
>>> +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
>>> +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
>>> +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
>>> +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
>>> +};
>>
>> OMG!  Vile, vile vile!
>>
> 
> I really feel this isn't the driver you are objecting to as such
> but the way regmap operates and also we seem to always have the same
> discussions around regmap every time we push a driver. Is there
> any way me, you and Mark could hash this out and find out a way to
> handle regmaps that is acceptable to you? I don't suppose you are
> in Edinburgh at the moment for ELCE?
> 

I suppose if Mark was willing to promote the regmap drivers to be a
top-level subsystem that could contain the regmap definitions of devices
then we could dump our regmap definitions in there, where Mark can review
it as he's familiar with regmap and the chips and the reasons why things
are done the way they are, rather than Lee having to stress about it every
time we need to create an MFD device that uses regmap. Though that would
make the initialization of an MFD rather awkward with the code required
to init the MFD it not actually being in the MFD tree.

>>> +	/* Wait for Lochnagar to boot */
>>> +	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "Failed to read device ID: %d\n", ret);
>>
>> Eh?
>>
>> So you read the LOCHNAGAR_SOFTWARE_RESET register and out pops the
>> device/revision IDs?  That's just random!
>
> I shall let the hardware guys know you don't approve of their
> life choices :-) and add some comments to the code.
> 
>>> +	ret = devm_of_platform_populate(dev);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
>>> +		return ret;
>>> +	}
>>
>> Please do not mix OF and MFD device registration strategies.
>>
>> Pick one and register all devices through your chosen method.
>>
> 
> Hmmm we use this to do things like register some fixed regulators
> and clocks that don't need any control but do need to be associated
> with the device. I could do that through the MFD although it is in
> direct conflict with the feedback on the clock patches I received
> to move the fixed clocks into devicetree rather than registering
> them manually (see v2 of the patch chain).
> 
> I will have a look see if I can find any ideas that will make
> everyone happy but we might need to discuss with Mark and the
> clock guys.
> 
>>> +	.probe_new = lochnagar_i2c_probe,
>>
>> Hasn't this been replaced yet?
>>
> 
> I will check, the patchset has been around internally for a while
> so it is possible this is no longer needed.
> 
>>> +#ifndef CIRRUS_LOCHNAGAR_H
>>> +#define CIRRUS_LOCHNAGAR_H
>>> +
>>> +#include "lochnagar1_regs.h"
>>> +#include "lochnagar2_regs.h"
>>
>> Why are you including these here?
>>
> 
> It is just a convenience so the drivers only need to include
> lochnagar.h rather than including all three headers manually.
> 
>>> diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
>>> diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h
>>
>> So Lochnagar 1 and 2 are completely different devices?
>>
>> What do they do?
>>
> 
> Completely different devices is a bit strong, they are different
> versions of the same system. They have quite different register
> maps but provide very similar functionality.
> 

The register maps are different partly because some silicon
used on the V1 is no longer manufactured and partly because some
silicon added in V2 didn't fit into the older register map.

> All the other comments I will get fixed up for the next spin of
> the patches.
> 
> Thanks,
> Charles
>
Mark Brown Oct. 25, 2018, 10:12 a.m. | #15
On Thu, Oct 25, 2018 at 10:28:16AM +0100, Richard Fitzgerald wrote:
> On 25/10/18 09:26, Charles Keepax wrote:
> > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:

> > > I'm really not a fan of these so call 'patches'.

> > > Can't you set the registers up proper way?

> > I will see if we could move any out of here or define any of the
> > registers but as we have discussed before it is not always possible.

> Also patches generally come out of hardware tuning/qualification/tools
> as this list of address,value. So it's easy for people to dump an update
> into the driver as a trivial copy-paste but more work if they have to
> reverse-engineer the patch list from hardware/datasheet into what each
> line "means" and then find the relevant lines of code to change. It's also
> much easier to answer the question "Have these hardware patches been
> applied to the driver?" if we have them in the original documented format.
> It just makes people's lives more difficult if they have to search around
> the code to try to find something that looks like the originally specified
> patch list. We don't use them just as a lazy way to setup some registers.

Further, the main intended use for register patches is for registers
that the device manufacturer has no intention of documenting beyond
providing a list of register writes to be done on device startup.  The
common example is test registers with chicken bits that turn out to have
been needed, it's not realistic to expect that vendors are going to
start documenting their test registers.  It's normal to see these
applied only on early revisions of parts (eg, rev A needs a patch to
adjust things to match rev B which is the mass production version).

> > I really feel this isn't the driver you are objecting to as such
> > but the way regmap operates and also we seem to always have the same
> > discussions around regmap every time we push a driver. Is there
> > any way me, you and Mark could hash this out and find out a way to
> > handle regmaps that is acceptable to you? I don't suppose you are
> > in Edinburgh at the moment for ELCE?

> I suppose if Mark was willing to promote the regmap drivers to be a
> top-level subsystem that could contain the regmap definitions of devices
> then we could dump our regmap definitions in there, where Mark can review
> it as he's familiar with regmap and the chips and the reasons why things
> are done the way they are, rather than Lee having to stress about it every
> time we need to create an MFD device that uses regmap. Though that would
> make the initialization of an MFD rather awkward with the code required
> to init the MFD it not actually being in the MFD tree.

I'm not totally against dumping the data tables in some other directory
(we could do ../regmap/tables or whatever) but I fear it's going to
cause otherwise needless cross tree issues.
Charles Keepax Oct. 25, 2018, 10:56 a.m. | #16
On Thu, Oct 25, 2018 at 11:12:48AM +0100, Mark Brown wrote:
> On Thu, Oct 25, 2018 at 10:28:16AM +0100, Richard Fitzgerald wrote:
> > On 25/10/18 09:26, Charles Keepax wrote:
> > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > I really feel this isn't the driver you are objecting to as such
> > > but the way regmap operates and also we seem to always have the same
> > > discussions around regmap every time we push a driver. Is there
> > > any way me, you and Mark could hash this out and find out a way to
> > > handle regmaps that is acceptable to you? I don't suppose you are
> > > in Edinburgh at the moment for ELCE?
> 
> > I suppose if Mark was willing to promote the regmap drivers to be a
> > top-level subsystem that could contain the regmap definitions of devices
> > then we could dump our regmap definitions in there, where Mark can review
> > it as he's familiar with regmap and the chips and the reasons why things
> > are done the way they are, rather than Lee having to stress about it every
> > time we need to create an MFD device that uses regmap. Though that would
> > make the initialization of an MFD rather awkward with the code required
> > to init the MFD it not actually being in the MFD tree.
> 
> I'm not totally against dumping the data tables in some other directory
> (we could do ../regmap/tables or whatever) but I fear it's going to
> cause otherwise needless cross tree issues.

I guess there is a question here, if i split the regmap stuff
into a separate file within mfd would that be enough? Is it just
the mixing of these tables and the code you object to Lee or is
it the fact the tables exist at all?

Thanks,
Charles
Lee Jones Oct. 25, 2018, 11:42 a.m. | #17
On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> On 25/10/18 09:26, Charles Keepax wrote:
> > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > > > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > > > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> > ...
> > > > +	{ LOCHNAGAR1_LED1,            0x00 },
> > > > +	{ LOCHNAGAR1_LED2,            0x00 },
> > > > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > > > +};
> > > 
> > > Why do you need to specify each register value?
> > 
> > The way regmap operates it needs to know the starting value of
> > each register. It will use this to initialise the cache and to
> > determine if writes need to actually update the hardware on
> > cache_syncs after devices have been powered back up.

That sounds crazy to me.  Some devices have thousands of registers.
At a line per register, that's thousands of lines of code/cruft.
Especially seeing as most (sane?) register layouts I've seen default
to zero.  Then default values can be changed at the leisure of the
s/w.

Even if it is absolutely critical that you have to supply these to
Regmap up-front, instead of on first use/read, why can't you just
supply the oddball non-zero ones?

> > > > +static const struct reg_sequence lochnagar1_patch[] = {
> > > > +	{ 0x40, 0x0083 },
> > > > +	{ 0x46, 0x0001 },
> > > > +	{ 0x47, 0x0018 },
> > > > +	{ 0x50, 0x0000 },
> > > > +};
> > > 
> > > I'm really not a fan of these so call 'patches'.
> > > 
> > > Can't you set the registers up proper way?
> > > 
> > 
> > I will see if we could move any out of here or define any of the
> > registers but as we have discussed before it is not always possible.
> > 
> 
> Also patches generally come out of hardware tuning/qualification/tools
> as this list of address,value. So it's easy for people to dump an update
> into the driver as a trivial copy-paste but more work if they have to
> reverse-engineer the patch list from hardware/datasheet into what each
> line "means" and then find the relevant lines of code to change. It's also
> much easier to answer the question "Have these hardware patches been
> applied to the driver?" if we have them in the original documented format.
> It just makes people's lives more difficult if they have to search around
> the code to try to find something that looks like the originally specified
> patch list. We don't use them just as a lazy way to setup some registers.

I understand why they normally exist (sometimes people are just lazy
too) (Mark: BTW chicken-bits sound delicious!).  They're just ugly
from an Open Source PoV.

> > > > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > > > +{
> > > > +	switch (reg) {
> > > > +	case LOCHNAGAR_SOFTWARE_RESET:
> > > > +	case LOCHNAGAR_FIRMWARE_ID1:
> > > > +	case LOCHNAGAR_FIRMWARE_ID2:
> > ...
> > > > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > > > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > > > +		return true;
> > > > +	default:
> > > > +		return false;
> > > > +	}
> > > > +}
> > > > +
> > > > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > > > +{
> > > > +	switch (reg) {
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL3:
> > ...
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > > > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > > > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > > > +		return true;
> > > > +	default:
> > > > +		return false;
> > > > +	}
> > > > +}
> > > 
> > > This is getting silly now.  Can't you use ranges?
> > 
> > I can if you feel strongly about it? But it does make the drivers
> > much more error prone and significantly more annoying to work
> > with. I find it is really common to be checking that a register
> > is handled correctly through the regmap callbacks and it is nice
> > to just be able to grep for that. Obviously this won't work for
> > all devices/regmaps as well since many will not have consecutive
> > addresses on registers, for example having multi-byte registers
> > that are byte addressed.
> > 
> > How far would you like me to take this as well? Is it just the
> > numeric registers you want ranges for ie.
> > 
> > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> > 
> > Or is it all consecutive registers even if they are unrelated
> > (exmaple is probably not accurate as I haven't checked the
> > addresses):
> > 
> > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> > 
> > I don't mind the first at all but the second is getting really
> > horrible in my opinion.

My issue is that we have one end of the scale, where contributors are
submitting patches, trying to remove a line of code here, a line of
code there, then there are patches like this one which describe the
initial value, readable status, writable status and volatile status of
each and every register.

The API is obscene and needs a re-work IMHO.

I really hope we do not really have to list every register, but *if we
absolutely must*, let's do it once:

  REG_ADDRESS, WRV, INITIAL_VALUE

> > > > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > > > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > > > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > > > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > > > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> > ...
> > > > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > > > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > > > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > > > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > > > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > > > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > > > +};
> > > 
> > > OMG!  Vile, vile vile!
> > 
> > I really feel this isn't the driver you are objecting to as such
> > but the way regmap operates and also we seem to always have the same
> > discussions around regmap every time we push a driver.

Absolutely.  I didn't like it before.  I like it even less now.

> > Is there
> > any way me, you and Mark could hash this out and find out a way to
> > handle regmaps that is acceptable to you? I don't suppose you are
> > in Edinburgh at the moment for ELCE?

I'm not at ELCE I'm afraid.

> I suppose if Mark was willing to promote the regmap drivers to be a
> top-level subsystem that could contain the regmap definitions of devices
> then we could dump our regmap definitions in there, where Mark can review
> it as he's familiar with regmap and the chips and the reasons why things
> are done the way they are, rather than Lee having to stress about it every
> time we need to create an MFD device that uses regmap. Though that would
> make the initialization of an MFD rather awkward with the code required
> to init the MFD it not actually being in the MFD tree.

My issue isn't where all this bumph lives.

It's the fact that it's required (at least at this level) at all.

> > > > +	/* Wait for Lochnagar to boot */
> > > > +	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
> > > > +	if (ret < 0) {
> > > > +		dev_err(dev, "Failed to read device ID: %d\n", ret);
> > > 
> > > Eh?
> > > 
> > > So you read the LOCHNAGAR_SOFTWARE_RESET register and out pops the
> > > device/revision IDs?  That's just random!
> > 
> > I shall let the hardware guys know you don't approve of their
> > life choices :-) and add some comments to the code.

Please do.  And tell them to stop drinking at lunch time. ;)

> > > > +	ret = devm_of_platform_populate(dev);
> > > > +	if (ret < 0) {
> > > > +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > 
> > > Please do not mix OF and MFD device registration strategies.
> > > 
> > > Pick one and register all devices through your chosen method.
> > 
> > Hmmm we use this to do things like register some fixed regulators
> > and clocks that don't need any control but do need to be associated
> > with the device. I could do that through the MFD although it is in
> > direct conflict with the feedback on the clock patches I received
> > to move the fixed clocks into devicetree rather than registering
> > them manually (see v2 of the patch chain).

The I suggest moving everything to DT.

> > I will have a look see if I can find any ideas that will make
> > everyone happy but we might need to discuss with Mark and the
> > clock guys.
> > 
> > > > +	.probe_new = lochnagar_i2c_probe,
> > > 
> > > Hasn't this been replaced yet?
> > 
> > I will check, the patchset has been around internally for a while
> > so it is possible this is no longer needed.
> > 
> > > > +#ifndef CIRRUS_LOCHNAGAR_H
> > > > +#define CIRRUS_LOCHNAGAR_H
> > > > +
> > > > +#include "lochnagar1_regs.h"
> > > > +#include "lochnagar2_regs.h"
> > > 
> > > Why are you including these here?
> > > 
> > 
> > It is just a convenience so the drivers only need to include
> > lochnagar.h rather than including all three headers manually.

That's against convention.

If a source file needs a head, it should include it explicitly.

> > > > diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
> > > > diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h
> > > 
> > > So Lochnagar 1 and 2 are completely different devices?
> > > 
> > > What do they do?
> > 
> > Completely different devices is a bit strong, they are different
> > versions of the same system. They have quite different register
> > maps but provide very similar functionality.
> 
> The register maps are different partly because some silicon
> used on the V1 is no longer manufactured and partly because some
> silicon added in V2 didn't fit into the older register map.

I just looked at the maps, which appeared to be vastly different.

> > All the other comments I will get fixed up for the next spin of
> > the patches.
Charles Keepax Oct. 25, 2018, 12:49 p.m. | #18
On Thu, Oct 25, 2018 at 12:42:05PM +0100, Lee Jones wrote:
> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> > On 25/10/18 09:26, Charles Keepax wrote:
> > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > > > > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > > > > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> > > ...
> > > > > +	{ LOCHNAGAR1_LED1,            0x00 },
> > > > > +	{ LOCHNAGAR1_LED2,            0x00 },
> > > > > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > > > > +};
> > > > 
> > > > Why do you need to specify each register value?
> > > 
> > > The way regmap operates it needs to know the starting value of
> > > each register. It will use this to initialise the cache and to
> > > determine if writes need to actually update the hardware on
> > > cache_syncs after devices have been powered back up.
> 
> That sounds crazy to me.  Some devices have thousands of registers.
> At a line per register, that's thousands of lines of code/cruft.
> Especially seeing as most (sane?) register layouts I've seen default
> to zero.  Then default values can be changed at the leisure of the
> s/w.
> 
> Even if it is absolutely critical that you have to supply these to
> Regmap up-front, instead of on first use/read, why can't you just
> supply the oddball non-zero ones?
> 

I don't think you can sensibly get away with not supplying
default values. You say most sane register layouts have zero
values, alas again you may not be the biggest fan of our hardware
guys. The Lochnagar actually does have mostly zero defaults but
that is very much not generally the case with our hardware.

One of the main aims of regmap is to avoid bus accesses when
possible but I guess on the first write/read to any register you
could insert a read to pull the default, although I do worry
there is some corner case/flexibility that is going to cause
headaches later.

I am not sure that dynamically constructing the defaults would be
possible from a table of non-zero ones, or at least doing so would
probably require a fairly major change to the way registers are
specified since with the current callback based approach and a
sparse regmap you could need to iterate over billions of
potential registers to build the table.

> > > > > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > > > > +{
> > > > > +	switch (reg) {
> > > > > +	case LOCHNAGAR_SOFTWARE_RESET:
> > > > > +	case LOCHNAGAR_FIRMWARE_ID1:
> > > > > +	case LOCHNAGAR_FIRMWARE_ID2:
> > > ...
> > > > > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > > > > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > > > > +		return true;
> > > > > +	default:
> > > > > +		return false;
> > > > > +	}
> > > > > +}
> > > > > +
> > > > > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > > > > +{
> > > > > +	switch (reg) {
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL3:
> > > ...
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > > > > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > > > > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > > > > +		return true;
> > > > > +	default:
> > > > > +		return false;
> > > > > +	}
> > > > > +}
> > > > 
> > > > This is getting silly now.  Can't you use ranges?
> > > 
> > > I can if you feel strongly about it? But it does make the drivers
> > > much more error prone and significantly more annoying to work
> > > with. I find it is really common to be checking that a register
> > > is handled correctly through the regmap callbacks and it is nice
> > > to just be able to grep for that. Obviously this won't work for
> > > all devices/regmaps as well since many will not have consecutive
> > > addresses on registers, for example having multi-byte registers
> > > that are byte addressed.
> > > 
> > > How far would you like me to take this as well? Is it just the
> > > numeric registers you want ranges for ie.
> > > 
> > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> > > 
> > > Or is it all consecutive registers even if they are unrelated
> > > (exmaple is probably not accurate as I haven't checked the
> > > addresses):
> > > 
> > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> > > 
> > > I don't mind the first at all but the second is getting really
> > > horrible in my opinion.
> 
> My issue is that we have one end of the scale, where contributors are
> submitting patches, trying to remove a line of code here, a line of
> code there, then there are patches like this one which describe the
> initial value, readable status, writable status and volatile status of
> each and every register.
> 

Removing code is one thing but I would argue that data tables are
somewhat less of a concern. I guess kernel size for very small
systems but then is someone going to be using Lochnagar on one of
those.

> The API is obscene and needs a re-work IMHO.
> 
> I really hope we do not really have to list every register, but *if we
> absolutely must*, let's do it once:
> 
>   REG_ADDRESS, WRV, INITIAL_VALUE
> 

It might be possible to combine all these into one thing,
although again its a fairly major core change.

> > > > > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > > > > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > > > > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > > > > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > > > > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> > > ...
> > > > > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > > > > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > > > > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > > > > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > > > > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > > > > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > > > > +};
> > > > 
> > > > OMG!  Vile, vile vile!
> > > 
> > > I really feel this isn't the driver you are objecting to as such
> > > but the way regmap operates and also we seem to always have the same
> > > discussions around regmap every time we push a driver.
> 
> Absolutely.  I didn't like it before.  I like it even less now.
> 

I guess the question from my side becomes do you want to block
this driver pending on major refactoring to regmap? I will have a
think about what I can do but its going to affect a LOT of drivers.

> > > > > +	ret = devm_of_platform_populate(dev);
> > > > > +	if (ret < 0) {
> > > > > +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> > > > > +		return ret;
> > > > > +	}
> > > > 
> > > > Please do not mix OF and MFD device registration strategies.
> > > > 
> > > > Pick one and register all devices through your chosen method.
> > > 
> > > Hmmm we use this to do things like register some fixed regulators
> > > and clocks that don't need any control but do need to be associated
> > > with the device. I could do that through the MFD although it is in
> > > direct conflict with the feedback on the clock patches I received
> > > to move the fixed clocks into devicetree rather than registering
> > > them manually (see v2 of the patch chain).
> 
> The I suggest moving everything to DT.
> 

I will have a look and see what that would look like.

Thanks,
Charles
Charles Keepax Oct. 25, 2018, 1:20 p.m. | #19
On Thu, Oct 25, 2018 at 01:49:05PM +0100, Charles Keepax wrote:
> On Thu, Oct 25, 2018 at 12:42:05PM +0100, Lee Jones wrote:
> > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> > > On 25/10/18 09:26, Charles Keepax wrote:
> > > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > > > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > I really feel this isn't the driver you are objecting to as such
> > > > but the way regmap operates and also we seem to always have the same
> > > > discussions around regmap every time we push a driver.
> > 
> > Absolutely.  I didn't like it before.  I like it even less now.
> > 
> 
> I guess the question from my side becomes do you want to block
> this driver pending on major refactoring to regmap? I will have a
> think about what I can do but its going to affect a LOT of drivers.
> 

Actually one more thought, perhaps as a halfway for now i could
look into removing the readables and defaults. We lose some things
like error checking that we are reading real registers but as
this driver doesnt currently do cache syncs we might be able to
get away with this for now.

Unless anyone strenuously objects i will have a look at the
options there. As well as looking at wider refactoring but aiming
further out.

Thanks,
Charles
Richard Fitzgerald Oct. 25, 2018, 1:40 p.m. | #20
On 25/10/18 12:42, Lee Jones wrote:
> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
>> On 25/10/18 09:26, Charles Keepax wrote:
>>> On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
>>>> On Mon, 08 Oct 2018, Charles Keepax wrote:
>>>>> From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>>>> +static const struct reg_default lochnagar1_reg_defaults[] = {
>>>>> +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
>>>>> +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
>>> ...
>>>>> +	{ LOCHNAGAR1_LED1,            0x00 },
>>>>> +	{ LOCHNAGAR1_LED2,            0x00 },
>>>>> +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
>>>>> +};
>>>>
>>>> Why do you need to specify each register value?
>>>
>>> The way regmap operates it needs to know the starting value of
>>> each register. It will use this to initialise the cache and to
>>> determine if writes need to actually update the hardware on
>>> cache_syncs after devices have been powered back up.
> 
> That sounds crazy to me.  Some devices have thousands of registers.

Largely the point. How long do you think it would take to populate the
cache if you had to read thousands of registers over I2C? Boot time matters.
Deferring it until it's touched can create various unpredictable and
annoying behaviour later, for example if a lot of cache entries are
written while the chip is asleep and the initial values weren't known
then a suspend/resume cannot filter out writes that are setting the
register to its default (which regmap does to avoid unnecessary bus traffic).
So the resume could have a large amount of unnecessary overhead writing
registers to a value they already have or reading the initial values of
those registers.

> At a line per register, that's thousands of lines of code/cruft.
> Especially seeing as most (sane?) register layouts I've seen default
> to zero.

Not a valid generalization. And it's not a question of sanity, the purpose
of the register and the overhead to setup a use-case also matter.
There are many reasons why the default of a register might not be zero.
Take a look at drivers/mfd/wm5110-tables.c, a lot of the registers don't
have a default of zero (and that's only the registers accessed by the driver.)
It's particularly true of registers that affect things like voltage and
current sources, zero might be a very inappropriate default - even dangerous.
Also enable bits, if some things must power-up enabled and others don't, unless
you want a chip that has a confusing mix of inverted and non-inverted enable
bits. Another side to this is to reduce the number of writes to enable _typical_
behaviour - if an IP block has say 100 registers and you have to write all of
them to make it work that's a lot of overhead compared to them defaulting to
typical values used 99.9% of the time and you only need to write one or two
registers to use it.

   Then default values can be changed at the leisure of the
> s/w.

Potentially with a lot of overhead, especially on those chips with thousands
of registers to set to useful non-zero values before you can use it.

Lochnagar doesn't have that many registers but convention and consistency also
comes into play. Regmap is used in a particular way and it helps people a lot
if every driver using it follows the convention.

> 
> Even if it is absolutely critical that you have to supply these to
> Regmap up-front, instead of on first use/read, why can't you just
> supply the oddball non-zero ones?
> 

If you aren't happy with the regmap subsystem you could send some
patches to change it to what you would be happy with (and patch the ~1300
drivers that use it)

Like any kernel subsystem it has an API that we have to obey to be able to
use it.

>>>>> +static const struct reg_sequence lochnagar1_patch[] = {
>>>>> +	{ 0x40, 0x0083 },
>>>>> +	{ 0x46, 0x0001 },
>>>>> +	{ 0x47, 0x0018 },
>>>>> +	{ 0x50, 0x0000 },
>>>>> +};
>>>>
>>>> I'm really not a fan of these so call 'patches'.
>>>>
>>>> Can't you set the registers up proper way?
>>>>
>>>
>>> I will see if we could move any out of here or define any of the
>>> registers but as we have discussed before it is not always possible.
>>>
>>
>> Also patches generally come out of hardware tuning/qualification/tools
>> as this list of address,value. So it's easy for people to dump an update
>> into the driver as a trivial copy-paste but more work if they have to
>> reverse-engineer the patch list from hardware/datasheet into what each
>> line "means" and then find the relevant lines of code to change. It's also
>> much easier to answer the question "Have these hardware patches been
>> applied to the driver?" if we have them in the original documented format.
>> It just makes people's lives more difficult if they have to search around
>> the code to try to find something that looks like the originally specified
>> patch list. We don't use them just as a lazy way to setup some registers.
> 
> I understand why they normally exist (sometimes people are just lazy
> too) (Mark: BTW chicken-bits sound delicious!).  They're just ugly
> from an Open Source PoV.
> 

In my opinion a lot of the source code in Linux is much uglier than
these tables.

>>>>> +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
>>>>> +{
>>>>> +	switch (reg) {
>>>>> +	case LOCHNAGAR_SOFTWARE_RESET:
>>>>> +	case LOCHNAGAR_FIRMWARE_ID1:
>>>>> +	case LOCHNAGAR_FIRMWARE_ID2:
>>> ...
>>>>> +	case LOCHNAGAR2_MICVDD_CTRL2:
>>>>> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
>>>>> +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
>>>>> +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
>>>>> +		return true;
>>>>> +	default:
>>>>> +		return false;
>>>>> +	}
>>>>> +}
>>>>> +
>>>>> +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
>>>>> +{
>>>>> +	switch (reg) {
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL1:
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL2:
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL3:
>>> ...
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL13:
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL14:
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL15:
>>>>> +	case LOCHNAGAR2_GPIO_CHANNEL16:
>>>>> +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
>>>>> +		return true;
>>>>> +	default:
>>>>> +		return false;
>>>>> +	}
>>>>> +}
>>>>
>>>> This is getting silly now.  Can't you use ranges?
>>>
>>> I can if you feel strongly about it? But it does make the drivers
>>> much more error prone and significantly more annoying to work
>>> with. I find it is really common to be checking that a register
>>> is handled correctly through the regmap callbacks and it is nice
>>> to just be able to grep for that. Obviously this won't work for
>>> all devices/regmaps as well since many will not have consecutive
>>> addresses on registers, for example having multi-byte registers
>>> that are byte addressed.
>>>
>>> How far would you like me to take this as well? Is it just the
>>> numeric registers you want ranges for ie.
>>>
>>> LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
>>>
>>> Or is it all consecutive registers even if they are unrelated
>>> (exmaple is probably not accurate as I haven't checked the
>>> addresses):
>>>
>>> LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
>>>
>>> I don't mind the first at all but the second is getting really
>>> horrible in my opinion.
> 
> My issue is that we have one end of the scale, where contributors are
> submitting patches, trying to remove a line of code here, a line of
> code there, then there are patches like this one which describe the
> initial value, readable status, writable status and volatile status of
> each and every register.
> 

If we could add support for new devices by removing lines of code that
would be cool :). Eventually Linux would support every piece of hardware
and be zero lines of code.

> The API is obscene and needs a re-work IMHO.
> 
> I really hope we do not really have to list every register, but *if we
> absolutely must*, let's do it once:
> 
>    REG_ADDRESS, WRV, INITIAL_VALUE
> 

To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
so it's not our responsibility if you don't like it. Mark Brown is the maintainer.

Submit your patches to Mark and the owners of those ~1300 drivers to propose
changes to regmap that you would be happy with.

>>>>> +static const struct reg_default lochnagar2_reg_defaults[] = {
>>>>> +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
>>>>> +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
>>>>> +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
>>>>> +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
>>> ...
>>>>> +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
>>>>> +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
>>>>> +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
>>>>> +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
>>>>> +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
>>>>> +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
>>>>> +};
>>>>
>>>> OMG!  Vile, vile vile!
>>>
>>> I really feel this isn't the driver you are objecting to as such
>>> but the way regmap operates and also we seem to always have the same
>>> discussions around regmap every time we push a driver.
> 
> Absolutely.  I didn't like it before.  I like it even less now.
> 
>>> Is there
>>> any way me, you and Mark could hash this out and find out a way to
>>> handle regmaps that is acceptable to you? I don't suppose you are
>>> in Edinburgh at the moment for ELCE?
> 
> I'm not at ELCE I'm afraid.
> 
>> I suppose if Mark was willing to promote the regmap drivers to be a
>> top-level subsystem that could contain the regmap definitions of devices
>> then we could dump our regmap definitions in there, where Mark can review
>> it as he's familiar with regmap and the chips and the reasons why things
>> are done the way they are, rather than Lee having to stress about it every
>> time we need to create an MFD device that uses regmap. Though that would
>> make the initialization of an MFD rather awkward with the code required
>> to init the MFD it not actually being in the MFD tree.
> 
> My issue isn't where all this bumph lives.
> 
> It's the fact that it's required (at least at this level) at all.
> 

As above, if one subsystem owner doesn't like another subsystem then those
subsystem owners should talk to each other and sort something out. It shouldn't
block patches that are just trying to use the subsystem as it currently exists
in the kernel.

>>>>> +	/* Wait for Lochnagar to boot */
>>>>> +	ret = lochnagar_wait_for_boot(lochnagar->regmap, &val);
>>>>> +	if (ret < 0) {
>>>>> +		dev_err(dev, "Failed to read device ID: %d\n", ret);
>>>>
>>>> Eh?
>>>>
>>>> So you read the LOCHNAGAR_SOFTWARE_RESET register and out pops the
>>>> device/revision IDs?  That's just random!
>>>
>>> I shall let the hardware guys know you don't approve of their
>>> life choices :-) and add some comments to the code.
> 
> Please do.  And tell them to stop drinking at lunch time. ;)
> 
>>>>> +	ret = devm_of_platform_populate(dev);
>>>>> +	if (ret < 0) {
>>>>> +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
>>>>> +		return ret;
>>>>> +	}
>>>>
>>>> Please do not mix OF and MFD device registration strategies.
>>>>
>>>> Pick one and register all devices through your chosen method.
>>>
>>> Hmmm we use this to do things like register some fixed regulators
>>> and clocks that don't need any control but do need to be associated
>>> with the device. I could do that through the MFD although it is in
>>> direct conflict with the feedback on the clock patches I received
>>> to move the fixed clocks into devicetree rather than registering
>>> them manually (see v2 of the patch chain).
> 
> The I suggest moving everything to DT.
> 
>>> I will have a look see if I can find any ideas that will make
>>> everyone happy but we might need to discuss with Mark and the
>>> clock guys.
>>>
>>>>> +	.probe_new = lochnagar_i2c_probe,
>>>>
>>>> Hasn't this been replaced yet?
>>>
>>> I will check, the patchset has been around internally for a while
>>> so it is possible this is no longer needed.
>>>
>>>>> +#ifndef CIRRUS_LOCHNAGAR_H
>>>>> +#define CIRRUS_LOCHNAGAR_H
>>>>> +
>>>>> +#include "lochnagar1_regs.h"
>>>>> +#include "lochnagar2_regs.h"
>>>>
>>>> Why are you including these here?
>>>>
>>>
>>> It is just a convenience so the drivers only need to include
>>> lochnagar.h rather than including all three headers manually.
> 
> That's against convention.
> 
> If a source file needs a head, it should include it explicitly.
> 
>>>>> diff --git a/include/linux/mfd/lochnagar1_regs.h b/include/linux/mfd/lochnagar1_regs.h
>>>>> diff --git a/include/linux/mfd/lochnagar2_regs.h b/include/linux/mfd/lochnagar2_regs.h
>>>>
>>>> So Lochnagar 1 and 2 are completely different devices?
>>>>
>>>> What do they do?
>>>
>>> Completely different devices is a bit strong, they are different
>>> versions of the same system. They have quite different register
>>> maps but provide very similar functionality.
>>
>> The register maps are different partly because some silicon
>> used on the V1 is no longer manufactured and partly because some
>> silicon added in V2 didn't fit into the older register map.
> 
> I just looked at the maps, which appeared to be vastly different.
> 

We said the maps are different.

>>> All the other comments I will get fixed up for the next spin of
>>> the patches.
>
Richard Fitzgerald Oct. 25, 2018, 1:47 p.m. | #21
On 25/10/18 14:20, Charles Keepax wrote:
> On Thu, Oct 25, 2018 at 01:49:05PM +0100, Charles Keepax wrote:
>> On Thu, Oct 25, 2018 at 12:42:05PM +0100, Lee Jones wrote:
>>> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
>>>> On 25/10/18 09:26, Charles Keepax wrote:
>>>>> On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
>>>>>> On Mon, 08 Oct 2018, Charles Keepax wrote:
>>>>>>> From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
>>>>> I really feel this isn't the driver you are objecting to as such
>>>>> but the way regmap operates and also we seem to always have the same
>>>>> discussions around regmap every time we push a driver.
>>>
>>> Absolutely.  I didn't like it before.  I like it even less now.
>>>
>>
>> I guess the question from my side becomes do you want to block
>> this driver pending on major refactoring to regmap? I will have a
>> think about what I can do but its going to affect a LOT of drivers.
>>
> 
> Actually one more thought, perhaps as a halfway for now i could
> look into removing the readables

Be careful with that, there are some addresses that are illegal to
access. What does regmap debugfs do if you don't have a readables
list? Just reading a debugfs shouldn't be able to kill the hardware.
You might need to add a precious list which is more error prone
than listing the valid readables we are using.

  and defaults. We lose some things
> like error checking that we are reading real registers but as
> this driver doesnt currently do cache syncs we might be able to
> get away with this for now.
> 
> Unless anyone strenuously objects i will have a look at the
> options there. As well as looking at wider refactoring but aiming
> further out.
> 
> Thanks,
> Charles
>
Lee Jones Oct. 26, 2018, 7:33 a.m. | #22
On Thu, 25 Oct 2018, Charles Keepax wrote:

> On Thu, Oct 25, 2018 at 12:42:05PM +0100, Lee Jones wrote:
> > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> > > On 25/10/18 09:26, Charles Keepax wrote:
> > > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > > > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > > > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > > > > > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > > > > > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> > > > ...
> > > > > > +	{ LOCHNAGAR1_LED1,            0x00 },
> > > > > > +	{ LOCHNAGAR1_LED2,            0x00 },
> > > > > > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > > > > > +};
> > > > > 
> > > > > Why do you need to specify each register value?
> > > > 
> > > > The way regmap operates it needs to know the starting value of
> > > > each register. It will use this to initialise the cache and to
> > > > determine if writes need to actually update the hardware on
> > > > cache_syncs after devices have been powered back up.
> > 
> > That sounds crazy to me.  Some devices have thousands of registers.
> > At a line per register, that's thousands of lines of code/cruft.
> > Especially seeing as most (sane?) register layouts I've seen default
> > to zero.  Then default values can be changed at the leisure of the
> > s/w.
> > 
> > Even if it is absolutely critical that you have to supply these to
> > Regmap up-front, instead of on first use/read, why can't you just
> > supply the oddball non-zero ones?
> 
> I don't think you can sensibly get away with not supplying
> default values. You say most sane register layouts have zero
> values, alas again you may not be the biggest fan of our hardware
> guys. The Lochnagar actually does have mostly zero defaults but
> that is very much not generally the case with our hardware.
> 
> One of the main aims of regmap is to avoid bus accesses when
> possible but I guess on the first write/read to any register you
> could insert a read to pull the default, although I do worry
> there is some corner case/flexibility that is going to cause
> headaches later.

This is basically what I am suggesting.

> I am not sure that dynamically constructing the defaults would be
> possible from a table of non-zero ones, or at least doing so would
> probably require a fairly major change to the way registers are
> specified since with the current callback based approach and a
> sparse regmap you could need to iterate over billions of
> potential registers to build the table.

What if a valid register range was provided with only the non-zero
values?

> > > > > > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > > > > > +{
> > > > > > +	switch (reg) {
> > > > > > +	case LOCHNAGAR_SOFTWARE_RESET:
> > > > > > +	case LOCHNAGAR_FIRMWARE_ID1:
> > > > > > +	case LOCHNAGAR_FIRMWARE_ID2:
> > > > ...
> > > > > > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > > > > > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > > > > > +		return true;
> > > > > > +	default:
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > > > > > +{
> > > > > > +	switch (reg) {
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL3:
> > > > ...
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > > > > > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > > > > > +		return true;
> > > > > > +	default:
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +}
> > > > > 
> > > > > This is getting silly now.  Can't you use ranges?
> > > > 
> > > > I can if you feel strongly about it? But it does make the drivers
> > > > much more error prone and significantly more annoying to work
> > > > with. I find it is really common to be checking that a register
> > > > is handled correctly through the regmap callbacks and it is nice
> > > > to just be able to grep for that. Obviously this won't work for
> > > > all devices/regmaps as well since many will not have consecutive
> > > > addresses on registers, for example having multi-byte registers
> > > > that are byte addressed.
> > > > 
> > > > How far would you like me to take this as well? Is it just the
> > > > numeric registers you want ranges for ie.
> > > > 
> > > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> > > > 
> > > > Or is it all consecutive registers even if they are unrelated
> > > > (exmaple is probably not accurate as I haven't checked the
> > > > addresses):
> > > > 
> > > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> > > > 
> > > > I don't mind the first at all but the second is getting really
> > > > horrible in my opinion.
> > 
> > My issue is that we have one end of the scale, where contributors are
> > submitting patches, trying to remove a line of code here, a line of
> > code there, then there are patches like this one which describe the
> > initial value, readable status, writable status and volatile status of
> > each and every register.
> 
> Removing code is one thing but I would argue that data tables are
> somewhat less of a concern. I guess kernel size for very small
> systems but then is someone going to be using Lochnagar on one of
> those.
> 
> > The API is obscene and needs a re-work IMHO.
> > 
> > I really hope we do not really have to list every register, but *if we
> > absolutely must*, let's do it once:
> > 
> >   REG_ADDRESS, WRV, INITIAL_VALUE
> 
> It might be possible to combine all these into one thing,
> although again its a fairly major core change.

I'm not suggesting that this will be solved overnight.

> > > > > > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > > > > > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> > > > ...
> > > > > > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > > > > > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > > > > > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > > > > > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > > > > > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > > > > > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > > > > > +};
> > > > > 
> > > > > OMG!  Vile, vile vile!
> > > > 
> > > > I really feel this isn't the driver you are objecting to as such
> > > > but the way regmap operates and also we seem to always have the same
> > > > discussions around regmap every time we push a driver.
> > 
> > Absolutely.  I didn't like it before.  I like it even less now.
> 
> I guess the question from my side becomes do you want to block
> this driver pending on major refactoring to regmap? I will have a
> think about what I can do but its going to affect a LOT of drivers.

No one is NACKing this driver.

We're using it as a vehicle for discussion.

> > > > > > +	ret = devm_of_platform_populate(dev);
> > > > > > +	if (ret < 0) {
> > > > > > +		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > 
> > > > > Please do not mix OF and MFD device registration strategies.
> > > > > 
> > > > > Pick one and register all devices through your chosen method.
> > > > 
> > > > Hmmm we use this to do things like register some fixed regulators
> > > > and clocks that don't need any control but do need to be associated
> > > > with the device. I could do that through the MFD although it is in
> > > > direct conflict with the feedback on the clock patches I received
> > > > to move the fixed clocks into devicetree rather than registering
> > > > them manually (see v2 of the patch chain).
> > 
> > The I suggest moving everything to DT.
> 
> I will have a look and see what that would look like.

Thanks.
Lee Jones Oct. 26, 2018, 8 a.m. | #23
On Thu, 25 Oct 2018, Richard Fitzgerald wrote:

> On 25/10/18 12:42, Lee Jones wrote:
> > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> > > On 25/10/18 09:26, Charles Keepax wrote:
> > > > On Thu, Oct 25, 2018 at 08:44:59AM +0100, Lee Jones wrote:
> > > > > On Mon, 08 Oct 2018, Charles Keepax wrote:
> > > > > > From: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > > > +static const struct reg_default lochnagar1_reg_defaults[] = {
> > > > > > +	{ LOCHNAGAR1_CDC_AIF1_SEL,    0x00 },
> > > > > > +	{ LOCHNAGAR1_CDC_AIF2_SEL,    0x00 },
> > > > ...
> > > > > > +	{ LOCHNAGAR1_LED1,            0x00 },
> > > > > > +	{ LOCHNAGAR1_LED2,            0x00 },
> > > > > > +	{ LOCHNAGAR1_I2C_CTRL,        0x01 },
> > > > > > +};
> > > > > 
> > > > > Why do you need to specify each register value?
> > > > 
> > > > The way regmap operates it needs to know the starting value of
> > > > each register. It will use this to initialise the cache and to
> > > > determine if writes need to actually update the hardware on
> > > > cache_syncs after devices have been powered back up.
> > 
> > That sounds crazy to me.  Some devices have thousands of registers.
> 
> Largely the point. How long do you think it would take to populate the
> cache if you had to read thousands of registers over I2C? Boot time matters.
> Deferring it until it's touched can create various unpredictable and
> annoying behaviour later, for example if a lot of cache entries are
> written while the chip is asleep and the initial values weren't known
> then a suspend/resume cannot filter out writes that are setting the
> register to its default (which regmap does to avoid unnecessary bus traffic).
> So the resume could have a large amount of unnecessary overhead writing
> registers to a value they already have or reading the initial values of
> those registers.

One more register read when initially writing to a register and one
more when resuming doesn't sound like a vast amount of over-head.

> > At a line per register, that's thousands of lines of code/cruft.
> > Especially seeing as most (sane?) register layouts I've seen default
> > to zero.
> 
> Not a valid generalization. And it's not a question of sanity, the purpose
> of the register and the overhead to setup a use-case also matter.
> There are many reasons why the default of a register might not be zero.
> Take a look at drivers/mfd/wm5110-tables.c, a lot of the registers don't
> have a default of zero (and that's only the registers accessed by the driver.)
> It's particularly true of registers that affect things like voltage and
> current sources, zero might be a very inappropriate default - even dangerous.
> Also enable bits, if some things must power-up enabled and others don't, unless
> you want a chip that has a confusing mix of inverted and non-inverted enable
> bits. Another side to this is to reduce the number of writes to enable _typical_
> behaviour - if an IP block has say 100 registers and you have to write all of
> them to make it work that's a lot of overhead compared to them defaulting to
> typical values used 99.9% of the time and you only need to write one or two
> registers to use it.

Not sure what you think I was suggesting above.  If the default values
are actually non-zero that's fine - we'll either leave them as they
are (if they are never changed, in which case Regmap doesn't even need
to know about them), document only those (non-zero) ones or wait until
they are read for the first time, then populate the cache.

Setting up the cache manually also sounds like a vector for potential
failure.  At least if you were to cache dynamically on first write
(either on start-up or after sleep) then the actual value will be
cached, rather than what a piece of C code says it should be.

>   Then default values can be changed at the leisure of the
> > s/w.
> 
> Potentially with a lot of overhead, especially on those chips with thousands
> of registers to set to useful non-zero values before you can use it.
> 
> Lochnagar doesn't have that many registers but convention and consistency also
> comes into play. Regmap is used in a particular way and it helps people a lot
> if every driver using it follows the convention.

Precisely my point.  Lochnagar is a small device yet it's required to
submit hundreds of lines of Regmap tables.  Imagine what that would
look like for a large device.

> > Even if it is absolutely critical that you have to supply these to
> > Regmap up-front, instead of on first use/read, why can't you just
> > supply the oddball non-zero ones?
> 
> If you aren't happy with the regmap subsystem you could send some
> patches to change it to what you would be happy with (and patch the ~1300
> drivers that use it)

That may well happen.  This is the pre-patch discussion.

Apologies that it had to happen on your submission, but this is that
alerted me to the issue(s).

> Like any kernel subsystem it has an API that we have to obey to be able to
> use it.

Again, this isn't about Lochnagar.

> > > > > > +static const struct reg_sequence lochnagar1_patch[] = {
> > > > > > +	{ 0x40, 0x0083 },
> > > > > > +	{ 0x46, 0x0001 },
> > > > > > +	{ 0x47, 0x0018 },
> > > > > > +	{ 0x50, 0x0000 },
> > > > > > +};
> > > > > 
> > > > > I'm really not a fan of these so call 'patches'.
> > > > > 
> > > > > Can't you set the registers up proper way?
> > > > > 
> > > > 
> > > > I will see if we could move any out of here or define any of the
> > > > registers but as we have discussed before it is not always possible.
> > > > 
> > > 
> > > Also patches generally come out of hardware tuning/qualification/tools
> > > as this list of address,value. So it's easy for people to dump an update
> > > into the driver as a trivial copy-paste but more work if they have to
> > > reverse-engineer the patch list from hardware/datasheet into what each
> > > line "means" and then find the relevant lines of code to change. It's also
> > > much easier to answer the question "Have these hardware patches been
> > > applied to the driver?" if we have them in the original documented format.
> > > It just makes people's lives more difficult if they have to search around
> > > the code to try to find something that looks like the originally specified
> > > patch list. We don't use them just as a lazy way to setup some registers.
> > 
> > I understand why they normally exist (sometimes people are just lazy
> > too) (Mark: BTW chicken-bits sound delicious!).  They're just ugly
> > from an Open Source PoV.
> 
> In my opinion a lot of the source code in Linux is much uglier than
> these tables.

Right.  I usually comment on those (when I see them) too.

Besides, just because some people committing murder, doesn't mean
other people shouldn't go to jail for stealing a car.

> > > > > > +static bool lochnagar2_readable_register(struct device *dev, unsigned int reg)
> > > > > > +{
> > > > > > +	switch (reg) {
> > > > > > +	case LOCHNAGAR_SOFTWARE_RESET:
> > > > > > +	case LOCHNAGAR_FIRMWARE_ID1:
> > > > > > +	case LOCHNAGAR_FIRMWARE_ID2:
> > > > ...
> > > > > > +	case LOCHNAGAR2_MICVDD_CTRL2:
> > > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL1:
> > > > > > +	case LOCHNAGAR2_VDDCORE_CDC_CTRL2:
> > > > > > +	case LOCHNAGAR2_SOUNDCARD_AIF_CTRL:
> > > > > > +		return true;
> > > > > > +	default:
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > > +static bool lochnagar2_volatile_register(struct device *dev, unsigned int reg)
> > > > > > +{
> > > > > > +	switch (reg) {
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL1:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL2:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL3:
> > > > ...
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL13:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL14:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL15:
> > > > > > +	case LOCHNAGAR2_GPIO_CHANNEL16:
> > > > > > +	case LOCHNAGAR2_ANALOGUE_PATH_CTRL1:
> > > > > > +		return true;
> > > > > > +	default:
> > > > > > +		return false;
> > > > > > +	}
> > > > > > +}
> > > > > 
> > > > > This is getting silly now.  Can't you use ranges?
> > > > 
> > > > I can if you feel strongly about it? But it does make the drivers
> > > > much more error prone and significantly more annoying to work
> > > > with. I find it is really common to be checking that a register
> > > > is handled correctly through the regmap callbacks and it is nice
> > > > to just be able to grep for that. Obviously this won't work for
> > > > all devices/regmaps as well since many will not have consecutive
> > > > addresses on registers, for example having multi-byte registers
> > > > that are byte addressed.
> > > > 
> > > > How far would you like me to take this as well? Is it just the
> > > > numeric registers you want ranges for ie.
> > > > 
> > > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR_GPIO_CHANNEL16
> > > > 
> > > > Or is it all consecutive registers even if they are unrelated
> > > > (exmaple is probably not accurate as I haven't checked the
> > > > addresses):
> > > > 
> > > > LOCHNAGAR2_GPIO_CHANNEL1...LOCHNAGAR2_ANALOGURE_PATH_CTRL1
> > > > 
> > > > I don't mind the first at all but the second is getting really
> > > > horrible in my opinion.
> > 
> > My issue is that we have one end of the scale, where contributors are
> > submitting patches, trying to remove a line of code here, a line of
> > code there, then there are patches like this one which describe the
> > initial value, readable status, writable status and volatile status of
> > each and every register.
> 
> If we could add support for new devices by removing lines of code that
> would be cool :). Eventually Linux would support every piece of hardware
> and be zero lines of code.

I'm starting to think that you've missed the point. ;)

> > The API is obscene and needs a re-work IMHO.
> > 
> > I really hope we do not really have to list every register, but *if we
> > absolutely must*, let's do it once:
> > 
> >    REG_ADDRESS, WRV, INITIAL_VALUE
> 
> To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
> so it's not our responsibility if you don't like it. Mark Brown is the maintainer.

Sounds very much like you are saying, "it's not Cirrus' fault,
therefore it is not my problem"?  Which is a terrible attitude.

Remember that the Linux kernel is an open community.  Anyone should be
free to discuss any relevant issue.  If we decide to take this
off-line, which is likely, then so be it.  In the mean time, as a
contributor to this community project, it's absolutely your
responsibly to help discuss and potentially solve wider issues than
just your lines of code.

> Submit your patches to Mark and the owners of those ~1300 drivers to propose
> changes to regmap that you would be happy with.

Quoting myself from above:

  "That may well happen.  This is the pre-patch discussion.

  Apologies that it had to happen on your submission, but this is that
  alerted me to the issue(s)."

> > > > > > +static const struct reg_default lochnagar2_reg_defaults[] = {
> > > > > > +	{ LOCHNAGAR2_CDC_AIF1_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_CDC_AIF2_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_CDC_AIF3_CTRL,         0x0000 },
> > > > > > +	{ LOCHNAGAR2_DSP_AIF1_CTRL,         0x0000 },
> > > > ...
> > > > > > +	{ LOCHNAGAR2_MINICARD_RESETS,       0x0000 },
> > > > > > +	{ LOCHNAGAR2_ANALOGUE_PATH_CTRL2,   0x0000 },
> > > > > > +	{ LOCHNAGAR2_COMMS_CTRL4,           0x0001 },
> > > > > > +	{ LOCHNAGAR2_SPDIF_CTRL,            0x0008 },
> > > > > > +	{ LOCHNAGAR2_POWER_CTRL,            0x0001 },
> > > > > > +	{ LOCHNAGAR2_SOUNDCARD_AIF_CTRL,    0x0000 },
> > > > > > +};
> > > > > 
> > > > > OMG!  Vile, vile vile!
> > > > 
> > > > I really feel this isn't the driver you are objecting to as such
> > > > but the way regmap operates and also we seem to always have the same
> > > > discussions around regmap every time we push a driver.
> > 
> > Absolutely.  I didn't like it before.  I like it even less now.
> > 
> > > > Is there
> > > > any way me, you and Mark could hash this out and find out a way to
> > > > handle regmaps that is acceptable to you? I don't suppose you are
> > > > in Edinburgh at the moment for ELCE?
> > 
> > I'm not at ELCE I'm afraid.
> > 
> > > I suppose if Mark was willing to promote the regmap drivers to be a
> > > top-level subsystem that could contain the regmap definitions of devices
> > > then we could dump our regmap definitions in there, where Mark can review
> > > it as he's familiar with regmap and the chips and the reasons why things
> > > are done the way they are, rather than Lee having to stress about it every
> > > time we need to create an MFD device that uses regmap. Though that would
> > > make the initialization of an MFD rather awkward with the code required
> > > to init the MFD it not actually being in the MFD tree.
> > 
> > My issue isn't where all this bumph lives.
> > 
> > It's the fact that it's required (at least at this level) at all.
> > 
> 
> As above, if one subsystem owner doesn't like another subsystem then those
> subsystem owners should talk to each other and sort something out. It shouldn't
> block patches that are just trying to use the subsystem as it currently exists
> in the kernel.

Again, no one is blocking this patch.

This driver was submitted for review/discussion.  We are discussing.
Mark Brown Oct. 26, 2018, 3:47 p.m. | #24
On Thu, Oct 25, 2018 at 01:49:05PM +0100, Charles Keepax wrote:

> I don't think you can sensibly get away with not supplying
> default values. You say most sane register layouts have zero
> values, alas again you may not be the biggest fan of our hardware
> guys. The Lochnagar actually does have mostly zero defaults but
> that is very much not generally the case with our hardware.

There's large classes of hardware where it's just not generally the
default over all manufacturers - things like audio CODECs and regulators
that have to represent continuous ranges of numerical values have to
decide between a helpful representation of numbers with a non-zero
default or having a more complicated representation of numbers which
manage to make zero correspond to whatever the default value is.  It's
also quite common for booleans you want to default on.

> One of the main aims of regmap is to avoid bus accesses when
> possible but I guess on the first write/read to any register you
> could insert a read to pull the default, although I do worry
> there is some corner case/flexibility that is going to cause
> headaches later.

It will actually populate the cache from I/O if you don't provide
defaults but it then can't tell what the physical defaults are, we could
read back from hardware but then we have to sync that with the hardware
being reset and if you read the whole thing back that gets super
expensive with slow buses.
Mark Brown Oct. 26, 2018, 3:49 p.m. | #25
On Thu, Oct 25, 2018 at 02:47:59PM +0100, Richard Fitzgerald wrote:

> access. What does regmap debugfs do if you don't have a readables
> list? Just reading a debugfs shouldn't be able to kill the hardware.
> You might need to add a precious list which is more error prone
> than listing the valid readables we are using.

It assumes everything is readable unless it's explicitly told otherwise
or the register map is overall write only (like a 7x9 one).  You could
mark the registers as precious to cause it to only do explicit I/O
operations on them but that's not what you mean and won't be as good a
guarantee that nothing can trigger a read.
Mark Brown Oct. 26, 2018, 8:32 p.m. | #26
On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:

> > Largely the point. How long do you think it would take to populate the
> > cache if you had to read thousands of registers over I2C? Boot time matters.
> > Deferring it until it's touched can create various unpredictable and
> > annoying behaviour later, for example if a lot of cache entries are
> > written while the chip is asleep and the initial values weren't known
> > then a suspend/resume cannot filter out writes that are setting the
> > register to its default (which regmap does to avoid unnecessary bus traffic).
> > So the resume could have a large amount of unnecessary overhead writing
> > registers to a value they already have or reading the initial values of
> > those registers.

> One more register read when initially writing to a register and one
> more when resuming doesn't sound like a vast amount of over-head.

Especially on resume extra register I/O really adds up - people really
care how long their system takes to come back from suspend, and how
quickly individual devices come back.  For devices that are on slow
buses like I2C this means that every register operation counts.  Boot
can be similarly pressured of course, though it's a less frequent issue
for these devices.

> Not sure what you think I was suggesting above.  If the default values
> are actually non-zero that's fine - we'll either leave them as they
> are (if they are never changed, in which case Regmap doesn't even need
> to know about them), document only those (non-zero) ones or wait until
> they are read for the first time, then populate the cache.

You can't assume that the device is in power on reset state unless the
driver reset it itself which may or may not be a good idea or even
possible, sometimes it's what you want but at other times even if it's
possible it can cause user visible disruption during the boot process
which is undesirable.

> Setting up the cache manually also sounds like a vector for potential
> failure.  At least if you were to cache dynamically on first write
> (either on start-up or after sleep) then the actual value will be
> cached, rather than what a piece of C code says it should be.

Even where there's no problem getting the hardware into a clean state it
can rapidly get very, very expensive to do this with larger register
sets on slow buses, and at the framework level we can't assume that
readback support is even present on the device (the earliest versions of
cache support were written to support such devices).  Some of the
userspaces that regmap devices get used with end up wanting to apply a
bunch of configuration at startup, if we can cut down on the amount of
I/O that's involved in doing that it can help them quite a bit.  You
also get userspaces that want to enumerate device state at startup,
that's a bit easier to change in userspace but it's not an unreasonable
thing to want to do and can also get very I/O heavy.

There is some potential for errors to be introduced but equally these
tables can be both generated and verified mechanically, tasks that are
particularly straightforward for the device vendors to do.  There are
also potential risks in doing this at runtime if we didn't get the
device reset, if we don't accurately mark the volatile registers as
volatile or if there's otherwise bugs in the code.

> Precisely my point.  Lochnagar is a small device yet it's required to
> submit hundreds of lines of Regmap tables.  Imagine what that would
> look like for a large device.

There's no *requirement* to provide the data even if you're using the
cache (and the cache support is entirely optional), there's just costs
to not providing it in terms of what features you can get from the
regmap API and the performance of the system.  Not every device is going
to be bothered by those costs, many devices don't provide all of the
data they could.

I'm not clear to me that Lochnagar will particularly benefit from
providing the cache defaults but it sounds like you've raised concerns
about other devices which would, and it seems clear that the readability
information is very useful for this device if there's registers that
it's unsafe to try to read from.

> > > Even if it is absolutely critical that you have to supply these to
> > > Regmap up-front, instead of on first use/read, why can't you just
> > > supply the oddball non-zero ones?

That doesn't work, we need to know both if the register has a default
value and what that value is - there's no great value in only supplying
the defaults for registers with non-zero values.

> > If you aren't happy with the regmap subsystem you could send some
> > patches to change it to what you would be happy with (and patch the ~1300
> > drivers that use it)

> That may well happen.  This is the pre-patch discussion.

> Apologies that it had to happen on your submission, but this is that
> alerted me to the issue(s).

The regmap cache API has been this way since it was introduced in 2011
FWIW, we did add range based support later which is purely data driven.

> > Like any kernel subsystem it has an API that we have to obey to be able to
> > use it.

> Again, this isn't about Lochnagar.

I think from the perspective of Richard and Charles who are just trying
to get their driver merged this is something of an abstract distinction.
If the driver were merged and this discussion were happening separately
their perspective would most likely be different.

> > > The API is obscene and needs a re-work IMHO.

> > > I really hope we do not really have to list every register, but *if we
> > > absolutely must*, let's do it once:

> > >    REG_ADDRESS, WRV, INITIAL_VALUE

There is the option to specify range based access tables instead of a
function, for a lot of devices this is a nice, easy way to specify
things that makes more sense so we support it.  We don't combine the
tables because they're range based and if there is 100% overlap you can
always just point at the same table.

We allow the functions partly because it lets people handle weird cases
but also because it turned out when I looked at this that a lot of the
time the compiler output for switch statements was pretty efficient with
sparse register maps and it makes the code incredibly simple, much
simpler than trying to parse access tables into a more efficient data
structure and IIRC more compact too.  It's possible that those tradeoffs
have changed since but at the time there was a class of devices where it
wasn't clear that putting more effort in would result in substantially
better outcomes.

> > To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
> > so it's not our responsibility if you don't like it. Mark Brown is the maintainer.

> Sounds very much like you are saying, "it's not Cirrus' fault,
> therefore it is not my problem"?  Which is a terrible attitude.

I think from the perspective of Charles and Richard this is sounding an
awful lot like you want them (or someone else) to rewrite a very widely
used kernel API before they can get their driver merged.  To me that
would be a completely disproportionate amount of effort for their goal
but unfortunately people do get asked to do things like that so it's not
an unreasonable fear for them to have.

> Remember that the Linux kernel is an open community.  Anyone should be
> free to discuss any relevant issue.  If we decide to take this
> off-line, which is likely, then so be it.  In the mean time, as a
> contributor to this community project, it's absolutely your
> responsibly to help discuss and potentially solve wider issues than
> just your lines of code.

It's also a community of people with differing amounts of ability to
contribute, due to things like time, energy and so on.  Not everyone can
work on everything they want to, let alone other things people ask them
to look at.

> > As above, if one subsystem owner doesn't like another subsystem then those
> > subsystem owners should talk to each other and sort something out. It shouldn't
> > block patches that are just trying to use the subsystem as it currently exists
> > in the kernel.

> Again, no one is blocking this patch.

> This driver was submitted for review/discussion.  We are discussing.

I kind of said this above but just to be clear this driver seems to me
like an idiomatic user of the regmap API as it is today.  My guess is
that we could probably loose the defaults tables and not suffer too much
but equally well they don't hurt from a regmap point of view.

Reviwed-by: Mark Brown <broonie@kernel.org>
Lee Jones Oct. 29, 2018, 11:04 a.m. | #27
On Fri, 26 Oct 2018, Mark Brown wrote:
> On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
> > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> 
> > > Largely the point. How long do you think it would take to populate the
> > > cache if you had to read thousands of registers over I2C? Boot time matters.
> > > Deferring it until it's touched can create various unpredictable and
> > > annoying behaviour later, for example if a lot of cache entries are
> > > written while the chip is asleep and the initial values weren't known
> > > then a suspend/resume cannot filter out writes that are setting the
> > > register to its default (which regmap does to avoid unnecessary bus traffic).
> > > So the resume could have a large amount of unnecessary overhead writing
> > > registers to a value they already have or reading the initial values of
> > > those registers.
> 
> > One more register read when initially writing to a register and one
> > more when resuming doesn't sound like a vast amount of over-head.
> 
> Especially on resume extra register I/O really adds up - people really
> care how long their system takes to come back from suspend, and how
> quickly individual devices come back.  For devices that are on slow
> buses like I2C this means that every register operation counts.  Boot
> can be similarly pressured of course, though it's a less frequent issue
> for these devices.
> 
> > Not sure what you think I was suggesting above.  If the default values
> > are actually non-zero that's fine - we'll either leave them as they
> > are (if they are never changed, in which case Regmap doesn't even need
> > to know about them), document only those (non-zero) ones or wait until
> > they are read for the first time, then populate the cache.
> 
> You can't assume that the device is in power on reset state unless the
> driver reset it itself which may or may not be a good idea or even
> possible, sometimes it's what you want but at other times even if it's
> possible it can cause user visible disruption during the boot process
> which is undesirable.
> 
> > Setting up the cache manually also sounds like a vector for potential
> > failure.  At least if you were to cache dynamically on first write
> > (either on start-up or after sleep) then the actual value will be
> > cached, rather than what a piece of C code says it should be.
> 
> Even where there's no problem getting the hardware into a clean state it
> can rapidly get very, very expensive to do this with larger register
> sets on slow buses, and at the framework level we can't assume that
> readback support is even present on the device (the earliest versions of
> cache support were written to support such devices).  Some of the
> userspaces that regmap devices get used with end up wanting to apply a
> bunch of configuration at startup, if we can cut down on the amount of
> I/O that's involved in doing that it can help them quite a bit.  You
> also get userspaces that want to enumerate device state at startup,
> that's a bit easier to change in userspace but it's not an unreasonable
> thing to want to do and can also get very I/O heavy.
> 
> There is some potential for errors to be introduced but equally these
> tables can be both generated and verified mechanically, tasks that are
> particularly straightforward for the device vendors to do.  There are
> also potential risks in doing this at runtime if we didn't get the
> device reset, if we don't accurately mark the volatile registers as
> volatile or if there's otherwise bugs in the code.
> 
> > Precisely my point.  Lochnagar is a small device yet it's required to
> > submit hundreds of lines of Regmap tables.  Imagine what that would
> > look like for a large device.
> 
> There's no *requirement* to provide the data even if you're using the
> cache (and the cache support is entirely optional), there's just costs
> to not providing it in terms of what features you can get from the
> regmap API and the performance of the system.  Not every device is going
> to be bothered by those costs, many devices don't provide all of the
> data they could.

So what do we do in the case where, due to the size of the device, the
amount of lines required by these tables go from crazy to grotesque?

> I'm not clear to me that Lochnagar will particularly benefit from
> providing the cache defaults but it sounds like you've raised concerns
> about other devices which would, and it seems clear that the readability
> information is very useful for this device if there's registers that
> it's unsafe to try to read from.

Any reduction in lines would be a good thing.  Charles, could you
please define what specific benefits you gain from providing providing
the pre-cache data please?  With a particular emphasis on whether the
trade-off is justified.

> > > > Even if it is absolutely critical that you have to supply these to
> > > > Regmap up-front, instead of on first use/read, why can't you just
> > > > supply the oddball non-zero ones?
> 
> That doesn't work, we need to know both if the register has a default
> value and what that value is - there's no great value in only supplying
> the defaults for registers with non-zero values.

All registers have a default value.  Why can't we assume that if a
register is writable and a default value was omitted then the default
is zero?

Ah wait!  As I was typing the above, I just had a thought.  We don't
actually provide a list of writable registers do we?  Only a the
ability to verify if one is writable (presumably) before a write.

That's frustrating!

> > > If you aren't happy with the regmap subsystem you could send some
> > > patches to change it to what you would be happy with (and patch the ~1300
> > > drivers that use it)
> 
> > That may well happen.  This is the pre-patch discussion.
> 
> > Apologies that it had to happen on your submission, but this is that
> > alerted me to the issue(s).
> 
> The regmap cache API has been this way since it was introduced in 2011
> FWIW, we did add range based support later which is purely data driven.

Utilising range support here would certainly help IMHO.

> > > Like any kernel subsystem it has an API that we have to obey to be able to
> > > use it.
> 
> > Again, this isn't about Lochnagar.
> 
> I think from the perspective of Richard and Charles who are just trying
> to get their driver merged this is something of an abstract distinction.
> If the driver were merged and this discussion were happening separately
> their perspective would most likely be different.

Charles has already mentioned that he'd take a look at the current
*use* (not changing the API, but the way in which Lochnagar
*uses/consumes* it).  Actions which would be most welcomed.

> > > > The API is obscene and needs a re-work IMHO.
> 
> > > > I really hope we do not really have to list every register, but *if we
> > > > absolutely must*, let's do it once:
> 
> > > >    REG_ADDRESS, WRV, INITIAL_VALUE
> 
> There is the option to specify range based access tables instead of a
> function, for a lot of devices this is a nice, easy way to specify
> things that makes more sense so we support it.  We don't combine the
> tables because they're range based and if there is 100% overlap you can
> always just point at the same table.
> 
> We allow the functions partly because it lets people handle weird cases
> but also because it turned out when I looked at this that a lot of the
> time the compiler output for switch statements was pretty efficient with
> sparse register maps and it makes the code incredibly simple, much
> simpler than trying to parse access tables into a more efficient data
> structure and IIRC more compact too.  It's possible that those tradeoffs
> have changed since but at the time there was a class of devices where it
> wasn't clear that putting more effort in would result in substantially
> better outcomes.
> 
> > > To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
> > > so it's not our responsibility if you don't like it. Mark Brown is the maintainer.
> 
> > Sounds very much like you are saying, "it's not Cirrus' fault,
> > therefore it is not my problem"?  Which is a terrible attitude.
> 
> I think from the perspective of Charles and Richard this is sounding an
> awful lot like you want them (or someone else) to rewrite a very widely
> used kernel API before they can get their driver merged.  To me that
> would be a completely disproportionate amount of effort for their goal
> but unfortunately people do get asked to do things like that so it's not
> an unreasonable fear for them to have.

I would see that as an unreasonable request.

To be clear, that is *not* what I am asking.

> > Remember that the Linux kernel is an open community.  Anyone should be
> > free to discuss any relevant issue.  If we decide to take this
> > off-line, which is likely, then so be it.  In the mean time, as a
> > contributor to this community project, it's absolutely your
> > responsibly to help discuss and potentially solve wider issues than
> > just your lines of code.
> 
> It's also a community of people with differing amounts of ability to
> contribute, due to things like time, energy and so on.  Not everyone can
> work on everything they want to, let alone other things people ask them
> to look at.

I'm not asking for code submission.  Merely contributing to this
discussion, or simply allowing it to happen on the back of one of
their submission is enough.

Denouncing all responsibility and a lack of care is not okay.

> > > As above, if one subsystem owner doesn't like another subsystem then those
> > > subsystem owners should talk to each other and sort something out. It shouldn't
> > > block patches that are just trying to use the subsystem as it currently exists
> > > in the kernel.
> 
> > Again, no one is blocking this patch.
> 
> > This driver was submitted for review/discussion.  We are discussing.
> 
> I kind of said this above but just to be clear this driver seems to me
> like an idiomatic user of the regmap API as it is today.  My guess is
> that we could probably loose the defaults tables and not suffer too much
> but equally well they don't hurt from a regmap point of view.

Perhaps Charles could elaborate on whether this is possible or not?

> Reviwed-by: Mark Brown <broonie@kernel.org>

Thanks.
Richard Fitzgerald Oct. 29, 2018, 11:52 a.m. | #28
On 29/10/18 11:04, Lee Jones wrote:
> On Fri, 26 Oct 2018, Mark Brown wrote:
>> On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
>>> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
>>
>>>> Largely the point. How long do you think it would take to populate the
>>>> cache if you had to read thousands of registers over I2C? Boot time matters.
>>>> Deferring it until it's touched can create various unpredictable and
>>>> annoying behaviour later, for example if a lot of cache entries are
>>>> written while the chip is asleep and the initial values weren't known
>>>> then a suspend/resume cannot filter out writes that are setting the
>>>> register to its default (which regmap does to avoid unnecessary bus traffic).
>>>> So the resume could have a large amount of unnecessary overhead writing
>>>> registers to a value they already have or reading the initial values of
>>>> those registers.
>>
>>> One more register read when initially writing to a register and one
>>> more when resuming doesn't sound like a vast amount of over-head.
>>
>> Especially on resume extra register I/O really adds up - people really
>> care how long their system takes to come back from suspend, and how
>> quickly individual devices come back.  For devices that are on slow
>> buses like I2C this means that every register operation counts.  Boot
>> can be similarly pressured of course, though it's a less frequent issue
>> for these devices.
>>
>>> Not sure what you think I was suggesting above.  If the default values
>>> are actually non-zero that's fine - we'll either leave them as they
>>> are (if they are never changed, in which case Regmap doesn't even need
>>> to know about them), document only those (non-zero) ones or wait until
>>> they are read for the first time, then populate the cache.
>>
>> You can't assume that the device is in power on reset state unless the
>> driver reset it itself which may or may not be a good idea or even
>> possible, sometimes it's what you want but at other times even if it's
>> possible it can cause user visible disruption during the boot process
>> which is undesirable.
>>
>>> Setting up the cache manually also sounds like a vector for potential
>>> failure.  At least if you were to cache dynamically on first write
>>> (either on start-up or after sleep) then the actual value will be
>>> cached, rather than what a piece of C code says it should be.
>>
>> Even where there's no problem getting the hardware into a clean state it
>> can rapidly get very, very expensive to do this with larger register
>> sets on slow buses, and at the framework level we can't assume that
>> readback support is even present on the device (the earliest versions of
>> cache support were written to support such devices).  Some of the
>> userspaces that regmap devices get used with end up wanting to apply a
>> bunch of configuration at startup, if we can cut down on the amount of
>> I/O that's involved in doing that it can help them quite a bit.  You
>> also get userspaces that want to enumerate device state at startup,
>> that's a bit easier to change in userspace but it's not an unreasonable
>> thing to want to do and can also get very I/O heavy.
>>
>> There is some potential for errors to be introduced but equally these
>> tables can be both generated and verified mechanically, tasks that are
>> particularly straightforward for the device vendors to do.  There are
>> also potential risks in doing this at runtime if we didn't get the
>> device reset, if we don't accurately mark the volatile registers as
>> volatile or if there's otherwise bugs in the code.
>>
>>> Precisely my point.  Lochnagar is a small device yet it's required to
>>> submit hundreds of lines of Regmap tables.  Imagine what that would
>>> look like for a large device.
>>
>> There's no *requirement* to provide the data even if you're using the
>> cache (and the cache support is entirely optional), there's just costs
>> to not providing it in terms of what features you can get from the
>> regmap API and the performance of the system.  Not every device is going
>> to be bothered by those costs, many devices don't provide all of the
>> data they could.
> 
> So what do we do in the case where, due to the size of the device, the
> amount of lines required by these tables go from crazy to grotesque?
> 
>> I'm not clear to me that Lochnagar will particularly benefit from
>> providing the cache defaults but it sounds like you've raised concerns
>> about other devices which would, and it seems clear that the readability
>> information is very useful for this device if there's registers that
>> it's unsafe to try to read from.
> 
> Any reduction in lines would be a good thing.  Charles, could you
> please define what specific benefits you gain from providing providing
> the pre-cache data please?  With a particular emphasis on whether the
> trade-off is justified.
> 

Why so much concern over the number of source lines of a data table?
If we were talking about removing lines of executable code to make it
more efficient - yes, that's a good thing.
Worrying about the number of lines of a data table, I don't see the
imperative for that.

Since this seems to be a significant part of your objection it would help
if you could tell us WHY you object so much to lines of tables.

>>>>> Even if it is absolutely critical that you have to supply these to
>>>>> Regmap up-front, instead of on first use/read, why can't you just
>>>>> supply the oddball non-zero ones?
>>
>> That doesn't work, we need to know both if the register has a default
>> value and what that value is - there's no great value in only supplying
>> the defaults for registers with non-zero values.
> 
> All registers have a default value.  Why can't we assume that if a
> register is writable and a default value was omitted then the default
> is zero?
> 
> Ah wait!  As I was typing the above, I just had a thought.  We don't
> actually provide a list of writable registers do we?  Only a the
> ability to verify if one is writable (presumably) before a write.
> 
> That's frustrating!
> 
>>>> If you aren't happy with the regmap subsystem you could send some
>>>> patches to change it to what you would be happy with (and patch the ~1300
>>>> drivers that use it)
>>
>>> That may well happen.  This is the pre-patch discussion.
>>
>>> Apologies that it had to happen on your submission, but this is that
>>> alerted me to the issue(s).
>>
>> The regmap cache API has been this way since it was introduced in 2011
>> FWIW, we did add range based support later which is purely data driven.
> 
> Utilising range support here would certainly help IMHO.
> 
>>>> Like any kernel subsystem it has an API that we have to obey to be able to
>>>> use it.
>>
>>> Again, this isn't about Lochnagar.
>>
>> I think from the perspective of Richard and Charles who are just trying
>> to get their driver merged this is something of an abstract distinction.
>> If the driver were merged and this discussion were happening separately
>> their perspective would most likely be different.
> 
> Charles has already mentioned that he'd take a look at the current
> *use* (not changing the API, but the way in which Lochnagar
> *uses/consumes* it).  Actions which would be most welcomed.
>

Maybe Charles will find an alternative that doesn't affect performance/functionality
or is an acceptable tradeoff. But still, it seems an odd maintainer requirement
"please use this other subsystem less effectively to make _my_ subsystem have fewer
lines of source".

>>>>> The API is obscene and needs a re-work IMHO.
>>
>>>>> I really hope we do not really have to list every register, but *if we
>>>>> absolutely must*, let's do it once:
>>
>>>>>     REG_ADDRESS, WRV, INITIAL_VALUE
>>
>> There is the option to specify range based access tables instead of a
>> function, for a lot of devices this is a nice, easy way to specify
>> things that makes more sense so we support it.  We don't combine the
>> tables because they're range based and if there is 100% overlap you can
>> always just point at the same table.
>>
>> We allow the functions partly because it lets people handle weird cases
>> but also because it turned out when I looked at this that a lot of the
>> time the compiler output for switch statements was pretty efficient with
>> sparse register maps and it makes the code incredibly simple, much
>> simpler than trying to parse access tables into a more efficient data
>> structure and IIRC more compact too.  It's possible that those tradeoffs
>> have changed since but at the time there was a class of devices where it
>> wasn't clear that putting more effort in would result in substantially
>> better outcomes.
>>
>>>> To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
>>>> so it's not our responsibility if you don't like it. Mark Brown is the maintainer.
>>
>>> Sounds very much like you are saying, "it's not Cirrus' fault,
>>> therefore it is not my problem"?  Which is a terrible attitude.
>>
>> I think from the perspective of Charles and Richard this is sounding an
>> awful lot like you want them (or someone else) to rewrite a very widely
>> used kernel API before they can get their driver merged.  To me that
>> would be a completely disproportionate amount of effort for their goal
>> but unfortunately people do get asked to do things like that so it's not
>> an unreasonable fear for them to have.
> 
> I would see that as an unreasonable request.
> 
> To be clear, that is *not* what I am asking.
>

It sounded that way, so I apologize if that wasn't what you meant. It just
read as if you thought we were the owners (or only users) of regmap so we
can just go and change it as per your suggestion and then resend this patch.

>>> Remember that the Linux kernel is an open community.  Anyone should be
>>> free to discuss any relevant issue.  If we decide to take this
>>> off-line, which is likely, then so be it.  In the mean time, as a
>>> contributor to this community project, it's absolutely your
>>> responsibly to help discuss and potentially solve wider issues than
>>> just your lines of code.
>>
>> It's also a community of people with differing amounts of ability to
>> contribute, due to things like time, energy and so on.  Not everyone can
>> work on everything they want to, let alone other things people ask them
>> to look at.
> 
> I'm not asking for code submission.  Merely contributing to this
> discussion, or simply allowing it to happen on the back of one of
> their submission is enough.
> 
> Denouncing all responsibility and a lack of care is not okay.
>
>>>> As above, if one subsystem owner doesn't like another subsystem then those
>>>> subsystem owners should talk to each other and sort something out. It shouldn't
>>>> block patches that are just trying to use the subsystem as it currently exists
>>>> in the kernel.
>>
>>> Again, no one is blocking this patch.
>>
>>> This driver was submitted for review/discussion.  We are discussing.
>>
>> I kind of said this above but just to be clear this driver seems to me
>> like an idiomatic user of the regmap API as it is today.  My guess is
>> that we could probably loose the defaults tables and not suffer too much
>> but equally well they don't hurt from a regmap point of view.
> 
> Perhaps Charles could elaborate on whether this is possible or not?
> 
>> Reviwed-by: Mark Brown <broonie@kernel.org>
> 
> Thanks.
>
Richard Fitzgerald Oct. 29, 2018, 12:36 p.m. | #29
On 29/10/18 11:04, Lee Jones wrote:
> On Fri, 26 Oct 2018, Mark Brown wrote:
>> On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
>>> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
>>
>>>> Largely the point. How long do you think it would take to populate the
>>>> cache if you had to read thousands of registers over I2C? Boot time matters.
>>>> Deferring it until it's touched can create various unpredictable and
>>>> annoying behaviour later, for example if a lot of cache entries are
>>>> written while the chip is asleep and the initial values weren't known
>>>> then a suspend/resume cannot filter out writes that are setting the
>>>> register to its default (which regmap does to avoid unnecessary bus traffic).
>>>> So the resume could have a large amount of unnecessary overhead writing
>>>> registers to a value they already have or reading the initial values of
>>>> those registers.
>>
>>> One more register read when initially writing to a register and one
>>> more when resuming doesn't sound like a vast amount of over-head.
>>
>> Especially on resume extra register I/O really adds up - people really
>> care how long their system takes to come back from suspend, and how
>> quickly individual devices come back.  For devices that are on slow
>> buses like I2C this means that every register operation counts.  Boot
>> can be similarly pressured of course, though it's a less frequent issue
>> for these devices.
>>
>>> Not sure what you think I was suggesting above.  If the default values
>>> are actually non-zero that's fine - we'll either leave them as they
>>> are (if they are never changed, in which case Regmap doesn't even need
>>> to know about them), document only those (non-zero) ones or wait until
>>> they are read for the first time, then populate the cache.
>>
>> You can't assume that the device is in power on reset state unless the
>> driver reset it itself which may or may not be a good idea or even
>> possible, sometimes it's what you want but at other times even if it's
>> possible it can cause user visible disruption during the boot process
>> which is undesirable.
>>
>>> Setting up the cache manually also sounds like a vector for potential
>>> failure.  At least if you were to cache dynamically on first write
>>> (either on start-up or after sleep) then the actual value will be
>>> cached, rather than what a piece of C code says it should be.
>>
>> Even where there's no problem getting the hardware into a clean state it
>> can rapidly get very, very expensive to do this with larger register
>> sets on slow buses, and at the framework level we can't assume that
>> readback support is even present on the device (the earliest versions of
>> cache support were written to support such devices).  Some of the
>> userspaces that regmap devices get used with end up wanting to apply a
>> bunch of configuration at startup, if we can cut down on the amount of
>> I/O that's involved in doing that it can help them quite a bit.  You
>> also get userspaces that want to enumerate device state at startup,
>> that's a bit easier to change in userspace but it's not an unreasonable
>> thing to want to do and can also get very I/O heavy.
>>
>> There is some potential for errors to be introduced but equally these
>> tables can be both generated and verified mechanically, tasks that are
>> particularly straightforward for the device vendors to do.  There are
>> also potential risks in doing this at runtime if we didn't get the
>> device reset, if we don't accurately mark the volatile registers as
>> volatile or if there's otherwise bugs in the code.
>>
>>> Precisely my point.  Lochnagar is a small device yet it's required to
>>> submit hundreds of lines of Regmap tables.  Imagine what that would
>>> look like for a large device.
>>
>> There's no *requirement* to provide the data even if you're using the
>> cache (and the cache support is entirely optional), there's just costs
>> to not providing it in terms of what features you can get from the
>> regmap API and the performance of the system.  Not every device is going
>> to be bothered by those costs, many devices don't provide all of the
>> data they could.
> 
> So what do we do in the case where, due to the size of the device, the
> amount of lines required by these tables go from crazy to grotesque?
> 
>> I'm not clear to me that Lochnagar will particularly benefit from
>> providing the cache defaults but it sounds like you've raised concerns
>> about other devices which would, and it seems clear that the readability
>> information is very useful for this device if there's registers that
>> it's unsafe to try to read from.
> 
> Any reduction in lines would be a good thing.  Charles, could you
> please define what specific benefits you gain from providing providing
> the pre-cache data please?  With a particular emphasis on whether the
> trade-off is justified.
> 
>>>>> Even if it is absolutely critical that you have to supply these to
>>>>> Regmap up-front, instead of on first use/read, why can't you just
>>>>> supply the oddball non-zero ones?
>>
>> That doesn't work, we need to know both if the register has a default
>> value and what that value is - there's no great value in only supplying
>> the defaults for registers with non-zero values.
> 
> All registers have a default value.  Why can't we assume that if a
> register is writable and a default value was omitted then the default
> is zero?
> 
> Ah wait!  As I was typing the above, I just had a thought.  We don't
> actually provide a list of writable registers do we?  Only a the
> ability to verify if one is writable (presumably) before a write.
> 
> That's frustrating!
> 
>>>> If you aren't happy with the regmap subsystem you could send some
>>>> patches to change it to what you would be happy with (and patch the ~1300
>>>> drivers that use it)
>>
>>> That may well happen.  This is the pre-patch discussion.
>>
>>> Apologies that it had to happen on your submission, but this is that
>>> alerted me to the issue(s).
>>
>> The regmap cache API has been this way since it was introduced in 2011
>> FWIW, we did add range based support later which is purely data driven.
> 
> Utilising range support here would certainly help IMHO.
> 
>>>> Like any kernel subsystem it has an API that we have to obey to be able to
>>>> use it.
>>
>>> Again, this isn't about Lochnagar.
>>
>> I think from the perspective of Richard and Charles who are just trying
>> to get their driver merged this is something of an abstract distinction.
>> If the driver were merged and this discussion were happening separately
>> their perspective would most likely be different.
> 
> Charles has already mentioned that he'd take a look at the current
> *use* (not changing the API, but the way in which Lochnagar
> *uses/consumes* it).  Actions which would be most welcomed.
> 
>>>>> The API is obscene and needs a re-work IMHO.
>>
>>>>> I really hope we do not really have to list every register, but *if we
>>>>> absolutely must*, let's do it once:
>>
>>>>>     REG_ADDRESS, WRV, INITIAL_VALUE
>>
>> There is the option to specify range based access tables instead of a
>> function, for a lot of devices this is a nice, easy way to specify
>> things that makes more sense so we support it.  We don't combine the
>> tables because they're range based and if there is 100% overlap you can
>> always just point at the same table.
>>
>> We allow the functions partly because it lets people handle weird cases
>> but also because it turned out when I looked at this that a lot of the
>> time the compiler output for switch statements was pretty efficient with
>> sparse register maps and it makes the code incredibly simple, much
>> simpler than trying to parse access tables into a more efficient data
>> structure and IIRC more compact too.  It's possible that those tradeoffs
>> have changed since but at the time there was a class of devices where it
>> wasn't clear that putting more effort in would result in substantially
>> better outcomes.
>>
>>>> To re-iterate, regmap is a standard kernel subsystem. It's not owned by Cirrus,
>>>> so it's not our responsibility if you don't like it. Mark Brown is the maintainer.
>>
>>> Sounds very much like you are saying, "it's not Cirrus' fault,
>>> therefore it is not my problem"?  Which is a terrible attitude.
>>
>> I think from the perspective of Charles and Richard this is sounding an
>> awful lot like you want them (or someone else) to rewrite a very widely
>> used kernel API before they can get their driver merged.  To me that
>> would be a completely disproportionate amount of effort for their goal
>> but unfortunately people do get asked to do things like that so it's not
>> an unreasonable fear for them to have.
> 
> I would see that as an unreasonable request.
> 
> To be clear, that is *not* what I am asking.
> 
>>> Remember that the Linux kernel is an open community.  Anyone should be
>>> free to discuss any relevant issue.  If we decide to take this
>>> off-line, which is likely, then so be it.  In the mean time, as a
>>> contributor to this community project, it's absolutely your
>>> responsibly to help discuss and potentially solve wider issues than
>>> just your lines of code.
>>
>> It's also a community of people with differing amounts of ability to
>> contribute, due to things like time, energy and so on.  Not everyone can
>> work on everything they want to, let alone other things people ask them
>> to look at.
> 
> I'm not asking for code submission.  Merely contributing to this
> discussion, or simply allowing it to happen on the back of one of
> their submission is enough.
> 
> Denouncing all responsibility and a lack of care is not okay.
> 
>>>> As above, if one subsystem owner doesn't like another subsystem then those
>>>> subsystem owners should talk to each other and sort something out. It shouldn't
>>>> block patches that are just trying to use the subsystem as it currently exists
>>>> in the kernel.
>>
>>> Again, no one is blocking this patch.
>>
>>> This driver was submitted for review/discussion.  We are discussing.
>>
>> I kind of said this above but just to be clear this driver seems to me
>> like an idiomatic user of the regmap API as it is today.  My guess is
>> that we could probably loose the defaults tables and not suffer too much
>> but equally well they don't hurt from a regmap point of view.
> 
> Perhaps Charles could elaborate on whether this is possible or not?
> 
>> Reviwed-by: Mark Brown <broonie@kernel.org>
> 
> Thanks.
> 

If we're going to need to change regmap to get drivers that use regmap accepted into
MFD (at least without crippling them), can Lee or Mark please create a separate discussion
thread for that, and include the major contributors/users of regmap so Lee can air his
objections and proposals to a more appropriate group of people and we can get feedback
from the right people, and hopefully come to some sort of decision.
Mark Brown Oct. 29, 2018, 12:57 p.m. | #30
On Mon, Oct 29, 2018 at 11:04:39AM +0000, Lee Jones wrote:
> On Fri, 26 Oct 2018, Mark Brown wrote:

> > There's no *requirement* to provide the data even if you're using the
> > cache (and the cache support is entirely optional), there's just costs
> > to not providing it in terms of what features you can get from the
> > regmap API and the performance of the system.  Not every device is going
> > to be bothered by those costs, many devices don't provide all of the
> > data they could.

> So what do we do in the case where, due to the size of the device, the
> amount of lines required by these tables go from crazy to grotesque?

My recommendation when the tables get in the way of the rest of the
driver is to move them into a separate file that contains only tables,
these get big but they're sitting in a separate file that only contains
big data tables so they're fairly simple to look at (and generate or
whatever) and people working on the active code don't need to look at
them.

> > That doesn't work, we need to know both if the register has a default
> > value and what that value is - there's no great value in only supplying
> > the defaults for registers with non-zero values.

> All registers have a default value.  Why can't we assume that if a
> register is writable and a default value was omitted then the default
> is zero?

There are volatile registers which can't be part of the cache as the
hardware might change the state, and we have things like device
identification registers which are fixed but which we want to read from
the device.  This second set of registers can usually be modelled quite
happily as volatile registers though, we don't tend to read them often.

There's also registers where the user just didn't tell us what's going
on, either through oversight or because there's some code that touches
undocumented test registers at runtime for quirk reasons using a read,
modify write cycle.  We could try to insist that the device drivers
always provide a default or mark things as volatile but that's likely to
be an uphill struggle and more error prone.

> Ah wait!  As I was typing the above, I just had a thought.  We don't
> actually provide a list of writable registers do we?  Only a the
> ability to verify if one is writable (presumably) before a write.

> That's frustrating!

You can provide the writability information either with the function or
with data.  The tables code doesn't currently scale very well to large,
sparse register maps but that could in theory be optimized.  Few devices
bother providing distinct writability information as it's not often an
issue.
Charles Keepax Nov. 1, 2018, 10:28 a.m. | #31
On Mon, Oct 29, 2018 at 11:04:39AM +0000, Lee Jones wrote:
> On Fri, 26 Oct 2018, Mark Brown wrote:
> > On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
> > > On Thu, 25 Oct 2018, Richard Fitzgerald wrote:

I have re-ordered some of the quotes here for the benefit
of clarity. I will start with the Lochnagar focused bits
and then cover some of the more general regmap discussion.
Apologies for the wall of text towards the end but it seemed
wise to explore some of the why for parts of the current regmap
implementation and some of the implications for changing it.

> > I think from the perspective of Richard and Charles who are just trying
> > to get their driver merged this is something of an abstract distinction.
> > If the driver were merged and this discussion were happening separately
> > their perspective would most likely be different.
>
> Charles has already mentioned that he'd take a look at the current
> *use* (not changing the API, but the way in which Lochnagar
> *uses/consumes* it).  Actions which would be most welcomed.
>
> > I kind of said this above but just to be clear this driver seems to me
> > like an idiomatic user of the regmap API as it is today.  My guess is
> > that we could probably loose the defaults tables and not suffer too much
> > but equally well they don't hurt from a regmap point of view.
>
> Perhaps Charles could elaborate on whether this is possible or not?

So on this front I have had a look through and we can drop the
defaults tables for Lochnagar, although as I shall cover later
Lochnagar is the exceptional case with respect to our normal
devices.

> Utilising range support here would certainly help IMHO.
>

I am rather hesitant to switch to the range API. Whilst it is
significantly more clear in certain situations, such as say
mapping out the memory for a DSP, where there exist large
amorphis blobs of identical function registers. I am really
not convinced it really suits something like the register map
that controls Lochnagar. You have an intertwinned mix of
various purpose registers, each with a clearly defined name
and potentially with quite fine grained properties.

Certainly when working with the driver I want to be able to
fairly quickly see what registers are marked as by name. The
callbacks are very simple and I don't need to look up where
register are in the regmap to be able check their attributes.
But perhaps I have just got too used to seeing these callbacks,
things do have a way of becoming normal after being exposed to
them for a while.

What I will try for the next spin is to try to use as much:

  case REG1...REG2:

As I can without totally sacrificing grepability/clarity and
hopefully that will get us to a compromise we can all live
with at least for now. Lochnagar is a fairly small part so it
feels like this should be doable.

> > > > +       ret = devm_of_platform_populate(dev);
> > > > +       if (ret < 0) {
> > > > +               dev_err(dev, "Failed to populate child
> > > > nodes:
> > > > %d\n", ret);
> > > > +               return ret;
> > > > +       }
> > >
> > > Please do not mix OF and MFD device registration
> > > strategies.
> > >
> > > Pick one and register all devices through your chosen
> > > method.
> >
> > Hmmm we use this to do things like register some fixed
> > regulators
> > and clocks that don't need any control but do need to be
> > associated 
> > with the device. I could do that through the MFD although it
> > is in 
> > direct conflict with the feedback on the clock patches I
> > received
> > to move the fixed clocks into devicetree rather than
> > registering
> > them manually (see v2 of the patch chain).
>
> The I suggest moving everything to DT.

So pulling this out from earlier discussions in this thread,
it seems I can happily move all the child device registration
into device tree. I will also try this for the next version of
the patch, unless anyone wants to object? But it does change
the DT binding quite a lot as the individual sub drivers now
each require their own node rather than one single unified
Lochnagar node.

> > > Precisely my point.  Lochnagar is a small device yet it's required to
> > > submit hundreds of lines of Regmap tables.  Imagine what that would
> > > look like for a large device.
> >
> > There's no *requirement* to provide the data even if you're using the
> > cache (and the cache support is entirely optional), there's just costs
> > to not providing it in terms of what features you can get from the
> > regmap API and the performance of the system.  Not every device is going
> > to be bothered by those costs, many devices don't provide all of the
> > data they could.
>
> So what do we do in the case where, due to the size of the device, the
> amount of lines required by these tables go from crazy to grotesque?
>

Ultimately, I guess I have always just viewed it as just data
tables. Its a lot of lines of source but its not complicated,
and complexity has really always been the thing I try to avoid.

> > > > > Even if it is absolutely critical that you have to supply these to
> > > > > Regmap up-front, instead of on first use/read, why can't you just
> > > > > supply the oddball non-zero ones?
> > 
> > That doesn't work, we need to know both if the register has a default
> > value and what that value is - there's no great value in only supplying
> > the defaults for registers with non-zero values.
> 
> All registers have a default value.  Why can't we assume that if a
> register is writable and a default value was omitted then the default
> is zero?

Defaults basically serve two purposes in regmap:

 1) Initialise the register cache.

    There are basically three ways you could handle this
    (at least that I can think of) and regmap supports all
    three. Obviously each with their own pros and cons:

     1.1) Table of default values
          + Fast boot time
          - Uses some additional memory
     1.2) Read all the registers from the device on boot
          + Uses less memory
          - Potentially very slow boot time
     1.3) Only read values as you touch registers
          + Uses less memory
          + Usually no significant impact on boot time
          - Can't do read or read/write/modify operations on
            previously untouched registers whilst chip is off

    1.3 does probably make sense here for Lochnagar since we
    don't currently power things down. However, it is worth
    noting that such an approach is extremely challenging for many
    devices. For example the CODECs generally have all sorts of
    actual user-facing interface that needs to be accessible
    regardless of if the CODEC is powered up or down and powering it
    up to access registers would end up being either horrific on
    power consumption and/or horrific in terms of code complexity.

    1.1 and 1.2 are basically a straight trade off. Generally
    for our devices we talking loads of registers and
    potentially connected over I2C. Our customers care deeply
    about device boot time, Android has specs for such things
    and often it is tight for a system to make those specs.
    Conversly, generally the products we integrate into have
    a fairly large amount of memory. As such this is a no
    brainer of a choice for most of our devices.

 2) Determine what registers should be synchronised on a cache
    sync.

    A cache sync is usually done when pulling a device out of
    low power mode to reapply the currently desired register
    settings. As discussed in 1) we don't currently do cache
    syncs on Lochnagar, but this would affect most of our
    parts. Again I can see three approaches to synchronising
    the cache:

     2.1) Sync out registers that arn't at their default value
          + Only syncs register that are actually needed
          - Requires a table of defaults to work
     2.2) Store a per register dirty flag in the cache
          + No up front table required in the driver
          + Potentially less memory as only a single bit required
            per register, although realising that saving might be
            very hard on some cache types
          - May sync registers that arn't required
     2.3) Sync all registers from the cache
          + No memory usage
          - Potentially large penalty for cache sync

    Regmap has options to do 2.3, however for most of our
    devices that would be totally unacceptable. Our parts have
    a lot of registers most of which are cacheable and for
    power reasons cache syncs happen as the audio path is being
    brought up. Again Android has targets here and they are
    in low 10's of millseconds so bus accesses really do matter.

    2.1 is the normal proceedure in regmap, although this
    is defined on a per cache implementation basis, with the
    core providing 2.1 as a default.

    Again it's a bit of a trade off between 2.1 and 2.2, but
    given 1 pretty much requires us to know the defaults
    anyway, 2.1 will in general make the most sense, at least
    for Cirrus parts.

So I would conclude, certainly for most Cirrus devices, we
do need to know the defaults at least for the vast majority
of registers. I guess the next question would be could we
generate some of the defaults table to reduce the table size
in the driver. I would agree with you that the only sensible
approach to reducing the defaults table size I can see would be
to not have to specify defaults for registers with a default
of zero. As an example to set expectations cs47l90, probably
the largest CODEC we have made, has 1357 defaults of which
693 are zero so ~50%.

The issue really boils down to how do we tell the difference
between a register that has no default, and one that has a default
of zero? There are a few problems I can foresee on this front:

 1) There are occasions where people use a readable,
    non-volatile register with no default for things like
    device ID or revision. The idea being it can be read once
    which will populate it into the cache then it never needs
    to be read from the hardware again. Especially useful if
    this information might be required whilst the hardware
    is powered down.

    We could potentially update such drivers to mark the
    register as volatile and then store the value explicitly
    in the drivers data structures?

 2) There are occasions where a readable, writeable,
    non-volatile register cannot be given a fixed default.
    Usually this will be registers that are configured by
    firmware or hardware during boot but then have control
    passed to the kernel.

    For example a couple of registers on Lochnagar will be
    configured by the board controller itself depending on
    the CODEC that is attached, mostly regulators that are
    enabled during boot being set to appropriate voltages to
    avoid hardware damage. To handle these we don't give them
    defaults which forces a read from the device but then after
    that we can use the cache.

    For this one would probably have to add a new register
    property (in addition to the existing readable, writeable,
    volatile, and precious) for no default. Which would require
    an additional callback. Although I guess that would cover 1
    as well, and normally there would be very few registers in
    this catagory.

 3) Rather nervous there are other issues I am not currently
    thinking of this is a widely used API and I am mostly
    familiar only with the style of devices I work with.

    We could potentially add an assume zero defaults flag,
    that would at least then only apply the change to devices
    that opt in?

One other related thing that rests on my mind is that creating
the defaults table is going to be a little intensive. I guess
it is not so bad if using the ranges API, so perhaps we would
need to tie it in with that. Although it is still a little
fiddly as you need to work out overlaps between the ranges for
different properties to be more efficient than just checking
each register.  For the callback based systems though you
would have to check each register and for example coming
back to cs47l90, the highest register address is 0x3FFE7C
which means you would need to call over 4 million callbacks
probably multiple times whilst constructing your defaults table.

> Ah wait!  As I was typing the above, I just had a thought.  We don't
> actually provide a list of writable registers do we?  Only a the
> ability to verify if one is writable (presumably) before a write.

You can mark registers as writable. Its just that most drivers
don't bother to define the writable registers, there isn't
presently a great reason to do so and in that case the core
defaults to all register addresses being writable. I guess if
we added the automatic zero defaults you might well want to
start adding this callback though.

> > > > > The API is obscene and needs a re-work IMHO.
> > 
> > > > > I really hope we do not really have to list every register, but *if we
> > > > > absolutely must*, let's do it once:
> > 
> > > > >    REG_ADDRESS, WRV, INITIAL_VALUE
> > 
> > There is the option to specify range based access tables instead of a
> > function, for a lot of devices this is a nice, easy way to specify
> > things that makes more sense so we support it.  We don't combine the
> > tables because they're range based and if there is 100% overlap you can
> > always just point at the same table.
> > 
> > We allow the functions partly because it lets people handle weird cases
> > but also because it turned out when I looked at this that a lot of the
> > time the compiler output for switch statements was pretty efficient with
> > sparse register maps and it makes the code incredibly simple, much
> > simpler than trying to parse access tables into a more efficient data
> > structure and IIRC more compact too.  It's possible that those tradeoffs
> > have changed since but at the time there was a class of devices where it
> > wasn't clear that putting more effort in would result in substantially
> > better outcomes.
> > 

Exactly this, the regmap core makes a lot of readable/volatile
checks and the callbacks with switch statements are a very
runtime efficient way to implement those, in addition to the
code clarity.

I guess historically we have tried to be verbose as it
really made things very simple for us. We have scripts that
can generate the defaults and callbacks from outputs of the
hardware design giving us a very high degree of confidence in
them. Range based approaches always result in a bit of manual
work, unless we just literally pulled sequential ranges but that
results in code that is really not friendly when developing
the driver. But maybe that is the point we need to budge on
from the Cirrus side.

I think this really the crux of my concerns here about updating
the regmap API are that it feels like almost any path we take
from here results in worse runtime performance and probably
equal or worse memory usage. In return we gain a reduction of
some, whilst admittedly large, very simple data tables from
the driver. Which is a trade it feels hard to get invested in.

Anyway, well done to anyone who made it this far. I will
keep pondering on the issue, hopefully we can fudge things
for Lochnagar. But maybe we can come up with some longer term
compromise that devolves more of the regmap stuff through the
driver, hopefully reducing both the central data tables and the
large register include files both of which seem to be constant
pain points with review of parts. Although I am not sure how
much we can do for Arizona/Madera at this point but the parts
after that change the regmap quite dramatically so will need
a new driver so perhaps that is the point to look at things.

Thanks,
Charles
Richard Fitzgerald Nov. 1, 2018, 11:40 a.m. | #32
On 01/11/18 10:28, Charles Keepax wrote:
> On Mon, Oct 29, 2018 at 11:04:39AM +0000, Lee Jones wrote:
>> On Fri, 26 Oct 2018, Mark Brown wrote:
>>> On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
>>>> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> 
> I have re-ordered some of the quotes here for the benefit
> of clarity. I will start with the Lochnagar focused bits
> and then cover some of the more general regmap discussion.
> Apologies for the wall of text towards the end but it seemed
> wise to explore some of the why for parts of the current regmap
> implementation and some of the implications for changing it.
> 
>>> I think from the perspective of Richard and Charles who are just trying
>>> to get their driver merged this is something of an abstract distinction.
>>> If the driver were merged and this discussion were happening separately
>>> their perspective would most likely be different.
>>
>> Charles has already mentioned that he'd take a look at the current
>> *use* (not changing the API, but the way in which Lochnagar
>> *uses/consumes* it).  Actions which would be most welcomed.
>>
>>> I kind of said this above but just to be clear this driver seems to me
>>> like an idiomatic user of the regmap API as it is today.  My guess is
>>> that we could probably loose the defaults tables and not suffer too much
>>> but equally well they don't hurt from a regmap point of view.
>>
>> Perhaps Charles could elaborate on whether this is possible or not?
> 
> So on this front I have had a look through and we can drop the
> defaults tables for Lochnagar, although as I shall cover later
> Lochnagar is the exceptional case with respect to our normal
> devices.
> 
>> Utilising range support here would certainly help IMHO.
>>
> 
> I am rather hesitant to switch to the range API. Whilst it is
> significantly more clear in certain situations, such as say
> mapping out the memory for a DSP, where there exist large
> amorphis blobs of identical function registers. I am really
> not convinced it really suits something like the register map
> that controls Lochnagar. You have an intertwinned mix of
> various purpose registers, each with a clearly defined name
> and potentially with quite fine grained properties.
> 
> Certainly when working with the driver I want to be able to
> fairly quickly see what registers are marked as by name. The
> callbacks are very simple and I don't need to look up where
> register are in the regmap to be able check their attributes.
> But perhaps I have just got too used to seeing these callbacks,
> things do have a way of becoming normal after being exposed to
> them for a while.
> 
> What I will try for the next spin is to try to use as much:
> 
>    case REG1...REG2:
> 
> As I can without totally sacrificing grepability/clarity and
> hopefully that will get us to a compromise we can all live
> with at least for now. Lochnagar is a fairly small part so it
> feels like this should be doable.
> 
>>>>> +       ret = devm_of_platform_populate(dev);
>>>>> +       if (ret < 0) {
>>>>> +               dev_err(dev, "Failed to populate child
>>>>> nodes:
>>>>> %d\n", ret);
>>>>> +               return ret;
>>>>> +       }
>>>>
>>>> Please do not mix OF and MFD device registration
>>>> strategies.
>>>>
>>>> Pick one and register all devices through your chosen
>>>> method.
>>>
>>> Hmmm we use this to do things like register some fixed
>>> regulators
>>> and clocks that don't need any control but do need to be
>>> associated
>>> with the device. I could do that through the MFD although it
>>> is in
>>> direct conflict with the feedback on the clock patches I
>>> received
>>> to move the fixed clocks into devicetree rather than
>>> registering
>>> them manually (see v2 of the patch chain).
>>
>> The I suggest moving everything to DT.
> 
> So pulling this out from earlier discussions in this thread,
> it seems I can happily move all the child device registration
> into device tree. I will also try this for the next version of
> the patch, unless anyone wants to object? But it does change
> the DT binding quite a lot as the individual sub drivers now
> each require their own node rather than one single unified
> Lochnagar node.
> 

We went through this discussion with the Madera MFD patches. I had
originally implemented it using DT to register the child drivers and
it was nice in some ways each driver having its own node. But Mark
and Rob didn't like it so I went back to non-DT child registration with
all sharing the parent MFD node. It would be nice if we could stick to
one way of doing it so that Cirrus drivers don't flip-flop between
different styles of DT binding.

With the Madera MFD, since it now uses non-DT registration of children
if there was a reason we need to be able to register DT-defined children
(and there are potential uses like adding platform-specific virtual
regulators that are treated as a child) the bindings are now fixed so we
would end up having a mixed non-DT and DT registration.

<snip>
Mark Brown Nov. 1, 2018, 12:01 p.m. | #33
On Thu, Nov 01, 2018 at 10:28:28AM +0000, Charles Keepax wrote:

>      1.2) Read all the registers from the device on boot
>           + Uses less memory
>           - Potentially very slow boot time
>      1.3) Only read values as you touch registers
>           + Uses less memory
>           + Usually no significant impact on boot time
>           - Can't do read or read/write/modify operations on
>             previously untouched registers whilst chip is off

Both of these options also require that you be able to reset the device
which isn't possible with all devices and systems.

> I think this really the crux of my concerns here about updating
> the regmap API are that it feels like almost any path we take
> from here results in worse runtime performance and probably
> equal or worse memory usage. In return we gain a reduction of
> some, whilst admittedly large, very simple data tables from
> the driver. Which is a trade it feels hard to get invested in.

Right, I don't see an urgent problem either - if the tables were
complicated to think about it'd be a bit different but they aren't
particularly and like you say there's tradeoffs both at runtime and
at development time for trying to make the source code smaller.

Equally if there are useful things we can add to the API for some use
cases I'm all for that so long as they don't get in the way of other
users, most of the extensions in the API have come from someone having
problems and coming up with a fix that meets their needs.
Mark Brown Nov. 1, 2018, 12:04 p.m. | #34
On Thu, Nov 01, 2018 at 11:40:01AM +0000, Richard Fitzgerald wrote:
> On 01/11/18 10:28, Charles Keepax wrote:

> > So pulling this out from earlier discussions in this thread,
> > it seems I can happily move all the child device registration
> > into device tree. I will also try this for the next version of
> > the patch, unless anyone wants to object? But it does change
> > the DT binding quite a lot as the individual sub drivers now
> > each require their own node rather than one single unified
> > Lochnagar node.

> We went through this discussion with the Madera MFD patches. I had
> originally implemented it using DT to register the child drivers and
> it was nice in some ways each driver having its own node. But Mark
> and Rob didn't like it so I went back to non-DT child registration with
> all sharing the parent MFD node. It would be nice if we could stick to
> one way of doing it so that Cirrus drivers don't flip-flop between
> different styles of DT binding.

The basic concern I have is encoding the current Linux idea of how to
split the subfunctions up into drivers into an ABI - the clocks in CODEC
drivers is the obvious example, they might want to be in the clock API
in future.  If there's a very direct mapping onto individual hardware
blocks that worries me a lot less since it's more obviously reflecting
how the hardware is designed.
Richard Fitzgerald Nov. 1, 2018, 2:17 p.m. | #35
On 01/11/18 10:28, Charles Keepax wrote:
> On Mon, Oct 29, 2018 at 11:04:39AM +0000, Lee Jones wrote:
>> On Fri, 26 Oct 2018, Mark Brown wrote:
>>> On Fri, Oct 26, 2018 at 09:00:51AM +0100, Lee Jones wrote:
>>>> On Thu, 25 Oct 2018, Richard Fitzgerald wrote:
> 

<snip>

>>>> Precisely my point.  Lochnagar is a small device yet it's required to
>>>> submit hundreds of lines of Regmap tables.  Imagine what that would
>>>> look like for a large device.
>>>
>>> There's no *requirement* to provide the data even if you're using the
>>> cache (and the cache support is entirely optional), there's just costs
>>> to not providing it in terms of what features you can get from the
>>> regmap API and the performance of the system.  Not every device is going
>>> to be bothered by those costs, many devices don't provide all of the
>>> data they could.
>>
>> So what do we do in the case where, due to the size of the device, the
>> amount of lines required by these tables go from crazy to grotesque?
>>
> 
> Ultimately, I guess I have always just viewed it as just data
> tables. Its a lot of lines of source but its not complicated,
> and complexity has really always been the thing I try to avoid.
>

Again my question is why does it matter how many lines of source the tables
take? Unlike actual code, reducing the number of lines in a table doesn't
mean its more efficient. The tables aren't even particularly large when
compiled, a few kilobytes for CS47L90, which the users of that codec don't
care about compared to speed.

I can give a size comparison based on the Madera patches. When built as
modules for 32-bit ARM, the pinctrl driver for the (very simple) GPIOs on
those codecs builds to 22kB. All the regmap tables for the CS47L90
(the largest codec), including the defaults and all the readable/volatile
functions, builds to 23kB, and the smaller (but still quite big) CS47L35
to 16kB. So we're talking <= a trivial pinctrl driver even for a big
complex device like a CS47L90 with >1350 registers. For another comparison
the bulk of the CS47L90 object is the audio driver, which is ~2.2MB. So
the regmap tables are < 0.1%.

>>>>>> Even if it is absolutely critical that you have to supply these to
>>>>>> Regmap up-front, instead of on first use/read, why can't you just
>>>>>> supply the oddball non-zero ones?
>>>
>>> That doesn't work, we need to know both if the register has a default
>>> value and what that value is - there's no great value in only supplying
>>> the defaults for registers with non-zero values.
>>
>> All registers have a default value.  Why can't we assume that if a
>> register is writable and a default value was omitted then the default
>> is zero?
> 
> Defaults basically serve two purposes in regmap:
> 
>   1) Initialise the register cache.
> 
>      There are basically three ways you could handle this
>      (at least that I can think of) and regmap supports all
>      three. Obviously each with their own pros and cons:
> 
>       1.1) Table of default values
>            + Fast boot time
>            - Uses some additional memory
>       1.2) Read all the registers from the device on boot
>            + Uses less memory
>            - Potentially very slow boot time
>       1.3) Only read values as you touch registers
>            + Uses less memory
>            + Usually no significant impact on boot time
>            - Can't do read or read/write/modify operations on
>              previously untouched registers whilst chip is off
> 
>      1.3 does probably make sense here for Lochnagar since we
>      don't currently power things down. However, it is worth
>      noting that such an approach is extremely challenging for many
>      devices. For example the CODECs generally have all sorts of
>      actual user-facing interface that needs to be accessible
>      regardless of if the CODEC is powered up or down and powering it
>      up to access registers would end up being either horrific on
>      power consumption and/or horrific in terms of code complexity.
> 
>      1.1 and 1.2 are basically a straight trade off. Generally
>      for our devices we talking loads of registers and
>      potentially connected over I2C. Our customers care deeply
>      about device boot time, Android has specs for such things
>      and often it is tight for a system to make those specs.
>      Conversly, generally the products we integrate into have
>      a fairly large amount of memory. As such this is a no
>      brainer of a choice for most of our devices.
> 
>   2) Determine what registers should be synchronised on a cache
>      sync.
> 
>      A cache sync is usually done when pulling a device out of
>      low power mode to reapply the currently desired register
>      settings. As discussed in 1) we don't currently do cache
>      syncs on Lochnagar, but this would affect most of our
>      parts. Again I can see three approaches to synchronising
>      the cache:
> 
>       2.1) Sync out registers that arn't at their default value
>            + Only syncs register that are actually needed
>            - Requires a table of defaults to work
>       2.2) Store a per register dirty flag in the cache
>            + No up front table required in the driver
>            + Potentially less memory as only a single bit required
>              per register, although realising that saving might be
>              very hard on some cache types
>            - May sync registers that arn't required
>       2.3) Sync all registers from the cache
>            + No memory usage
>            - Potentially large penalty for cache sync
> 
>      Regmap has options to do 2.3, however for most of our
>      devices that would be totally unacceptable. Our parts have
>      a lot of registers most of which are cacheable and for
>      power reasons cache syncs happen as the audio path is being
>      brought up. Again Android has targets here and they are
>      in low 10's of millseconds so bus accesses really do matter.
> 
>      2.1 is the normal proceedure in regmap, although this
>      is defined on a per cache implementation basis, with the
>      core providing 2.1 as a default.
> 
>      Again it's a bit of a trade off between 2.1 and 2.2, but
>      given 1 pretty much requires us to know the defaults
>      anyway, 2.1 will in general make the most sense, at least
>      for Cirrus parts.
> 
> So I would conclude, certainly for most Cirrus devices, we
> do need to know the defaults at least for the vast majority
> of registers. I guess the next question would be could we
> generate some of the defaults table to reduce the table size
> in the driver. I would agree with you that the only sensible
> approach to reducing the defaults table size I can see would be
> to not have to specify defaults for registers with a default
> of zero. As an example to set expectations cs47l90, probably
> the largest CODEC we have made, has 1357 defaults of which
> 693 are zero so ~50%.
> 
> The issue really boils down to how do we tell the difference
> between a register that has no default, and one that has a default
> of zero? There are a few problems I can foresee on this front:
> 
>   1) There are occasions where people use a readable,
>      non-volatile register with no default for things like
>      device ID or revision. The idea being it can be read once
>      which will populate it into the cache then it never needs
>      to be read from the hardware again. Especially useful if
>      this information might be required whilst the hardware
>      is powered down.
> 
>      We could potentially update such drivers to mark the
>      register as volatile and then store the value explicitly
>      in the drivers data structures?
> 
>   2) There are occasions where a readable, writeable,
>      non-volatile register cannot be given a fixed default.
>      Usually this will be registers that are configured by
>      firmware or hardware during boot but then have control
>      passed to the kernel.
> 
>      For example a couple of registers on Lochnagar will be
>      configured by the board controller itself depending on
>      the CODEC that is attached, mostly regulators that are
>      enabled during boot being set to appropriate voltages to
>      avoid hardware damage. To handle these we don't give them
>      defaults which forces a read from the device but then after
>      that we can use the cache.
> 
>      For this one would probably have to add a new register
>      property (in addition to the existing readable, writeable,
>      volatile, and precious) for no default. Which would require
>      an additional callback. Although I guess that would cover 1
>      as well, and normally there would be very few registers in
>      this catagory.
> 
>   3) Rather nervous there are other issues I am not currently
>      thinking of this is a widely used API and I am mostly
>      familiar only with the style of devices I work with.
> 
>      We could potentially add an assume zero defaults flag,
>      that would at least then only apply the change to devices
>      that opt in?
> 
> One other related thing that rests on my mind is that creating
> the defaults table is going to be a little intensive. I guess
> it is not so bad if using the ranges API, so perhaps we would
> need to tie it in with that. Although it is still a little
> fiddly as you need to work out overlaps between the ranges for
> different properties to be more efficient than just checking
> each register.  For the callback based systems though you
> would have to check each register and for example coming
> back to cs47l90, the highest register address is 0x3FFE7C
> which means you would need to call over 4 million callbacks
> probably multiple times whilst constructing your defaults table.
>

Part of the point of the cache is that you can read and write non-volatile
register values without powering up the hardware. For that to work for
registers with zero defaults you have 3 options:

1) Add extra conditionals into the regmap cache code so that if a register
is accessed and a default hasn't been declared, check (using some other
configuration table or function) whether this is one that must be populated
from the hardware or can be assumed to be zero.
   + would work
   - it's more overhead in the cache code paths and the caching code is
     already adding code path overhead. It's not safe to assume "well we
     only need to do this once so it's ok", what if a time-critical block
     of code accesses a large sequence of registers that all invoke this
     extra code path?

2) Power-up the hardware during boot and touch all the registers with
zero values.
   + ensures the cache is pre-populated with all defaults so no deferred
     overhead that could happen at an inconvenient time as (1) would.
   - adds boot time overhead.
   - if the registers are sparse (as on Cirrus codecs) this can't be a
     simple loop, it would need a table and we were trying to avoid
     tabulating registers that have a zero default

3) Regmap initializes to zero all cache entries where the register default
wasn't given and the register is not in the list "non-volatile registers that
must be populated from the real hardware value"
   + as (2) it avoids the potential of a large deferred cache initialization
     at an inconvenient time.
   - a missing address in the defaults table could be
        i.   a register with a zero default,
        ii.  a register that must be initialized from the hardware,
        iii. an unused address.
     The only way regmap could decide would be to check the readable/writable
     definitions and a new table of "non-volatile registers that must be
     populated from the real hardware value" to decide whether the address
     is a register or not. For a very large sparse register map there could
     be a huge number of addresses that have to be checked. We can get some
     idea how long this might take, regmap's debugfs builds a table this way
     and for the largest Cirrus codec (not upstream yet) we've seen this take
     up to 2 minutes (on an ARM CPU). While that's a maximum it gives some
     idea - even if we added only 10 seconds to boot time that's 10 seconds
     to long for most users of our codecs.

4) Regmap pre-populates the entire cache with zeros and then fills in any
non-zero defaults from the provided defaults table.
    + simpler to implement in regmap than (3)
    - more overhead than (3), you have to fill the whole cache so you're
      invoking the full 2 minute overhead that I mentioned in (3).

In practice (1) is feasible and could be acceptable for some devices
(but those are quite likely also devices where the current defaults table
isn't large anyway). It would have to be an option flag because it could
break the behaviour of existing regmap clients. But the potential for a
deferred overhead popping up at bad times might not always be acceptable.
One particular bad place is syncing the cache around a suspend/resume to
know the default values to eliminate useless writes so effectively (1) is
turning into (3). As a suspend/resume can be subject to tight OS limits
on time to setup a use-case it's not something we want to be bloating,
even if it's only on the first invocation.

In fact if there was anything we really wanted to contribute to regmap it
would be things that makes it faster. There don't seem to be any options
for shortening the data tables that don't also make regmap slower in some
way. I suppose if someone could make regmap faster we'd then have some
extra leeway to put in things that add overhead but then it's a shame to
have gained some speed and then take it away.

<snip>

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