Messages in this thread | | | Date | Wed, 28 Dec 2016 15:16:59 -0800 | From | Stephen Boyd <> | Subject | Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets |
| |
On 12/20, Vivek Gautam wrote: > Qualcomm SOCs have QMP phy controller that provides support > to a number of controller, viz. PCIe, UFS, and USB. > Add a new driver, based on generic phy framework, for this > phy controller. > > Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> > Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > > + > +static struct phy *qcom_qmp_phy_xlate(struct device *dev, > + struct of_phandle_args *args) > +{ > + struct qcom_qmp_phy *qphy = dev_get_drvdata(dev); > + int i; > + > + if (WARN_ON(args->args[0] >= qphy->cfg->nlanes)) > + return ERR_PTR(-ENODEV); > + > + for (i = 0; i < qphy->cfg->nlanes; i++) > + /* phys[i]->index */ > + if (i == args->args[0]) > + return qphy->phys[i]->phy;
What's the loop for? If args->arg[0] < qphy->cfg->nlanes then we should be able to directly index the qphy->phys array with that number and return it.
> + > + return ERR_PTR(-ENODEV); > +} > + [...] > + > +/* > + * The <s>_pipe_clksrc generated by PHY goes to the GCC that gate > + * controls it. The <s>_pipe_clk coming out of the GCC is requested > + * by the PHY driver for its operations. > + * We register the <s>_pipe_clksrc here. The gcc driver takes care > + * of assigning this <s>_pipe_clksrc as parent to <s>_pipe_clk. > + * Below picture shows this relationship. > + * > + * +--------------+ > + * | PHY block |<<---------------------------------------+ > + * | | | > + * | +-------+ | +-----+ | > + * I/P---^-->| PLL |--^--->pipe_clksrc--->| GCC |--->pipe_clk---+ > + * clk | +-------+ | +-----+ > + * +--------------+
There are mixed tabs and spaces in this diagram causing confusion in my editor. Please make it only spaces so the picture comes out correctly.
> + * > + */ > +static int phy_pipe_clk_register(struct qcom_qmp_phy *qphy, int id) > +{ > + char clk_name[MAX_PROP_NAME];
I'm not sure MAX_PROP_NAME is the same as some max clk name but ok. We should be able to calculate that the maximum is length of usb3_phy_pipe_clk_src for now though?
> + struct clk *clk; > + > + memset(&clk_name, 0, sizeof(clk_name)); > + switch (qphy->cfg->type) { > + case PHY_TYPE_USB3: > + snprintf(clk_name, MAX_PROP_NAME, "usb3_phy_pipe_clk_src"); > + break; > + case PHY_TYPE_PCIE: > + snprintf(clk_name, MAX_PROP_NAME, "pcie_%d_pipe_clk_src", id); > + break; > + default: > + return -EINVAL; > + } > + > + /* controllers using QMP phys use 125MHz pipe clock interface */ > + clk = clk_register_fixed_rate(qphy->dev, clk_name, NULL, 0, 125000000);
I was hoping you would be able to calculate the actual output rate by reading hardware. This is ok too though. Just please use clk_hw_register_fixed_rate() instead. And you'll probably need some sort of devm() usage here to handle probe failure, so I would probably roll my own and allocate a fixed_rate clk structure and set the rate/name directly and then call devm_clk_hw_register().
> + > + return PTR_ERR_OR_ZERO(clk); -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
| |