Message-ID: <9b92b5f03632e8793253ba75fc00f6e3.sboyd@kernel.org>
Date: Wed, 28 Aug 2024 11:30:51 -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, linux-clk@...r.kernel.org, heiko@...ech.de, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v3 4/5] clk: clk-gpio: add driver for gated-fixed-clocks

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.

Powered by blists - more mailing lists