Re: [PATCH v2 2/3] clk: add driver for voltage controlled oscillators
From: Stephen Boyd <sboyd@kernel.org>
Date: 2024-07-26 22:39:13
Also in:
linux-clk, linux-devicetree, linux-rockchip, lkml
Quoting Heiko Stuebner (2024-07-15 04:02:50)
quoted hunk ↗ jump to hunk
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 4abe16c8ccdfe..ca7b7b7ddfd8d 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile@@ -79,6 +79,7 @@ obj-$(CONFIG_COMMON_CLK_SI521XX) += clk-si521xx.o obj-$(CONFIG_COMMON_CLK_VC3) += clk-versaclock3.o obj-$(CONFIG_COMMON_CLK_VC5) += clk-versaclock5.o obj-$(CONFIG_COMMON_CLK_VC7) += clk-versaclock7.o +obj-$(CONFIG_COMMON_CLK_VCO) += clk-vco.o
Wrong section. It's basically a common clk type.
quoted hunk ↗ jump to hunk
obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o obj-$(CONFIG_COMMON_CLK_XGENE) += clk-xgene.odiff --git a/drivers/clk/clk-vco.c b/drivers/clk/clk-vco.c new file mode 100644 index 0000000000000..f7fe2bc627f36 --- /dev/null +++ b/drivers/clk/clk-vco.c@@ -0,0 +1,133 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de> + * + * Generic voltage controlled oscillator + */ + +#include <linux/clk-provider.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> + +struct clk_vco { + struct device *dev; + struct clk_hw hw; + u32 rate; + struct regulator *supply; + struct gpio_desc *enable_gpio; +}; + +#define to_clk_vco(_hw) container_of(_hw, struct clk_vco, hw) + +static int clk_vco_prepare(struct clk_hw *hw) +{ + return regulator_enable(to_clk_vco(hw)->supply); +} + +static void clk_vco_unprepare(struct clk_hw *hw) +{ + regulator_disable(to_clk_vco(hw)->supply); +} + +static int clk_vco_enable(struct clk_hw *hw) +{ + gpiod_set_value(to_clk_vco(hw)->enable_gpio, 1); + return 0; +} + +static void clk_vco_disable(struct clk_hw *hw) +{ + gpiod_set_value(to_clk_vco(hw)->enable_gpio, 0); +}
It looks similar to clk-gpio.c code, but not as complete because it assumes gpios can't sleep. Please look into reusing that code somehow, possibly exporting 'clk_gpio_gate_ops' and struct clk_gpio for use in this new driver. It would be good to fold the sleepable gpio bit as well somehow, maybe with a new function to get a device's gpiod along with returning a const pointer to the clk_ops that can be copied and amended with the regulator part.
+
+static unsigned long clk_vco_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ return to_clk_vco(hw)->rate;
+}
+
+const struct clk_ops clk_vco_ops = {
+ .prepare = clk_vco_prepare,
+ .unprepare = clk_vco_unprepare,
+ .enable = clk_vco_enable,
+ .disable = clk_vco_disable,
+ .recalc_rate = clk_vco_recalc_rate,
+};
+
+static int clk_vco_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct clk_vco *clkgen;
+ const char *clk_name;
+ int ret;
+
+ clkgen = devm_kzalloc(dev, sizeof(*clkgen), GFP_KERNEL);
+ if (!clkgen)
+ return -ENOMEM;
+
+ clkgen->dev = dev;Is this used outside of probe? Why stash it?
+
+ if (device_property_read_u32(dev, "clock-frequency", &clkgen->rate))
+ return dev_err_probe(dev, -EIO, "failed to get clock-frequency");
+
+ ret = device_property_read_string(dev, "clock-output-names", &clk_name);
+ if (ret)
+ clk_name = fwnode_get_name(dev->fwnode);
+
+ clkgen->supply = devm_regulator_get_optional(dev, "vdd");
+ if (IS_ERR(clkgen->supply)) {
+ if (PTR_ERR(clkgen->supply) != -ENODEV)
+ return dev_err_probe(dev, PTR_ERR(clkgen->supply),
+ "failed to get regulator\n");
+ clkgen->supply = NULL;
+ }
+
+ clkgen->enable_gpio = devm_gpiod_get_optional(dev, "enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(clkgen->enable_gpio))
+ return dev_err_probe(dev, PTR_ERR(clkgen->enable_gpio),
+ "failed to get gpio\n");
+
+ ret = gpiod_direction_output(clkgen->enable_gpio, 0);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "failed to set gpio output");Missing newline.
+ + clkgen->hw.init = CLK_HW_INIT_NO_PARENT(clk_name, &clk_vco_ops, 0); + + /* register the clock */ + ret = devm_clk_hw_register(dev, &clkgen->hw); + if (ret) + return dev_err_probe(dev, ret, + "failed to register clock\n"); +