Re: [PATCH v3 3/5] clk: qcom: Add QDU1000 and QRU1000 GCC support

[Bjorn Andersson <andersson@kernel.org>]

On Wed, Oct 26, 2022 at 12:04:39PM -0700, Melody Olvera wrote:
> diff --git a/drivers/clk/qcom/gcc-qdu1000.c b/drivers/clk/qcom/gcc-qdu1000.c
> new file mode 100644
> index 000000000000..7bd8ebf0ddb5
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-qdu1000.c
> @@ -0,0 +1,2645 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,gcc-qdu1000.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "clk-regmap-divider.h"
> +#include "clk-regmap-mux.h"
> +#include "clk-regmap-phy-mux.h"
> +#include "reset.h"
> +
> +enum {
> +	P_BI_TCXO,
> +	P_GCC_GPLL0_OUT_EVEN,
> +	P_GCC_GPLL0_OUT_MAIN,
> +	P_GCC_GPLL1_OUT_MAIN,
> +	P_GCC_GPLL2_OUT_MAIN,
> +	P_GCC_GPLL3_OUT_MAIN,
> +	P_GCC_GPLL4_OUT_MAIN,
> +	P_GCC_GPLL5_OUT_MAIN,
> +	P_GCC_GPLL6_OUT_MAIN,
> +	P_GCC_GPLL7_OUT_MAIN,
> +	P_GCC_GPLL8_OUT_MAIN,
> +	P_PCIE_0_PHY_AUX_CLK,
> +	P_PCIE_0_PIPE_CLK,
> +	P_SLEEP_CLK,
> +	P_USB3_PHY_WRAPPER_GCC_USB30_PIPE_CLK,
> +};
[..]
> +static const struct clk_parent_data gcc_parent_data_1[] = {
> +	{ .index = P_BI_TCXO },
> +	{ .hw = &gcc_gpll0.clkr.hw },
> +	{ .index = P_SLEEP_CLK },

.index here refers to the index in the clocks property in DT.

I think it's okay to reuse the parent-enum, but the entries within must
then match the order defined in the DT binding. So you need to ensure
that the first N entires in the enum matches the binding.

Perhaps it's cleaner to just carry a separate enum for the clocks order,
as we've done in the other drivers?

If nothing else it makes it clear that one number space is arbitrary and
internal to the driver and the other is ABI.

> +	{ .hw = &gcc_gpll0_out_even.clkr.hw },
> +};
> +
[..]
> +static struct clk_regmap_mux gcc_pcie_0_phy_aux_clk_src = {
> +	.reg = 0x9d080,
> +	.shift = 0,
> +	.width = 2,
> +	.parent_map = gcc_parent_map_6,
> +	.clkr = {
> +		.hw.init = &(const struct clk_init_data){

Sorry for being picky, but I do like when there's a space between the
')' and '{' in these lines...

> +			.name = "gcc_pcie_0_phy_aux_clk_src",
> +			.parent_data = gcc_parent_data_6,
> +			.num_parents = ARRAY_SIZE(gcc_parent_data_6),
> +			.ops = &clk_regmap_mux_closest_ops,
> +		},
> +	},
> +};

Regards,
Bjorn