Re: [PATCH 2/2] clk: rs9: Add Renesas 9-series PCIe clock generator driver

[Marek Vasut <marex@denx.de>]

On 2/18/22 23:15, Stephen Boyd wrote:

Hi,

>>>> @@ -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?

I suspect it is IDT IP, since the datasheet looks similar to what 
versaclock datasheets used to look like, except the register layout is 
much simpler and the registers are completely different. So it _might_ 
be some mutation of the IDT PLL, and it is now owned by renesas.

The renesas SoC clock IP is something entirely different from the IP 
used here.

>>>> +#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?

Yes

[...]

>>> 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.

Setting parent_clk to NULL should be enough.

[...]