Hi Stephan, Thanks for the review, > -----Original Message----- > From: Stephen Boyd [mailto:sboyd@xxxxxxxxxx] > Sent: Monday, March 19, 2018 1:10 PM > To: Jolly Shah <JOLLYS@xxxxxxxxxx>; linux-clk@xxxxxxxxxxxxxxx; > mark.rutland@xxxxxxx; michal.simek@xxxxxxxxxx; mturquette@xxxxxxxxxxxx; > robh+dt@xxxxxxxxxx; sboyd@xxxxxxxxxxxxxx > Cc: Rajan Vaja <RAJANV@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jolly Shah > <JOLLYS@xxxxxxxxxx>; Tejas Patel <TEJASP@xxxxxxxxxx>; Shubhrajyoti Datta > <shubhraj@xxxxxxxxxx> > Subject: Re: [PATCH 3/3] drivers: clk: Add ZynqMP clock driver > > > +/** > > + * struct zynqmp_clock - Structure for clock > > + * @clk_name: Clock name > > + * @valid: Validity flag of clock > > + * @init_enable: init_enable flag of clock > > + * @type: Clock type (Output/External) > > + * @node: Clock tolology nodes > > + * @num_nodes: Number of nodes present in topology > > + * @parent: structure of parent of clock > > + * @num_parents: Number of parents of clock > > + */ > > +struct zynqmp_clock { > > + char clk_name[MAX_NAME_LEN]; > > + u32 valid; > > + u32 init_enable; > > + enum clk_type type; > > Is this ever assigned? Yes its assigned in zynqmp_get_clock_info function below. > > > + * Return: Returns status, either success or error+reason. > > + */ > > +static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index, u32 > *parents) > > +{ > > + struct zynqmp_pm_query_data qdata = {0}; > > + u32 ret_payload[PAYLOAD_ARG_CNT]; > > + int ret; > > + > > + qdata.qid = PM_QID_CLOCK_GET_PARENTS; > > + qdata.arg1 = clock_id; > > + qdata.arg2 = index; > > + > > + ret = eemi_ops->query_data(qdata, ret_payload); > > + memcpy(parents, &ret_payload[1], CLK_GET_PARENTS_RESP_WORDS * > 4); > > Is 4 sizeof(u32)? Or is it supposed to be 3 for the 3 parents returned? It should be 3. > > > + > > +/** > > + * zynqmp_clk_setup() - Setup the clock framework and register clocks > > + * @np: Device node > > + */ > > +static void __init zynqmp_clk_setup(struct device_node *np) > > +{ > > + int idx; > > + > > + idx = of_property_match_string(np, "clock-names", "pss_ref_clk"); > > + if (idx < 0) { > > + pr_err("pss_ref_clk not provided\n"); > > + return; > > + } > > + idx = of_property_match_string(np, "clock-names", "video_clk"); > > + if (idx < 0) { > > + pr_err("video_clk not provided\n"); > > + return; > > + } > > + idx = of_property_match_string(np, "clock-names", "pss_alt_ref_clk"); > > + if (idx < 0) { > > + pr_err("pss_alt_ref_clk not provided\n"); > > + return; > > + } > > + idx = of_property_match_string(np, "clock-names", "aux_ref_clk"); > > + if (idx < 0) { > > + pr_err("aux_ref_clk not provided\n"); > > + return; > > + } > > + idx = of_property_match_string(np, "clock-names", "gt_crx_ref_clk"); > > + if (idx < 0) { > > + pr_err("aux_ref_clk not provided\n"); > > + return; > > + } > > Why are we doing all this? Please don't verify DT contents in driver > code. The intent is not to go ahead with other clock registration if these external clocks are not available. Where should it be validated? > > + > > +/** > > + * pll_get_mode - Get mode of PLL > > + * @hw: Handle between common and hardware-specific interfaces > > + * > > + * Return: Mode of PLL > > + */ > > +static inline enum pll_mode pll_get_mode(struct clk_hw *hw) > > +{ > > + struct zynqmp_pll *clk = to_zynqmp_pll(hw); > > + u32 clk_id = clk->clk_id; > > + const char *clk_name = clk_hw_get_name(hw); > > + u32 ret_payload[PAYLOAD_ARG_CNT]; > > How big is PAYLOAD_ARG_CNT? > Its 5. Rest all comments will be taken care in next version. Thanks, Jolly Shah ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
