| Message ID | 20181008132542.19775-1-ckeepax@opensource.cirrus.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v2,1/5] mfd: lochnagar: Add initial binding documentation | expand |
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");
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
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.
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.
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.
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.
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 >
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
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!
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.
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.
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?
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
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 >
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.
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
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.
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
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
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. >
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 >
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.
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.
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.
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.
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>
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.
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. >
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.
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.
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
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>
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.
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.
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>
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
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