Re: [PATCH v1 3/4] clk: qcom: add clock controller driver for qca8386/qca8084

[Krzysztof Kozlowski]

On 09/08/2023 10:00, Luo Jie wrote:
> Add clock & reset controller driver for qca8386/qca8084.
> 
> Signed-off-by: Luo Jie <quic_luoj@xxxxxxxxxxx>
> ---
>  drivers/clk/qcom/Kconfig       |    8 +
>  drivers/clk/qcom/Makefile      |    1 +
>  drivers/clk/qcom/nsscc-qca8k.c | 2195 ++++++++++++++++++++++++++++++++
>  3 files changed, 2204 insertions(+)
>  create mode 100644 drivers/clk/qcom/nsscc-qca8k.c
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 263e55d75e3f..d84705ff920d 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -195,6 +195,14 @@ config IPQ_GCC_9574
>  	  i2c, USB, SD/eMMC, etc. Select this for the root clock
>  	  of ipq9574.
>  
> +config IPQ_NSSCC_QCA8K
> +	tristate "QCA8K(QCA8386 or QCA8084) NSS Clock Controller"

Is it specific to some arch? We keep ARM or ARM64 for most of the entries.

> +	help
> +	  Support for NSS(Network SubSystem) clock controller on
> +	  qca8386/qca8084 chip.
> +	  Say Y if you want to use network features of switch or PHY
> +	  device. Select this for the root clock of qca8k.
> +
>  config MSM_GCC_8660
>  	tristate "MSM8660 Global Clock Controller"
>  	depends on ARM || COMPILE_TEST

...

> +static int nss_cc_qca8k_probe(struct mdio_device *mdiodev)
> +{
> +	struct device *dev = &mdiodev->dev;
> +	struct regmap *regmap;
> +	struct qcom_reset_controller *reset;
> +	struct qcom_cc_desc desc = nss_cc_qca8k_desc;
> +	size_t num_clks = desc.num_clks;
> +	struct clk_regmap **rclks = desc.clks;
> +	struct qcom_cc *cc;
> +	int ret, i;
> +
> +	cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
> +	if (!cc)
> +		return -ENOMEM;
> +
> +	cc->rclks = rclks;
> +	cc->num_rclks = num_clks;
> +	reset = &cc->reset;
> +
> +	regmap = devm_regmap_init(dev, NULL, mdiodev->bus, desc.config);
> +

Drop blank line.

> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "Failed to init MDIO regmap\n");

All of error returns could be converted return dev_err_probe(), just to
have smaller code. Not a requirement, though.

> +		return PTR_ERR(regmap);
> +	}
> +
> +	reset->rcdev.of_node = dev->of_node;
> +	reset->rcdev.dev = dev;
> +	reset->rcdev.ops = &qcom_reset_ops;
> +	reset->rcdev.owner = dev->driver->owner;
> +	reset->rcdev.nr_resets = desc.num_resets;
> +	reset->regmap = regmap;
> +	reset->reset_map = desc.resets;
> +
> +	ret = devm_reset_controller_register(dev, &reset->rcdev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register QCA8K reset controller: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < num_clks; i++) {
> +		if (!rclks[i])
> +			continue;
> +
> +		ret = devm_clk_register_regmap(dev, rclks[i]);
> +		if (ret) {
> +			dev_err(dev, "Failed to regmap register for QCA8K clock: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(dev, qcom_qca8k_clk_hw_get, cc);
> +	if (ret) {
> +		dev_err(dev, "Failed to register provider for QCA8K clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(dev, "Registered NSSCC QCA8K clocks\n");

Drop the simple info for probe status. Kernel has other ways to do this.

> +	return ret;
> +}
> +
> +static const struct of_device_id nss_cc_qca8k_match_table[] = {
> +	{ .compatible = "qcom,qca8085-nsscc" },
> +	{ .compatible = "qcom,qca8084-nsscc" },
> +	{ .compatible = "qcom,qca8082-nsscc" },
> +	{ .compatible = "qcom,qca8386-nsscc" },
> +	{ .compatible = "qcom,qca8385-nsscc" },
> +	{ .compatible = "qcom,qca8384-nsscc" },

You only need qca8084 here. Drop all other entries.

> +	{ }
> +};



Best regards,
Krzysztof