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

Marek Vasut
2022-02-18T01:26:28Z

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

[...]

>> +#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()

[...]

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

Remnant from development, dropped.

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

>> + 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;
>> +} …