| Message ID | 87bm821rkf.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | add 74aup1g157gw 2-input multiplexer as clock driver | expand |
Quoting Kuninori Morimoto (2018-10-09 19:16:48) > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 292056b..9cfeb0e 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -299,5 +299,6 @@ source "drivers/clk/sunxi-ng/Kconfig" > source "drivers/clk/tegra/Kconfig" > source "drivers/clk/ti/Kconfig" > source "drivers/clk/uniphier/Kconfig" > +source "drivers/clk/nxp/Kconfig" Please sort this alphabetically. > > endmenu > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index ed344eb..fcfd42a 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -84,7 +84,7 @@ obj-$(CONFIG_ARCH_MMP) += mmp/ > endif > obj-y += mvebu/ > obj-$(CONFIG_ARCH_MXS) += mxs/ > -obj-$(CONFIG_COMMON_CLK_NXP) += nxp/ > +obj-y += nxp/ There's the CONFIG_COMMON_CLK_NXP config symbol that could go into the drivers/clk/nxp/Kconfig file then and also wrap the Makefile contents in the nxp subdirectory too? We don't want to lose that config option and start enabling this driver all the time. > obj-$(CONFIG_MACH_PISTACHIO) += pistachio/ > obj-$(CONFIG_COMMON_CLK_PXA) += pxa/ > obj-$(CONFIG_COMMON_CLK_QCOM) += qcom/ > diff --git a/drivers/clk/nxp/clk-74aup1g157gw.c b/drivers/clk/nxp/clk-74aup1g157gw.c > new file mode 100644 > index 0000000..78199bb > --- /dev/null > +++ b/drivers/clk/nxp/clk-74aup1g157gw.c > @@ -0,0 +1,213 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// Copyright (C) 2018 Renesas Solutions Corp. > +// Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > +// > +#include <linux/module.h> > +#include <linux/clk.h> > +#include <linux/clk-provider.h> > +#include <linux/slab.h> > +#include <linux/err.h> > +#include <linux/gpio/consumer.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > + > +#define CLK_I_NUM 2 > +struct clk_priv { > + struct clk_hw hw; > + struct device *dev; > + struct clk *i[CLK_I_NUM]; > + struct gpio_desc *sel; > + long (*round_rate)(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate); > +}; > +#define hw_to_priv(_hw) container_of(_hw, struct clk_priv, hw) > +#define priv_to_dev(priv) priv->dev Nitpick: This macro is just obfuscating, there isn't any container_of() usage going on so please remove it. > +#define for_each_iclk(i) \ > + for ((i) = 0; (i) < CLK_I_NUM; (i)++) Nitpick: This macros isn't making things any shorter, so please remove it. > + > +static int clk74_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct clk_priv *priv = hw_to_priv(hw); > + struct device *dev = priv_to_dev(priv); > + int i; > + > + for_each_iclk(i) { > + if (rate == clk_get_rate(priv->i[i])) { > + dev_dbg(dev, "set rate %lu as i%d\n", rate, i); > + gpiod_set_value_cansleep(priv->sel, i); > + return 0; > + } > + } > + > + dev_err(dev, "unsupported rate %lu\n", rate); > + return -EIO; > +} > + > +static long clk74_round_rate_close(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + struct clk_priv *priv = hw_to_priv(hw); > + struct device *dev = priv_to_dev(priv); > + unsigned long min = ~0; > + unsigned long ret = 0; > + int i; > + > + /* > + * select closest rate > + */ > + for_each_iclk(i) { > + unsigned long irate = clk_get_rate(priv->i[i]); > + unsigned long diff = abs(rate - irate); > + > + if (min > diff) { > + min = diff; > + ret = irate; > + } > + } > + > + dev_dbg(dev, "(close)round rate %lu\n", ret); > + > + return ret; > +} > + > +static long clk74_round_rate_audio(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + struct clk_priv *priv = hw_to_priv(hw); > + struct device *dev = priv_to_dev(priv); > + unsigned long ret = 0; > + int is_8k = 0; > + int i; > + > + /* > + * select 48kHz or 44.1kHz category rate > + */ > + if (!(rate % 8000)) > + is_8k = 1; > + > + for_each_iclk(i) { > + unsigned long irate = clk_get_rate(priv->i[i]); > + > + if (( is_8k && !(irate % 8000)) || > + (!is_8k && (irate % 8000))) { > + ret = irate; > + } > + } > + > + dev_dbg(dev, "(audio)round rate %lu\n", ret); > + > + return ret; > +} > + > +static long clk74_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + struct clk_priv *priv = hw_to_priv(hw); > + > + return priv->round_rate(hw, rate, parent_rate); > +} > + > +static unsigned long clk74_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct clk_priv *priv = hw_to_priv(hw); > + struct device *dev = priv_to_dev(priv); > + unsigned long rate; > + int i = gpiod_get_raw_value_cansleep(priv->sel); > + > + rate = clk_get_rate(priv->i[i]); > + > + dev_dbg(dev, "recalc rate %lu as i%d\n", rate, i); > + > + return rate; > +} > + > +static u8 clk74_get_parent(struct clk_hw *hw) > +{ > + struct clk_priv *priv = hw_to_priv(hw); > + > + return gpiod_get_raw_value_cansleep(priv->sel); > +} > + > +static const struct clk_ops clk74_ops = { > + .set_rate = clk74_set_rate, > + .round_rate = clk74_round_rate, > + .recalc_rate = clk74_recalc_rate, > + .get_parent = clk74_get_parent, > +}; Can this all be handled by the 'gpio-mux-clock' compatible/driver? I suppose it may need an update to add the rounding policy that you want via some sort of DT property, but otherwise it would be fine? > + > +static const char * const clk74_in_name[CLK_I_NUM] = { > + "i0", > + "i1", > +}; > + > +static int clk74_probe(struct platform_device *pdev) > +{ > + struct clk *clk; > + struct clk_init_data init; > + struct clk_priv *priv; > + struct device *dev = &pdev->dev; > + const char *parent_names[CLK_I_NUM]; > + int i, ret; > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENODEV; You mean -ENOMEM? > + > + for_each_iclk(i) { > + clk = devm_clk_get(dev, clk74_in_name[i]); > + if (IS_ERR(clk)) > + return -EPROBE_DEFER; Please return actual error code instead of overriding and returning probe defer. > + priv->i[i] = clk; > + parent_names[i] = __clk_get_name(clk); > + } > + > + memset(&init, 0, sizeof(init)); > + init.name = "74aup1g157gw"; > + init.ops = &clk74_ops; > + init.parent_names = parent_names; > + init.num_parents = CLK_I_NUM; > + > + priv->hw.init = &init; > + priv->dev = dev; > + priv->round_rate = of_device_get_match_data(dev); > + priv->sel = devm_gpiod_get(dev, "sel", 0); > + if (IS_ERR(priv->sel)) > + return -EPROBE_DEFER; Please return the actual error. > + > + gpiod_direction_output(priv->sel, 0); > + > + ret = devm_clk_hw_register(dev, &priv->hw); > + if (ret < 0) > + return ret; > + > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &priv->hw); > + if (ret < 0) > + return ret; > + > + platform_set_drvdata(pdev, priv); > + > + dev_info(dev, "probed\n"); Please remove noise like this. > + > + return 0; > +} > + > +#define OF_ID(name, func) { .compatible = name, .data = func } > +static const struct of_device_id clk74_of_match[] = { > + OF_ID("nxp,74aup1g157gw-clk", clk74_round_rate_close), > + OF_ID("nxp,74aup1g157gw-audio-clk", clk74_round_rate_audio), Nitpick: I'd prefer to not have the macro.
Hi Stephen Thank you for your feedback > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > > index 292056b..9cfeb0e 100644 > > --- a/drivers/clk/Kconfig > > +++ b/drivers/clk/Kconfig > > @@ -299,5 +299,6 @@ source "drivers/clk/sunxi-ng/Kconfig" > > source "drivers/clk/tegra/Kconfig" > > source "drivers/clk/ti/Kconfig" > > source "drivers/clk/uniphier/Kconfig" > > +source "drivers/clk/nxp/Kconfig" > > Please sort this alphabetically. Will do > > obj-$(CONFIG_ARCH_MXS) += mxs/ > > -obj-$(CONFIG_COMMON_CLK_NXP) += nxp/ > > +obj-y += nxp/ > > There's the CONFIG_COMMON_CLK_NXP config symbol that could go into the > drivers/clk/nxp/Kconfig file then and also wrap the Makefile contents in > the nxp subdirectory too? We don't want to lose that config option and > start enabling this driver all the time. OK, will do > Nitpick: This macro is just obfuscating, there isn't any container_of() > usage going on so please remove it. (snip) > Nitpick: This macros isn't making things any shorter, so please remove > it. will fix > > +static const struct clk_ops clk74_ops = { > > + .set_rate = clk74_set_rate, > > + .round_rate = clk74_round_rate, > > + .recalc_rate = clk74_recalc_rate, > > + .get_parent = clk74_get_parent, > > +}; > > Can this all be handled by the 'gpio-mux-clock' compatible/driver? I > suppose it may need an update to add the rounding policy that you want > via some sort of DT property, but otherwise it would be fine? Hmm.. not sure. If we can add new feature (= .round_rate ?) on gpio-mux-clock, I can consider it. > You mean -ENOMEM? (snip) > Please return actual error code instead of overriding and returning > probe defer. (snip) > Please return the actual error. (snip) > Please remove noise like this. (snip) > Nitpick: I'd prefer to not have the macro. will fix Best regards --- Kuninori Morimoto
Quoting Kuninori Morimoto (2018-10-11 17:37:30) > > > + .recalc_rate = clk74_recalc_rate, > > > + .get_parent = clk74_get_parent, > > > +}; > > > > Can this all be handled by the 'gpio-mux-clock' compatible/driver? I > > suppose it may need an update to add the rounding policy that you want > > via some sort of DT property, but otherwise it would be fine? > > Hmm.. not sure. > If we can add new feature (= .round_rate ?) on gpio-mux-clock, > I can consider it. Yes that would be the idea. Extend gpio-mux-clock driver to have what you want with rounding. I'm not really sure why there is a rounding policy needed though. Is it a static configuration at boot, or does the code using this gpio clk need to search the parent rate space somehow and mux it over?
Hi Stephen > > > > + .recalc_rate = clk74_recalc_rate, > > > > + .get_parent = clk74_get_parent, > > > > +}; > > > > > > Can this all be handled by the 'gpio-mux-clock' compatible/driver? I > > > suppose it may need an update to add the rounding policy that you want > > > via some sort of DT property, but otherwise it would be fine? > > > > Hmm.. not sure. > > If we can add new feature (= .round_rate ?) on gpio-mux-clock, > > I can consider it. > > Yes that would be the idea. Extend gpio-mux-clock driver to have what > you want with rounding. I'm not really sure why there is a rounding > policy needed though. Is it a static configuration at boot, or does the > code using this gpio clk need to search the parent rate space somehow > and mux it over? Thank you for pointing gpio-mux-clock. I tried to use it, and it works well instead of new driver. Actually, it is not 100%, but it should be solved sound side, not clock side, I think. But anyway, this driver is no longer needed, please drop it. Thank you very much
diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 292056b..9cfeb0e 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -299,5 +299,6 @@ source "drivers/clk/sunxi-ng/Kconfig" source "drivers/clk/tegra/Kconfig" source "drivers/clk/ti/Kconfig" source "drivers/clk/uniphier/Kconfig" +source "drivers/clk/nxp/Kconfig" endmenu diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index ed344eb..fcfd42a 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -84,7 +84,7 @@ obj-$(CONFIG_ARCH_MMP) += mmp/ endif obj-y += mvebu/ obj-$(CONFIG_ARCH_MXS) += mxs/ -obj-$(CONFIG_COMMON_CLK_NXP) += nxp/ +obj-y += nxp/ obj-$(CONFIG_MACH_PISTACHIO) += pistachio/ obj-$(CONFIG_COMMON_CLK_PXA) += pxa/ obj-$(CONFIG_COMMON_CLK_QCOM) += qcom/ diff --git a/drivers/clk/nxp/Kconfig b/drivers/clk/nxp/Kconfig new file mode 100644 index 0000000..44c8fed --- /dev/null +++ b/drivers/clk/nxp/Kconfig @@ -0,0 +1,4 @@ +config CLK_74AUP1G157GW + tristate "NXP 74AUP1G157GW Low-power 2-input multiplexer support as clock" + help + This enables NXP 74AUP1G157GW 2-input multiplexer as clock driver diff --git a/drivers/clk/nxp/Makefile b/drivers/clk/nxp/Makefile index d456ee6..2965bdd 100644 --- a/drivers/clk/nxp/Makefile +++ b/drivers/clk/nxp/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_ARCH_LPC18XX) += clk-lpc18xx-cgu.o obj-$(CONFIG_ARCH_LPC18XX) += clk-lpc18xx-ccu.o obj-$(CONFIG_ARCH_LPC18XX) += clk-lpc18xx-creg.o obj-$(CONFIG_ARCH_LPC32XX) += clk-lpc32xx.o +obj-$(CONFIG_CLK_74AUP1G157GW) += clk-74aup1g157gw.o diff --git a/drivers/clk/nxp/clk-74aup1g157gw.c b/drivers/clk/nxp/clk-74aup1g157gw.c new file mode 100644 index 0000000..78199bb --- /dev/null +++ b/drivers/clk/nxp/clk-74aup1g157gw.c @@ -0,0 +1,213 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// Copyright (C) 2018 Renesas Solutions Corp. +// Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> +// +#include <linux/module.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/slab.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> + +#define CLK_I_NUM 2 +struct clk_priv { + struct clk_hw hw; + struct device *dev; + struct clk *i[CLK_I_NUM]; + struct gpio_desc *sel; + long (*round_rate)(struct clk_hw *hw, unsigned long rate, + unsigned long *parent_rate); +}; +#define hw_to_priv(_hw) container_of(_hw, struct clk_priv, hw) +#define priv_to_dev(priv) priv->dev +#define for_each_iclk(i) \ + for ((i) = 0; (i) < CLK_I_NUM; (i)++) + +static int clk74_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct clk_priv *priv = hw_to_priv(hw); + struct device *dev = priv_to_dev(priv); + int i; + + for_each_iclk(i) { + if (rate == clk_get_rate(priv->i[i])) { + dev_dbg(dev, "set rate %lu as i%d\n", rate, i); + gpiod_set_value_cansleep(priv->sel, i); + return 0; + } + } + + dev_err(dev, "unsupported rate %lu\n", rate); + return -EIO; +} + +static long clk74_round_rate_close(struct clk_hw *hw, unsigned long rate, + unsigned long *parent_rate) +{ + struct clk_priv *priv = hw_to_priv(hw); + struct device *dev = priv_to_dev(priv); + unsigned long min = ~0; + unsigned long ret = 0; + int i; + + /* + * select closest rate + */ + for_each_iclk(i) { + unsigned long irate = clk_get_rate(priv->i[i]); + unsigned long diff = abs(rate - irate); + + if (min > diff) { + min = diff; + ret = irate; + } + } + + dev_dbg(dev, "(close)round rate %lu\n", ret); + + return ret; +} + +static long clk74_round_rate_audio(struct clk_hw *hw, unsigned long rate, + unsigned long *parent_rate) +{ + struct clk_priv *priv = hw_to_priv(hw); + struct device *dev = priv_to_dev(priv); + unsigned long ret = 0; + int is_8k = 0; + int i; + + /* + * select 48kHz or 44.1kHz category rate + */ + if (!(rate % 8000)) + is_8k = 1; + + for_each_iclk(i) { + unsigned long irate = clk_get_rate(priv->i[i]); + + if (( is_8k && !(irate % 8000)) || + (!is_8k && (irate % 8000))) { + ret = irate; + } + } + + dev_dbg(dev, "(audio)round rate %lu\n", ret); + + return ret; +} + +static long clk74_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *parent_rate) +{ + struct clk_priv *priv = hw_to_priv(hw); + + return priv->round_rate(hw, rate, parent_rate); +} + +static unsigned long clk74_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct clk_priv *priv = hw_to_priv(hw); + struct device *dev = priv_to_dev(priv); + unsigned long rate; + int i = gpiod_get_raw_value_cansleep(priv->sel); + + rate = clk_get_rate(priv->i[i]); + + dev_dbg(dev, "recalc rate %lu as i%d\n", rate, i); + + return rate; +} + +static u8 clk74_get_parent(struct clk_hw *hw) +{ + struct clk_priv *priv = hw_to_priv(hw); + + return gpiod_get_raw_value_cansleep(priv->sel); +} + +static const struct clk_ops clk74_ops = { + .set_rate = clk74_set_rate, + .round_rate = clk74_round_rate, + .recalc_rate = clk74_recalc_rate, + .get_parent = clk74_get_parent, +}; + +static const char * const clk74_in_name[CLK_I_NUM] = { + "i0", + "i1", +}; + +static int clk74_probe(struct platform_device *pdev) +{ + struct clk *clk; + struct clk_init_data init; + struct clk_priv *priv; + struct device *dev = &pdev->dev; + const char *parent_names[CLK_I_NUM]; + int i, ret; + + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENODEV; + + for_each_iclk(i) { + clk = devm_clk_get(dev, clk74_in_name[i]); + if (IS_ERR(clk)) + return -EPROBE_DEFER; + priv->i[i] = clk; + parent_names[i] = __clk_get_name(clk); + } + + memset(&init, 0, sizeof(init)); + init.name = "74aup1g157gw"; + init.ops = &clk74_ops; + init.parent_names = parent_names; + init.num_parents = CLK_I_NUM; + + priv->hw.init = &init; + priv->dev = dev; + priv->round_rate = of_device_get_match_data(dev); + priv->sel = devm_gpiod_get(dev, "sel", 0); + if (IS_ERR(priv->sel)) + return -EPROBE_DEFER; + + gpiod_direction_output(priv->sel, 0); + + ret = devm_clk_hw_register(dev, &priv->hw); + if (ret < 0) + return ret; + + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &priv->hw); + if (ret < 0) + return ret; + + platform_set_drvdata(pdev, priv); + + dev_info(dev, "probed\n"); + + return 0; +} + +#define OF_ID(name, func) { .compatible = name, .data = func } +static const struct of_device_id clk74_of_match[] = { + OF_ID("nxp,74aup1g157gw-clk", clk74_round_rate_close), + OF_ID("nxp,74aup1g157gw-audio-clk", clk74_round_rate_audio), + { } +}; +MODULE_DEVICE_TABLE(of, clk74_of_match); + +static struct platform_driver clk74_driver = { + .driver = { + .name = "74aup1g157gw", + .of_match_table = clk74_of_match, + }, + .probe = clk74_probe, +}; +builtin_platform_driver(clk74_driver);