[PATCH 0/2] add 74aup1g157gw 2-input multiplexer as clock driver

[PATCH 0/2] add 74aup1g157gw 2-input multiplexer as clock driver

[Kuninori Morimoto]

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

[PATCH 1/2] dt-bindings: clock: add description of 74aup1g157gw

[Kuninori Morimoto]

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

Re: [PATCH 1/2] dt-bindings: clock: add description of 74aup1g157gw

[Rob Herring]

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
> 

Re: [PATCH 1/2] dt-bindings: clock: add description of 74aup1g157gw

[Kuninori Morimoto]

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.

[PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver

[Kuninori Morimoto]

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

Re: [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver

[Stephen Boyd]

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.

Re: [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver

[Kuninori Morimoto]

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

Re: [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver

[Stephen Boyd]

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?

Re: [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver

[Kuninori Morimoto]

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