Re: [PATCH v2 03/11] clk: renesas: clk-vbattb: Add VBATTB clock driver

[Stephen Boyd <sboyd@kernel.org>]

Quoting Claudiu (2024-07-16 03:30:17)
> diff --git a/drivers/clk/renesas/clk-vbattb.c b/drivers/clk/renesas/clk-vbattb.c
> new file mode 100644
> index 000000000000..8effe141fc0b
> --- /dev/null
> +++ b/drivers/clk/renesas/clk-vbattb.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * VBATTB clock driver
> + *
> + * Copyright (C) 2024 Renesas Electronics Corp.
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>

Prefer clk providers to not be clk consumers.

> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>

Is of_platform.h used?

Include mod_devicetable.h for of_device_id.

> +#include <linux/platform_device.h>
> +
> +#define VBATTB_BKSCCR                  0x0
> +#define VBATTB_BKSCCR_SOSEL            BIT(6)
> +#define VBATTB_SOSCCR2                 0x8
> +#define VBATTB_SOSCCR2_SOSTP2          BIT(0)
[..]
> +
> +static int vbattb_clk_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct clk_parent_data parent_data = {};
> +       struct device *dev = &pdev->dev;
> +       struct clk_init_data init = {};
> +       struct vbattb_clk *vbclk;
> +       u32 load_capacitance;
> +       struct clk_hw *hw;
> +       int ret, bypass;
> +
> +       vbclk = devm_kzalloc(dev, sizeof(*vbclk), GFP_KERNEL);
> +       if (!vbclk)
> +               return -ENOMEM;
> +
> +       vbclk->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(vbclk->base))
> +               return PTR_ERR(vbclk->base);
> +
> +       bypass = vbattb_clk_need_bypass(dev);

This is a tri-state bool :(

> +       if (bypass < 0) {
> +               return bypass;
> +       } else if (bypass) {
> +               parent_data.fw_name = "clkin";
> +               bypass = VBATTB_BKSCCR_SOSEL;

And now it is a mask value.

> +       } else {
> +               parent_data.fw_name = "xin";
> +       }
> +
> +       ret = of_property_read_u32(np, "renesas,vbattb-load-nanofarads", &load_capacitance);
> +       if (ret)
> +               return ret;
> +
> +       ret = vbattb_clk_validate_load_capacitance(vbclk, load_capacitance);
> +       if (ret)
> +               return ret;
> +
> +       vbattb_clk_update_bits(vbclk->base, VBATTB_BKSCCR, VBATTB_BKSCCR_SOSEL, bypass);

Please don't overload 'bypass'. Use two variables or a conditional.

I also wonder if this is really a mux, and either assigned-clock-parents
should be used, or the clk_ops should have an init routine that looks at
which parent is present by determining the index and then use that to
set the mux. The framework can take care of failing to set the other
parent when it isn't present.

> +
> +       spin_lock_init(&vbclk->lock);
> +
> +       init.name = "vbattclk";
> +       init.ops = &vbattb_clk_ops;
> +       init.parent_data = &parent_data;
> +       init.num_parents = 1;
> +       init.flags = 0;
> +
> +       vbclk->hw.init = &init;
> +       hw = &vbclk->hw;
> +
> +       ret = devm_clk_hw_register(dev, hw);
> +       if (ret)
> +               return ret;
> +
> +       return of_clk_add_hw_provider(np, of_clk_hw_simple_get, hw);
> +}
> +
> +static const struct of_device_id vbattb_clk_match[] = {
> +       { .compatible = "renesas,r9a08g045-vbattb-clk" },
> +       { /* sentinel */ }
> +};

Any MODULE_DEVICE_TABLE?