Re: [PATCH 2/2] clk: add 74aup1g157gw 2-input multiplexer as clock driver
From: Stephen Boyd <sboyd@kernel.org>
Date: 2018-10-11 06:40:02
Also in:
linux-clk, lkml
Quoting Kuninori Morimoto (2018-10-09 19:16:48)
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
endmenudiff --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.
quoted hunk ↗ jump to hunk
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.