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 <[email protected]>
> Tested-by: Srinivas Kandagatla <[email protected]>
> ---
> 
> +
> +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

Reply via email to