* [PATCH 0/2] add 74aup1g157gw 2-input multiplexer as clock driver
@ 2018-10-10 2:15 Kuninori Morimoto
2018-10-10 2:16 ` [PATCH 1/2] dt-bindings: clock: add description of 74aup1g157gw Kuninori Morimoto
2018-10-10 2:16 ` [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver Kuninori Morimoto
0 siblings, 2 replies; 9+ messages in thread
From: Kuninori Morimoto @ 2018-10-10 2:15 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland
Cc: linux-clk, devicetree, linux-kernel
Hi Michael, Stephen, Rob, Mark
These adds 74aup1g157gw 2-input multiplexer as clock driver.
Kuninori Morimoto (2):
dt-bindings: clock: add description of 74aup1g157gw
clk: add 74aup1g157gw 2-input multiplexer as clock driver
Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt | 19 ++++++
drivers/clk/Kconfig | 1 +
drivers/clk/Makefile | 2 +-
drivers/clk/nxp/Kconfig | 4 ++
drivers/clk/nxp/Makefile | 1 +
drivers/clk/nxp/clk-74aup1g157gw.c | 213 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 239 insertions(+), 1 deletion(-)
--
2.7.4
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] dt-bindings: clock: add description of 74aup1g157gw
2018-10-10 2:15 [PATCH 0/2] add 74aup1g157gw 2-input multiplexer as clock driver Kuninori Morimoto
@ 2018-10-10 2:16 ` Kuninori Morimoto
2018-10-17 16:05 ` Rob Herring
2018-10-10 2:16 ` [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver Kuninori Morimoto
1 sibling, 1 reply; 9+ messages in thread
From: Kuninori Morimoto @ 2018-10-10 2:16 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland
Cc: linux-clk, devicetree, linux-kernel
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
74aup1g157gw needs i0 and i1 pin as input, select and output it by
sel gpio pin. This patch adds description for 74aup1g157gw as clock
multiplexer.
"nxp,74aup1g157gw-clk" will select most closest input as output,
"nxp,74aup1g157gw-audio-clk" will select 48kHz/44.1kHz categorized
input as output.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
.../devicetree/bindings/clock/nxp,74aup1g157gw.txt | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt
diff --git a/Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt b/Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt
new file mode 100644
index 0000000..d07bc04
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt
@@ -0,0 +1,19 @@
+NXP 74AUP1G157GW Low-power 2-input multiplexer for clock
+
+Required properties:
+- compatible: "nxp,74aup1g157gw-clk" (select closest input as output)
+ "nxp,74aup1g157gw-audio-clk (select 48kHz / 44.1kHz as output)
+- clocks: select input clock as i0, i1
+- clock-names: "i0" and "i1"
+- sel-gpios: selector pin
+
+Examples:
+
+ clk {
+ #clock-cells = <0>;
+ compatible = "nxp,74aup1g157gw-audio-clk";
+ clocks = <&clk8snd>, <&clksnd>;
+ clock-names = "i0", "i1";
+ sel-gpios = <&gpio_exp_75 13 GPIO_ACTIVE_HIGH>;
+ };
+
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver
2018-10-10 2:15 [PATCH 0/2] add 74aup1g157gw 2-input multiplexer as clock driver Kuninori Morimoto
2018-10-10 2:16 ` [PATCH 1/2] dt-bindings: clock: add description of 74aup1g157gw Kuninori Morimoto
@ 2018-10-10 2:16 ` Kuninori Morimoto
2018-10-11 6:39 ` Stephen Boyd
1 sibling, 1 reply; 9+ messages in thread
From: Kuninori Morimoto @ 2018-10-10 2:16 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd, Rob Herring, Mark Rutland
Cc: linux-clk, devicetree, linux-kernel
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
74aup1g157gw needs i0 and i1 pin as input, select and output it by
sel gpio pin. This driver adds new 74aup1g157gw as clock multiplexer.
"nxp,74aup1g157gw-clk" will select most closest input as output,
"nxp,74aup1g157gw-audio-clk" will select 48kHz/44.1kHz categorized
input as output.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
drivers/clk/Kconfig | 1 +
drivers/clk/Makefile | 2 +-
drivers/clk/nxp/Kconfig | 4 +
drivers/clk/nxp/Makefile | 1 +
drivers/clk/nxp/clk-74aup1g157gw.c | 213 +++++++++++++++++++++++++++++++++++++
5 files changed, 220 insertions(+), 1 deletion(-)
create mode 100644 drivers/clk/nxp/Kconfig
create mode 100644 drivers/clk/nxp/clk-74aup1g157gw.c
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);
--
2.7.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver
2018-10-10 2:16 ` [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver Kuninori Morimoto
@ 2018-10-11 6:39 ` Stephen Boyd
2018-10-12 0:37 ` Kuninori Morimoto
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2018-10-11 6:39 UTC (permalink / raw)
To: Kuninori Morimoto, Mark Rutland, Michael Turquette, Rob Herring
Cc: linux-clk, devicetree, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver
2018-10-11 6:39 ` Stephen Boyd
@ 2018-10-12 0:37 ` Kuninori Morimoto
2018-10-12 16:06 ` Stephen Boyd
0 siblings, 1 reply; 9+ messages in thread
From: Kuninori Morimoto @ 2018-10-12 0:37 UTC (permalink / raw)
To: Stephen Boyd
Cc: Mark Rutland, Michael Turquette, Rob Herring, linux-clk,
devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver
2018-10-12 0:37 ` Kuninori Morimoto
@ 2018-10-12 16:06 ` Stephen Boyd
2018-10-14 23:53 ` Kuninori Morimoto
0 siblings, 1 reply; 9+ messages in thread
From: Stephen Boyd @ 2018-10-12 16:06 UTC (permalink / raw)
To: Kuninori Morimoto
Cc: Mark Rutland, Michael Turquette, Rob Herring, linux-clk,
devicetree, linux-kernel
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver
2018-10-12 16:06 ` Stephen Boyd
@ 2018-10-14 23:53 ` Kuninori Morimoto
0 siblings, 0 replies; 9+ messages in thread
From: Kuninori Morimoto @ 2018-10-14 23:53 UTC (permalink / raw)
To: Stephen Boyd
Cc: Mark Rutland, Michael Turquette, Rob Herring, linux-clk,
devicetree, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clock: add description of 74aup1g157gw
2018-10-10 2:16 ` [PATCH 1/2] dt-bindings: clock: add description of 74aup1g157gw Kuninori Morimoto
@ 2018-10-17 16:05 ` Rob Herring
2018-10-18 0:15 ` Kuninori Morimoto
0 siblings, 1 reply; 9+ messages in thread
From: Rob Herring @ 2018-10-17 16:05 UTC (permalink / raw)
To: Kuninori Morimoto
Cc: Michael Turquette, Stephen Boyd, Mark Rutland, linux-clk,
devicetree, linux-kernel
On Wed, Oct 10, 2018 at 02:16:35AM +0000, Kuninori Morimoto wrote:
>
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> 74aup1g157gw needs i0 and i1 pin as input, select and output it by
> sel gpio pin. This patch adds description for 74aup1g157gw as clock
> multiplexer.
> "nxp,74aup1g157gw-clk" will select most closest input as output,
> "nxp,74aup1g157gw-audio-clk" will select 48kHz/44.1kHz categorized
> input as output.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> .../devicetree/bindings/clock/nxp,74aup1g157gw.txt | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt b/Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt
> new file mode 100644
> index 0000000..d07bc04
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt
> @@ -0,0 +1,19 @@
> +NXP 74AUP1G157GW Low-power 2-input multiplexer for clock
> +
> +Required properties:
> +- compatible: "nxp,74aup1g157gw-clk" (select closest input as output)
> + "nxp,74aup1g157gw-audio-clk (select 48kHz / 44.1kHz as output)
I don't understand why you need the second one.
What's wrong with the existing gpio-mux-clock binding?
> +- clocks: select input clock as i0, i1
> +- clock-names: "i0" and "i1"
> +- sel-gpios: selector pin
> +
> +Examples:
> +
> + clk {
> + #clock-cells = <0>;
> + compatible = "nxp,74aup1g157gw-audio-clk";
> + clocks = <&clk8snd>, <&clksnd>;
> + clock-names = "i0", "i1";
> + sel-gpios = <&gpio_exp_75 13 GPIO_ACTIVE_HIGH>;
> + };
> +
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clock: add description of 74aup1g157gw
2018-10-17 16:05 ` Rob Herring
@ 2018-10-18 0:15 ` Kuninori Morimoto
0 siblings, 0 replies; 9+ messages in thread
From: Kuninori Morimoto @ 2018-10-18 0:15 UTC (permalink / raw)
To: Rob Herring
Cc: Michael Turquette, Stephen Boyd, Mark Rutland, linux-clk,
devicetree, linux-kernel
Hi Rob
Thank you for your feedback
> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >
> > 74aup1g157gw needs i0 and i1 pin as input, select and output it by
> > sel gpio pin. This patch adds description for 74aup1g157gw as clock
> > multiplexer.
> > "nxp,74aup1g157gw-clk" will select most closest input as output,
> > "nxp,74aup1g157gw-audio-clk" will select 48kHz/44.1kHz categorized
> > input as output.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
> > .../devicetree/bindings/clock/nxp,74aup1g157gw.txt | 19 +++++++++++++++++++
> > 1 file changed, 19 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt
> >
> > diff --git a/Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt b/Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt
> > new file mode 100644
> > index 0000000..d07bc04
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/nxp,74aup1g157gw.txt
> > @@ -0,0 +1,19 @@
> > +NXP 74AUP1G157GW Low-power 2-input multiplexer for clock
> > +
> > +Required properties:
> > +- compatible: "nxp,74aup1g157gw-clk" (select closest input as output)
> > + "nxp,74aup1g157gw-audio-clk (select 48kHz / 44.1kHz as output)
>
> I don't understand why you need the second one.
>
>
> What's wrong with the existing gpio-mux-clock binding?
Thanks.
I already noticed that gpio-mux-clock is very OK to me
(and I think I posted it to ML).
Please drop this patch.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-10-18 0:15 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-10 2:15 [PATCH 0/2] add 74aup1g157gw 2-input multiplexer as clock driver Kuninori Morimoto
2018-10-10 2:16 ` [PATCH 1/2] dt-bindings: clock: add description of 74aup1g157gw Kuninori Morimoto
2018-10-17 16:05 ` Rob Herring
2018-10-18 0:15 ` Kuninori Morimoto
2018-10-10 2:16 ` [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver Kuninori Morimoto
2018-10-11 6:39 ` Stephen Boyd
2018-10-12 0:37 ` Kuninori Morimoto
2018-10-12 16:06 ` Stephen Boyd
2018-10-14 23:53 ` Kuninori Morimoto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).