| Message ID | 20181008132542.19775-3-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Changes Requested, 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 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.
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 292056bbb30e9..9247c97f85d79 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -219,6 +219,13 @@ config COMMON_CLK_XGENE ---help--- Sypport for the APM X-Gene SoC reference, PLL, and device clocks. +config COMMON_CLK_LOCHNAGAR + tristate "Cirrus Logic Lochnagar clock driver" + depends on MFD_LOCHNAGAR + help + This driver supports the clocking features of the Cirrus Logic + Lochnagar audio development board. + config COMMON_CLK_NXP def_bool COMMON_CLK && (ARCH_LPC18XX || ARCH_LPC32XX) select REGMAP_MMIO if ARCH_LPC32XX diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index a84c5573cabea..0443a7219bdf4 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_COMMON_CLK_GEMINI) += clk-gemini.o obj-$(CONFIG_COMMON_CLK_ASPEED) += clk-aspeed.o obj-$(CONFIG_ARCH_HIGHBANK) += clk-highbank.o obj-$(CONFIG_CLK_HSDK) += clk-hsdk-pll.o +obj-$(CONFIG_COMMON_CLK_LOCHNAGAR) += clk-lochnagar.o obj-$(CONFIG_COMMON_CLK_MAX77686) += clk-max77686.o obj-$(CONFIG_COMMON_CLK_MAX9485) += clk-max9485.o obj-$(CONFIG_ARCH_MOXART) += clk-moxart.o diff --git a/drivers/clk/clk-lochnagar.c b/drivers/clk/clk-lochnagar.c new file mode 100644 index 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> +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#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; +}; + +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; + + 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, }, \ + } + +#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), +}; + +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); + + if (!lclk->regmap.ena_mask) + 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", + 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; + } + + 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); + 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, + 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); + + for (i = 0; i < ARRAY_SIZE(priv->lclks); i++) { + switch (priv->lclks[i].type) { + case LOCHNAGAR_CLK_TYPE_FIXED: + clk_unregister_fixed_rate(priv->clks[i]); + break; + default: + break; + } + } + + return 0; +} + +static struct platform_driver lochnagar_clk_driver = { + .driver = { + .name = "lochnagar-clk", + }, + + .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");