Quoting Alvin Šipraga (2024-05-17 06:02:15) > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index 3e9099504fad..a3d54b077e68 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -257,6 +257,13 @@ config COMMON_CLK_LAN966X > LAN966X SoC. GCK generates and supplies clock to various peripherals > within the SoC. > > +config COMMON_CLK_AD24XX > + bool "Clock driver for Analog Devices Inc. AD24xx" tristate > + depends on A2B_AD24XX_NODE Please make it be COMPILE_TESTed as well? > + help > + This driver supports the clock output functionality of AD24xx series > + A2B transceiver chips. > + > config COMMON_CLK_ASPEED > bool "Clock driver for Aspeed BMC SoCs" > depends on ARCH_ASPEED || COMPILE_TEST > diff --git a/drivers/clk/clk-ad24xx.c b/drivers/clk/clk-ad24xx.c > new file mode 100644 > index 000000000000..ed227c317faa > --- /dev/null > +++ b/drivers/clk/clk-ad24xx.c > @@ -0,0 +1,341 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * AD24xx clock driver > + * > + * Copyright (c) 2023 Alvin Šipraga <alsi@xxxxxxxxxxxxxxx> > + */ > + > +#include <linux/a2b/a2b.h> > +#include <linux/a2b/ad24xx.h> > +#include <linux/clk-provider.h> > +#include <linux/module.h> > +#include <linux/regmap.h> Include header for static_assert() at least. There's probably more that are needed, please check. > + > +#define AD24XX_NUM_CLKS 2 > + > +/* Define some safe macros to make the code more readable */ > +#define A2B_CLKCFG(_idx) (!(_idx) ? A2B_CLK1CFG : A2B_CLK2CFG) > + > +#define A2B_CLKCFG_DIV_SHIFT A2B_CLK1CFG_CLK1DIV_SHIFT > +#define A2B_CLKCFG_PDIV_SHIFT A2B_CLK1CFG_CLK1PDIV_SHIFT > + > +#define A2B_CLKCFG_DIV_MASK A2B_CLK1CFG_CLK1DIV_MASK > +#define A2B_CLKCFG_PDIV_MASK A2B_CLK1CFG_CLK1PDIV_MASK > +#define A2B_CLKCFG_INV_MASK A2B_CLK1CFG_CLK1INV_MASK > +#define A2B_CLKCFG_EN_MASK A2B_CLK1CFG_CLK1EN_MASK > + > +static_assert(A2B_CLK1CFG_CLK1DIV_MASK == A2B_CLK2CFG_CLK2DIV_MASK); > +static_assert(A2B_CLK1CFG_CLK1PDIV_MASK == A2B_CLK2CFG_CLK2PDIV_MASK); > +static_assert(A2B_CLK1CFG_CLK1INV_MASK == A2B_CLK2CFG_CLK2INV_MASK); > +static_assert(A2B_CLK1CFG_CLK1EN_MASK == A2B_CLK2CFG_CLK2EN_MASK); > + > +struct ad24xx_clkout { > + struct clk_hw hw; > + unsigned int idx; > + bool registered; > +}; > + > +struct ad24xx_clk { > + struct device *dev; Is this used? > + struct a2b_func *func; Is this used? > + struct a2b_node *node; Is this used? > + struct regmap *regmap; > + struct clk_hw *pll_hw; Is this used outside of probe? > + struct ad24xx_clkout clkouts[AD24XX_NUM_CLKS]; > +}; > + [..] > + > +static const struct regmap_config ad24xx_clk_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .cache_type = REGCACHE_RBTREE, No max_register? > +}; > + > +static struct clk_hw *ad24xx_clk_of_get(struct of_phandle_args *clkspec, void *data) > +{ > + struct ad24xx_clk *adclk = data; > + unsigned int idx = clkspec->args[0]; > + > + if (idx >= AD24XX_NUM_CLKS) > + return ERR_PTR(-EINVAL); > + > + if (!adclk->clkouts[idx].registered) > + return ERR_PTR(-ENOENT); > + > + return &adclk->clkouts[idx].hw; > +} > + > +static int ad24xx_clk_probe(struct device *dev) > +{ > + struct a2b_func *func = to_a2b_func(dev); > + struct a2b_node *node = func->node; > + struct device_node *np = dev->of_node; > + char *pll_name; > + const char *sync_clk_name; > + struct ad24xx_clk *adclk; > + int num_clks; > + int ret; > + int i; > + > + /* > + * Older series AD240x and AD241x chips have a single discrete > + * A2B_CLKCFG register that behaves differently to the A2B_CLKnCFG > + * registers of the later AD242x series. This driver only supports the > + * latter right now. > + */ > + if (!(node->chip_info->caps & A2B_CHIP_CAP_CLKOUT)) > + return -ENODEV; Maybe print a warning message to make it more obvious. > + > + adclk = devm_kzalloc(dev, sizeof(*adclk), GFP_KERNEL); > + if (!adclk) > + return -ENOMEM; > + > + adclk->regmap = > + devm_regmap_init_a2b_func(func, &ad24xx_clk_regmap_config); Put it on one line please . > + if (IS_ERR(adclk->regmap)) > + return PTR_ERR(adclk->regmap); > + > + adclk->dev = dev; > + adclk->func = func; > + adclk->node = node; > + dev_set_drvdata(dev, adclk); > + > + num_clks = of_property_count_strings(np, "clock-output-names"); > + if (num_clks < 0 || num_clks > AD24XX_NUM_CLKS) > + return -EINVAL; Please register all the clks provided by this chip. > + > + /* > + * Register the PLL internally to use it as the parent of the CLKOUTs. > + * The PLL runs at 2048 times the SYNC clock rate. > + */ > + pll_name = > + devm_kasprintf(dev, GFP_KERNEL, "%s_pll", dev_name(&node->dev)); > + if (!pll_name) > + return -ENOMEM; > + sync_clk_name = __clk_get_name(a2b_node_get_sync_clk(func->node)); > + adclk->pll_hw = devm_clk_hw_register_fixed_factor( > + dev, pll_name, sync_clk_name, 0, 2048, 1); I think this should be devm_clk_hw_register_fixed_factor_fwname(). > + if (IS_ERR(adclk->pll_hw)) > + return PTR_ERR(adclk->pll_hw); > + > + for (i = 0; i < num_clks; i++) { > + struct clk_init_data init = { }; > + const char *parent_names = clk_hw_get_name(adclk->pll_hw); Please use struct clk_parent_data instead of strings to describe topology. > + unsigned int idx = i; > + > + /* Clock outputs can be skipped with the clock-indices property */ > + of_property_read_u32_index(np, "clock-indices", i, &idx); > + if (idx > AD24XX_NUM_CLKS) > + return -EINVAL; > + > + ret = of_property_read_string_index(np, "clock-output-names", i, > + &init.name); The name should only be for debug purposes. Please don't use clock-output-names DT property. If you need to make it unique perhaps you can add in the device name or something like that? > + if (ret) > + return ret; > + > + init.ops = &ad24xx_clk_ops; > + init.parent_names = &parent_names; > + init.num_parents = 1; > + > + adclk->clkouts[idx].hw.init = &init; > + adclk->clkouts[idx].idx = idx; > + adclk->clkouts[idx].registered = true; > + > + ret = devm_clk_hw_register(dev, &adclk->clkouts[idx].hw); > + if (ret) > + return ret; > + } > + > + ret = devm_of_clk_add_hw_provider(dev, ad24xx_clk_of_get, adclk); > + if (ret) > + return ret; > + > + return 0; Please just return devm_of_clk_add_hw_provider(...) to prevent the cleanup crews from sending a followup patch. > +} > + > +static const struct of_device_id ad24xx_clk_of_match_table[] = { > + { .compatible = "adi,ad2420-clk" }, > + { .compatible = "adi,ad2421-clk" }, > + { .compatible = "adi,ad2422-clk" }, > + { .compatible = "adi,ad2425-clk" }, > + { .compatible = "adi,ad2426-clk" }, > + { .compatible = "adi,ad2427-clk" }, > + { .compatible = "adi,ad2428-clk" }, > + { .compatible = "adi,ad2429-clk" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, ad24xx_clk_of_match_table); > + > +static struct a2b_driver ad24xx_clk_driver = { I guess because this isn't a platform driver I can't merge this through the clk tree? Is there any difference from the platform bus? > + .driver = { > + .name = "ad24xx-clk", > + .of_match_table = ad24xx_clk_of_match_table, > + .probe_type = PROBE_PREFER_ASYNCHRONOUS, > + }, > + .probe = ad24xx_clk_probe, > +}; > +module_a2b_driver(ad24xx_clk_driver);