Hi, Stephen, On 17.07.2024 01:28, Stephen Boyd wrote: > 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. I added it here to be able to use devm_clk_get_optional() as it was proposed to me in v1 to avoid adding a new binding for bypass and detect if it's needed by checking the input clock name. > >> +#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. Ok. > >> +#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. OK. > > I also wonder if this is really a mux, It's a way to determine what type of clock (crystal oscillator or device clock) is connected to RTXIN/RTXOUT pins of the module (the module block diagram at [1]) based on the clock name. Depending on the type of the clock connected to RTXIN/RTXOUT we need to select the XC or XBYP as input for the mux at [1]. [1] https://gcdnb.pbrd.co/images/QYsCvhfQlX6n.png > 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. On the board, at any moment, it will be only one clock as input to the VBATTB clock (either crystal oscillator or a clock device). If I'm getting you correctly, this will involve describing both clocks in some scenarios. E.g. if want to use crystal osc, I can use this DT description: vbattclk: clock-controller@1c { compatible = "renesas,r9a08g045-vbattb-clk"; reg = <0 0x1c 0 0x10>; clocks = <&vbattb_xtal>; clock-names = "xin"; #clock-cells = <0>; status = "disabled"; }; vbattb_xtal: vbattb-xtal { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <32768>; }; If external clock device is to be used, I should describe a fake clock too: vbattclk: clock-controller@1c { compatible = "renesas,r9a08g045-vbattb-clk"; reg = <0 0x1c 0 0x10>; clocks = <&vbattb_xtal>, <&ext_clk>; clock-names = "xin", "clkin"; #clock-cells = <0>; status = "disabled"; }; vbattb_xtal: vbattb-xtal { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <0>; }; ext_clk: ext-clk { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <32768>; }; Is this what you are suggesting? > >> + >> + 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? I failed to added it. Thank you for your review, Claudiu Beznea