Message-ID: <84b55412bc61cdcbbc4e051c88827c00.sboyd@kernel.org>
Date: Fri, 26 Jul 2024 15:39:11 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Heiko Stuebner <heiko@...ech.de>, mturquette@...libre.com
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, heiko@...ech.de, linux-clk@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v2 2/3] clk: add driver for voltage controlled oscillators

Quoting Heiko Stuebner (2024-07-15 04:02:50)
> 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.

>  obj-$(CONFIG_COMMON_CLK_WM831X)                += clk-wm831x.o
>  obj-$(CONFIG_COMMON_CLK_XGENE)         += clk-xgene.o
>  
> diff --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@...ech.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");
> +

Powered by blists - more mailing lists