Re: [PATCH 4/5] clk: eyeq5: add OSPI table-based divider clock

From: Stephen Boyd <sboyd@kernel.org>
To: "Conor Dooley" <conor+dt@kernel.org>,
	"Gregory CLEMENT" <gregory.clement@bootlin.com>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Thomas Bogendoerfer" <tsbogend@alpha.franken.de>,
	"Théo Lebrun" <theo.lebrun@bootlin.com>
Cc: "Vladimir Kondratiev" <vladimir.kondratiev@mobileye.com>,
	linux-mips@vger.kernel.org, linux-clk@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Tawfik Bayouk" <tawfik.bayouk@mobileye.com>,
	"Théo Lebrun" <theo.lebrun@bootlin.com>
Subject: Re: [PATCH 4/5] clk: eyeq5: add OSPI table-based divider clock
Date: Tue, 19 Dec 2023 15:16:12 -0800	[thread overview]
Message-ID: <897fe92cfe40a086832e0c85ef5358bc.sboyd@kernel.org> (raw)
In-Reply-To: <20231218-mbly-clk-v1-4-44ce54108f06@bootlin.com>

Quoting Théo Lebrun (2023-12-18 09:14:19)
> The driver supports PLLs on the platform. Add the single divider clock
> of the platform.
> 
> Helpers from include/linux/clk-provider.h could have been used if it was
> not for the use of regmap to access the register.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---

This patch should be squashed with the previous one.

> diff --git a/drivers/clk/clk-eyeq5.c b/drivers/clk/clk-eyeq5.c
> index 74bcb8cec5c1..3382f4d870d7 100644
> --- a/drivers/clk/clk-eyeq5.c
> +++ b/drivers/clk/clk-eyeq5.c
> @@ -77,6 +78,8 @@ static const struct eq5c_pll {
[...]
> +
> +static int eq5c_ospi_div_set_rate(struct clk_hw *hw,
> +                                 unsigned long rate, unsigned long parent_rate)
> +{
> +       struct eq5c_ospi_div *div = clk_hw_to_ospi_priv(hw);
> +       unsigned int val;
> +       int value, ret;
> +
> +       value = divider_get_val(rate, parent_rate, eq5c_ospi_div_table,
> +                               OLB_OSPI_DIV_MASK_WIDTH, 0);
> +       if (value < 0)
> +               return value;
> +
> +       ret = regmap_read(div->olb, OLB_OSPI_REG, &val);
> +       if (ret) {
> +               pr_err("%s: regmap_read failed: %d\n", __func__, ret);
> +               return -ret;

Why negative ret?

> +       }
> +
> +       val &= ~OLB_OSPI_DIV_MASK;
> +       val |= FIELD_PREP(OLB_OSPI_DIV_MASK, value);
> +
> +       ret = regmap_write(div->olb, OLB_OSPI_REG, val);
> +       if (ret) {
> +               pr_err("%s: regmap_write failed: %d\n", __func__, ret);
> +               return -ret;

Why negative ret?

> +       }
> +
> +       return 0;
> +}
> +
> +const struct clk_ops eq5c_ospi_div_ops = {

static?

> +       .recalc_rate = eq5c_ospi_div_recalc_rate,
> +       .round_rate = eq5c_ospi_div_round_rate,
> +       .determine_rate = eq5c_ospi_div_determine_rate,
> +       .set_rate = eq5c_ospi_div_set_rate,
> +};
> +
> +static struct clk_hw *eq5c_init_ospi_div(const struct clk_hw *parent,
> +                                        struct regmap *olb)
> +{
> +       struct eq5c_ospi_div *div;
> +       int ret;
> +
> +       div = kzalloc(sizeof(*div), GFP_KERNEL);
> +       if (!div)
> +               return ERR_PTR(-ENOENT);
> +
> +       div->olb = olb;
> +       div->hw.init = CLK_HW_INIT_HW(EQ5C_OSPI_DIV_CLK_NAME, parent,
> +                                     &eq5c_ospi_div_ops, 0);
> +
> +       ret = clk_hw_register(NULL, &div->hw);
> +       if (ret) {
> +               pr_err("failed registering div_ospi: %d\n", ret);
> +               kfree(div);
> +               return ERR_PTR(-ENOENT);

return ERR_PTR(ret)

> +       }
> +
> +       return &div->hw;
> +}
> +
>  static void eq5c_init(struct device_node *np)
>  {
>         struct device_node *parent_np = of_get_parent(np);