Re: [PATCH v3 4/5] clk: clk-gpio: add driver for gated-fixed-clocks

Stephen Boyd
2024-08-28T18:30:54Z

Quoting Heiko Stuebner (2024-08-28 03:15:02)
> diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
> index cda362a2eca0..8bcdef340b4c 100644
> --- a/drivers/clk/clk-gpio.c
> +++ b/drivers/clk/clk-gpio.c
> @@ -239,3 +240,184 @@ static struct platform_driver gpio_clk_driver = {
> },
> };
> builtin_platform_driver(gpio_clk_driver);
> +
> +/**
> + * DOC: gated fixed clock, controlled with a gpio output and a regulator
> + * Traits of this clock:
> + * prepare - clk_prepare and clk_unprepare are function & control regulator
> + * optionally a gpio that can sleep
> + * enable - clk_enable and clk_disable are functional & control gpio
> + * rate - rate is fixed and set on clock generation

Maybe 'clock registration'

> + * parent - fixed clock is a root clock and has no parent.

Not sure why this one gets the period while other lines above don't.

> + */
> +
> +/**
> + * struct clk_gate_fixed - gated-fixed-clock
> + *
> + * clk_gpio: instance of clk_gpio for gate-gpio
> + * supply: supply regulator
> + * rate: fixed rate
> + */
> +struct clk_gated_fixed {
> + struct clk_gpio clk_gpio;
> + struct regulator *supply;
> + u32 rate;

unsigned long rate to match the CCF type please.

> +};
> +
> +#define to_clk_gated_fixed(_clk_gpio) container_of(_clk_gpio, struct clk_gated_fixed, clk_gpio)
> +
> +static unsigned long clk_gated_fixed_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + return to_clk_gated_fixed(to_clk_gpio(hw))->rate;
> +}
> +
> +static int clk_gated_fixed_prepare(struct clk_hw *hw)
> +{
> + struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw));
> +
> + if (!clk->supply)
> + return 0;
> +
> + return regulator_enable(clk->supply);
> +}
> +
> +static void clk_gated_fixed_unprepare(struct clk_hw *hw)
> +{
> + struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw));
> +
> + if (!clk->supply)
> + return;
> +
> + regulator_disable(clk->supply);
> +}
> +
> +static int clk_gated_fixed_is_prepared(struct clk_hw *hw)
> +{
> + struct clk_gated_fixed *clk = to_clk_gated_fixed(to_clk_gpio(hw));
> +
> + if (!clk->supply)
> + return true;
> +
> + return regulator_is_enabled(clk->supply);
> +}
> +
> +/*
> + * Fixed gated clock with non-sleeping gpio.
> + *
> + * Prepare operation turns on the supply regulator
> + * and the enable operation switches the enable-gpio.
> + */
> +const struct clk_ops clk_gated_fixed_ops = {

static

> + .prepare = clk_gated_fixed_prepare,
> + .unprepare = clk_gated_fixed_unprepare,
> + .is_prepared = clk_gated_fixed_is_prepared,
> + .enable = clk_gpio_gate_enable,
> + .disable = clk_gpio_gate_disable,
> + .is_enabled = clk_gpio_gate_is_enabled,
> + .recalc_rate = clk_gated_fixed_recalc_rate,
> +};
> +
> +static int clk_sleeping_gated_fixed_prepare(struct clk_hw *hw)
> +{
> + int ret;
> +
> + ret = clk_gated_fixed_prepare(hw);
> + if (ret)
> + return ret;
> +
> + ret = clk_sleeping_gpio_gate_prepare(hw);
> + if (ret)
> + clk_gated_fixed_unprepare(hw);
> +
> + return ret;
> +}
> +
> +static void clk_sleeping_gated_fixed_unprepare(struct clk_hw *hw)
> +{
> + clk_gated_fixed_unprepare(hw);
> + clk_sleeping_gpio_gate_unprepare(hw);
> +}
> +
> +/*
> + * Fixed gated clock with non-sleeping gpio.
> + *
> + * Enabling the supply regulator and switching the enable-gpio happens
> + * both in the prepare step.
> + * is_prepared only needs to check the gpio state, as toggling the
> + * gpio is the last step when preparing.
> + */
> +const struct clk_ops clk_sleeping_gated_fixed_ops = {

static

> + .prepare = clk_sleeping_gated_fixed_prepare,
> + .unprepare = clk_sleeping_gated_fixed_unprepare,
> + .is_prepared = clk_sleeping_gpio_gate_is_prepared,
> + .recalc_rate = clk_gated_fixed_recalc_rate,
> +};
> +
> +static int clk_gated_fixed_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct clk_gated_fixed *clk;
> + const struct clk_ops *ops;
> + const char *clk_name;
> + int ret;
> +
> + clk = devm_kzalloc(dev, sizeof(*clk), GFP_KERNEL);
> + if (!clk)
> + return -ENOMEM;
> +
> + if (device_property_read_u32(dev, "clock-frequency", &clk->rate))

Why not return the error code?

> + return dev_err_probe(dev, -EIO, "failed to get clock-frequency");

Missing newline on printk.

> +
> + ret = device_property_read_string(dev, "clock-output-names", &clk_name);
> + if (ret)
> + clk_name = fwnode_get_name(dev->fwnode);
> +
> + clk->supply = devm_regulator_get_optional(dev, "vdd");
> + if (IS_ERR(clk->supply)) {
> + if (PTR_ERR(clk->supply) != -ENODEV)
> + return dev_err_probe(dev, PTR_ERR(clk->supply),
> + "failed to get regulator\n");
> + clk->supply = NULL;
> + }
> +
> + clk->clk_gpio.gpiod = devm_gpiod_get_optional(dev, "enable",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(clk->clk_gpio.gpiod))
> + return dev_err_probe(dev, PTR_ERR(clk->clk_gpio.gpiod),
> + "failed to get gpio\n");
> +
> + if (gpiod_cansleep(clk->clk_gpio.gpiod))
> + ops = &clk_sleeping_gated_fixed_ops;
> + else
> + ops = &clk_gated_fixed_ops;
> +
> + clk->clk_gpio.hw.init = CLK_HW_INIT_NO_PARENT(clk_name, ops, 0);
> +
> + /* register the clock */
> + ret = devm_clk_hw_register(dev, &clk->clk_gpio.hw);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to register clock\n");
> +
> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> + &clk->clk_gpio.hw);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to register clock provider\n");
> +
> + return 0;
> +}
> +
> +static const struct of_device_id gated_fixed_clk_match_table[] = {
> + { .compatible = "gated-fixed-clock" },

Add a sentinel.

> +};
> +
> +static struct platform_driver gated_fixed_clk_driver = {
> + .probe = clk_gated_fixed_probe,
> + .driver = {
> + .name = "gated-fixed-clk",
> + .of_match_table = gated_fixed_clk_match_table,
> + },
> +};
> +builtin_platform_driver(gated_fixed_clk_driver);

The comment above builtin_platform_driver says "Each driver may only use
this macro once". Seems that we need to expand the macro.