Quoting Marek Vasut (2022-02-17 17:26:22)
> On 2/18/22 00:45, Stephen Boyd wrote:
> > 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?
>
> The drivers/clk/renesas/ is for renesas SoC (R-Car/RZ/...),
> this chip is different group (it's probably even IDT PLL IP).
Ah ok so it's not a renesas SoC but a renesas IP?
>
> [...]
>
> >> +#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.
>
> This one is for of_device_get_match_data()
>
So it's going away?
>
> >> +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.
>
> This one line I don't understand -- can you expand on what you expect me
> to do here ?
Use 'struct clk_parent_data' and set .index to 0 so that when
registering the clk you don't need to get the parent clk name.
>
> >> + 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);
To do that it looks like maybe we'll need to export
__clk_hw_register_fixed_factor() and introduces some sort of
clk_hw_register_fixed_factor_parent_data() API.
> >> + if (IS_ERR(hw))
> >> + return PTR_ERR(hw);
> >> +
> >> + rs9->clk_dif[i] = hw;
