Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752106AbcL1XRF (ORCPT <rfc822;w@1wt.eu>);
        Wed, 28 Dec 2016 18:17:05 -0500
Received: from smtp.codeaurora.org ([198.145.29.96]:33072 "EHLO
        smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751956AbcL1XRE (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 28 Dec 2016 18:17:04 -0500
DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 76A3860F78
Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org
Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=sboyd@codeaurora.org
Date: Wed, 28 Dec 2016 15:16:59 -0800
From: Stephen Boyd <sboyd@codeaurora.org>
To: Vivek Gautam <vivek.gautam@codeaurora.org>
Cc: robh+dt@kernel.org, kishon@ti.com, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, mark.rutland@arm.com,
        srinivas.kandagatla@linaro.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v3 4/4] phy: qcom-qmp: new qmp phy driver for
 qcom-chipsets
Message-ID: <20161228231659.GD17126@codeaurora.org>
References: <1482253431-23160-1-git-send-email-vivek.gautam@codeaurora.org>
 <1482253431-23160-5-git-send-email-vivek.gautam@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1482253431-23160-5-git-send-email-vivek.gautam@codeaurora.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3135
Lines: 95

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