Quoting Marek Vasut (2022-02-13 09:33:10)
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 6a98291350b64..3ec27842ec779 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_COMMON_CLK_STM32MP157) += clk-stm32mp1.o
> obj-$(CONFIG_COMMON_CLK_TPS68470) += clk-tps68470.o
> obj-$(CONFIG_CLK_TWL6040) += clk-twl6040.o
> obj-$(CONFIG_ARCH_VT8500) += clk-vt8500.o
> +obj-$(CONFIG_COMMON_CLK_RS9_PCIE) += clk-renesas-pcie.o
Is there a reason it doesn't go into drivers/clk/renesas?
> obj-$(CONFIG_COMMON_CLK_VC5) += clk-versaclock5.o
> obj-$(CONFIG_COMMON_CLK_WM831X) += clk-wm831x.o
> obj-$(CONFIG_COMMON_CLK_XGENE) += clk-xgene.o
> diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
> new file mode 100644
> index 0000000000000..0ad132cbe41b8
> --- /dev/null
> +++ b/drivers/clk/clk-renesas-pcie.c
> @@ -0,0 +1,336 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Driver for Renesas 9-series PCIe clock generator driver
> + *
> + * The following series can be supported:
> + * - 9FGV/9DBV/9DMV/9FGL/9DML/9QXL/9SQ
> + * Currently supported:
> + * - 9FGV0241
> + *
> + * Copyright (C) 2022 Marek Vasut <marex@xxxxxxx>
> + */
> +
> +#include <linux/clk.h>
Drop this include please.
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
Is this used? If not please remove.
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
Is this used? If not please remove.
> +#include <linux/rational.h>
Is this used? If not please remove.
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define RS9_REG_OE 0x0
> +#define RS9_REG_OE_DIF_OE(n) BIT((n) + 1)
> +#define RS9_REG_SS 0x1
> +#define RS9_REG_SS_AMP_0V6 0x0
> +#define RS9_REG_SS_AMP_0V7 0x1
> +#define RS9_REG_SS_AMP_0V8 0x2
> +#define RS9_REG_SS_AMP_0V9 0x3
> +#define RS9_REG_SS_AMP_MASK 0x3
> +#define RS9_REG_SS_SSC_100 0
> +#define RS9_REG_SS_SSC_M025 (1 << 3)
> +#define RS9_REG_SS_SSC_M050 (3 << 3)
> +#define RS9_REG_SS_SSC_MASK (3 << 3)
> +#define RS9_REG_SS_SSC_LOCK BIT(5)
> +#define RS9_REG_SR 0x2
> +#define RS9_REG_SR_2V0_DIF(n) 0
> +#define RS9_REG_SR_3V0_DIF(n) BIT((n) + 1)
> +#define RS9_REG_SR_DIF_MASK(n) BIT((n) + 1)
> +#define RS9_REG_REF 0x3
> +#define RS9_REG_REF_OE BIT(4)
> +#define RS9_REG_REF_OD BIT(5)
> +#define RS9_REG_REF_SR_SLOWEST 0
> +#define RS9_REG_REF_SR_SLOW (1 << 6)
> +#define RS9_REG_REF_SR_FAST (2 << 6)
> +#define RS9_REG_REF_SR_FASTER (3 << 6)
> +#define RS9_REG_VID 0x5
> +#define RS9_REG_DID 0x6
> +#define RS9_REG_BCP 0x7
> +
> +/* Supported Renesas 9-series models. */
> +enum rs9_model {
> + RENESAS_9FGV0241,
> +};
> +
> +/* Structure to describe features of a particular 9-series model */
> +struct rs9_chip_info {
> + const enum rs9_model model;
> + unsigned int num_clks;
> +};
> +
> +struct rs9_driver_data {
> + struct i2c_client *client;
> + struct regmap *regmap;
> + const struct rs9_chip_info *chip_info;
> + struct clk *pin_xin;
> + struct clk_hw *clk_dif[2];
> + u8 pll_amplitude;
> + u8 pll_ssc;
> + u8 clk_dif_sr;
> +};
> +
> +/*
> + * Renesas 9-series i2c regmap
> + */
> +static const struct regmap_range rs9_readable_ranges[] = {
> + regmap_reg_range(RS9_REG_OE, RS9_REG_REF),
> + regmap_reg_range(RS9_REG_VID, RS9_REG_BCP),
> +};
> +
> +static const struct regmap_access_table rs9_readable_table = {
> + .yes_ranges = rs9_readable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(rs9_readable_ranges),
> +};
> +
> +static const struct regmap_range rs9_writeable_ranges[] = {
> + regmap_reg_range(RS9_REG_OE, RS9_REG_REF),
> + regmap_reg_range(RS9_REG_BCP, RS9_REG_BCP),
> +};
> +
> +static const struct regmap_access_table rs9_writeable_table = {
> + .yes_ranges = rs9_writeable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(rs9_writeable_ranges),
> +};
> +
> +static const struct regmap_config rs9_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .cache_type = REGCACHE_RBTREE,
> + .max_register = 0x8,
That's a huge cache!
> + .rd_table = &rs9_readable_table,
> + .wr_table = &rs9_writeable_table,
> +};
> +
> +static int rs9_get_output_config(struct rs9_driver_data *rs9, int idx)
> +{
> + struct i2c_client *client = rs9->client;
> + unsigned char *name = "DIF0";
> + struct device_node *np;
> + int ret;
> + u32 sr;
> +
> + /* Set defaults */
> + rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
> + rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx);
> +
> + name[3] += idx;
> + np = of_get_child_by_name(client->dev.of_node, name);
> + if (!np)
> + return 0;
> +
> + /* Output clock slew rate */
> + ret = of_property_read_u32(np, "renesas,slew-rate", &sr);
Put of_node_put(np) here please
> + if (!ret) {
> + if (sr == 2000000) { /* 2V/ns */
> + rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
> + rs9->clk_dif_sr |= RS9_REG_SR_2V0_DIF(idx);
> + } else if (sr == 3000000) { /* 3V/ns (default) */
> + rs9->clk_dif_sr &= ~RS9_REG_SR_DIF_MASK(idx);
> + rs9->clk_dif_sr |= RS9_REG_SR_3V0_DIF(idx);
> + } else
> + ret = dev_err_probe(&client->dev, -EINVAL,
> + "Invalid renesas,slew-rate value\n");
> + }
> +
> + of_node_put(np);
> +
> + return ret;
> +}
> +
> +static int rs9_get_common_config(struct rs9_driver_data *rs9)
> +{
> + struct i2c_client *client = rs9->client;
> + struct device_node *np = client->dev.of_node;
> + unsigned int amp, ssc;
> + int ret;
> +
> + /* Set defaults */
> + rs9->pll_amplitude = RS9_REG_SS_AMP_0V7;
> + rs9->pll_ssc = RS9_REG_SS_SSC_100;
> +
> + /* Output clock amplitude */
> + ret = of_property_read_u32(np, "renesas,out-amplitude", &);
> + if (!ret) {
> + if (amp == 600000) /* 0.6V */
> + rs9->pll_amplitude = RS9_REG_SS_AMP_0V6;
> + else if (amp == 700000) /* 0.7V (default) */
> + rs9->pll_amplitude = RS9_REG_SS_AMP_0V7;
> + else if (amp == 800000) /* 0.8V */
> + rs9->pll_amplitude = RS9_REG_SS_AMP_0V8;
> + else if (amp == 900000) /* 0.9V */
> + rs9->pll_amplitude = RS9_REG_SS_AMP_0V9;
> + else
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Invalid renesas,out-amplitude value\n");
> + }
> +
> + /* Output clock spread spectrum */
> + ret = of_property_read_u32(np, "renesas,out-spread-spectrum", &ssc);
> + if (!ret) {
> + if (ssc == 100000) /* 100% ... no spread (default) */
> + rs9->pll_ssc = RS9_REG_SS_SSC_100;
> + else if (ssc == 99750) /* -0.25% ... down spread */
> + rs9->pll_ssc = RS9_REG_SS_SSC_M025;
> + else if (ssc == 99500) /* -0.50% ... down spread */
> + rs9->pll_ssc = RS9_REG_SS_SSC_M050;
> + else
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Invalid renesas,out-spread-spectrum value\n");
> + }
> +
> + return 0;
> +}
> +
> +static void rs9_update_config(struct rs9_driver_data *rs9)
> +{
> + int i;
> +
> + /* If amplitude is non-default, update it. */
> + if (rs9->pll_amplitude != RS9_REG_SS_AMP_0V7) {
> + regmap_update_bits(rs9->regmap, RS9_REG_SS, RS9_REG_SS_AMP_MASK,
> + rs9->pll_amplitude);
> + }
> +
> + /* If SSC is non-default, update it. */
> + if (rs9->pll_ssc != RS9_REG_SS_SSC_100) {
> + regmap_update_bits(rs9->regmap, RS9_REG_SS, RS9_REG_SS_SSC_MASK,
> + rs9->pll_ssc);
> + }
> +
> + for (i = 0; i < rs9->chip_info->num_clks; i++) {
> + if (rs9->clk_dif_sr & RS9_REG_SR_3V0_DIF(i))
> + continue;
> +
> + regmap_update_bits(rs9->regmap, RS9_REG_SR, RS9_REG_SR_3V0_DIF(i),
> + rs9->clk_dif_sr & RS9_REG_SR_3V0_DIF(i));
> + }
> +}
> +
> +static struct clk_hw *
> +rs9_of_clk_get(struct of_phandle_args *clkspec, void *data)
> +{
> + struct rs9_driver_data *rs9 = data;
> + unsigned int idx = clkspec->args[0];
> +
> + return rs9->clk_dif[idx];
> +}
> +
> +static const struct of_device_id clk_rs9_of_match[];
Why the forward declare?
> +
> +static int rs9_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct device_node *np = client->dev.of_node;
> + unsigned char *name = "DIF0";
> + struct rs9_driver_data *rs9;
> + const char *parent_clk;
> + struct clk_hw *hw;
> + int i, ret;
> +
> + rs9 = devm_kzalloc(&client->dev, sizeof(*rs9), GFP_KERNEL);
> + if (!rs9)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, rs9);
> + rs9->client = client;
> + rs9->chip_info = of_device_get_match_data(&client->dev);
Check for NULL? Use device_get_match_data()? Or does that not work for
i2c devices?
> +
> + /* Fetch common configuration from DT (if specified) */
> + ret = rs9_get_common_config(rs9);
> + if (ret)
> + return ret;
> +
> + /* Fetch DIFx output configuration from DT (if specified) */
> + for (i = 0; i < rs9->chip_info->num_clks; i++) {
> + ret = rs9_get_output_config(rs9, i);
> + if (ret)
> + return ret;
> + }
> +
> + /* Mandatory XTal */
Oh it's mandatory here but not in the binding?
> + parent_clk = of_clk_get_parent_name(np, 0);
Use clk_parent_data please.
> + if (!parent_clk)
> + return dev_err_probe(&client->dev, -EINVAL,
> + "Missing XTal input clock\n");
> +
> + rs9->regmap = devm_regmap_init_i2c(client, &rs9_regmap_config);
> + if (IS_ERR(rs9->regmap))
> + return dev_err_probe(&client->dev, PTR_ERR(rs9->regmap),
> + "Failed to allocate register map\n");
> +
> + /* Register clock */
> + for (i = 0; i < rs9->chip_info->num_clks; i++) {
> + name[3]++;
> + hw = clk_hw_register_fixed_factor(&client->dev, name,
> + parent_clk, 0, 4, 1);
> + if (IS_ERR(hw))
> + return PTR_ERR(hw);
> +
> + rs9->clk_dif[i] = hw;
> + }
> +
> + ret = devm_of_clk_add_hw_provider(&client->dev, rs9_of_clk_get, rs9);
> + if (!ret)
> + rs9_update_config(rs9);
> +
> + return ret;
> +}
