Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: UO6hnEJI0jgCptRStUSRHFswz44PU36PtuzA1j/MOVO/Wf3vyXKXm8t2407133kv/lcB/weKCB
 iwOybFQPdl7Q==
IronPort-SDR: gogS56CzkeTAhj4gf/NBkhnc2YLR41DiO9IoL86fVUfYgn0W5qOamzEYizWTABiIY9+PgZGvOB
 3L2qC5eQ8GNQ==
Subject: Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
To:     Vinod Koul <vkoul@kernel.org>
Cc:     linux-kernel@vger.kernel.org, kishon@ti.com,
        devicetree@vger.kernel.org, lee.jones@linaro.org, arnd@arndb.de,
        robh@kernel.org, andriy.shevchenko@intel.com,
        cheol.yong.kim@intel.com, chuanhua.lei@linux.intel.com,
        qi-ming.wu@intel.com, yixin.zhu@intel.com
References: <cover.1588230494.git.eswara.kota@linux.intel.com>
 <af8a7d7025990d22f6062953247cf80e64c6fd2f.1588230494.git.eswara.kota@linux.intel.com>
 <20200504072923.GN1375924@vkoul-mobl>
 <f12e76ac-e0fd-4afa-e1cd-2b90f175adfd@linux.intel.com>
 <20200504092034.GS1375924@vkoul-mobl>
From:   Dilip Kota <eswara.kota@linux.intel.com>
Message-ID: <0de7ba47-bc98-9267-46b4-230a86151c2d@linux.intel.com>
Date:   Mon, 4 May 2020 17:32:32 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200504092034.GS1375924@vkoul-mobl>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 5/4/2020 5:20 PM, Vinod Koul wrote:
> On 04-05-20, 16:26, Dilip Kota wrote:
>> On 5/4/2020 3:29 PM, Vinod Koul wrote:
>>> On 30-04-20, 15:15, Dilip Kota wrote:
>>>
>>>> +					  u32 mask, u32 val)
>>>> +{
>>>> +	u32 reg_val;
>>>> +
>>>> +	reg_val = readl(base + reg);
>>>> +	reg_val &= ~mask;
>>>> +	reg_val |= FIELD_PREP(mask, val);
>>>> +	writel(reg_val, base + reg);
>>> bypassing regmap here... why?
>> It is not regmap address, one of the below two addresses are passed to this
>> function.
> okay, perhaps add a comment somewhere that regmap is not used for this
> base?
I dont see a need of adding a comment, describing don't do regmap here.
>
>> struct intel_combo_phy {
>> ...
>>          void __iomem            *app_base;
>>          void __iomem            *cr_base;
>> ...
>> }
>
>>>> +static int intel_cbphy_calibrate(struct phy *phy)
>>>> +{
>>>> +	struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy);
>>>> +	struct intel_combo_phy *cbphy = iphy->parent;
>>>> +	void __iomem *cr_base = cbphy->cr_base;
>>>> +	int val, ret, id;
>>>> +
>>>> +	if (cbphy->phy_mode != PHY_XPCS_MODE)
>>>> +		return 0;
>>>> +
>>>> +	id = PHY_ID(iphy);
>>>> +
>>>> +	/* trigger auto RX adaptation */
>>>> +	combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id),
>>>> +			       ADAPT_REQ_MSK, 3);
>>>> +	/* Wait RX adaptation to finish */
>>>> +	ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, id),
>>>> +				 val, val & RX_ADAPT_ACK_BIT, 10, 5000);
>>>> +	if (ret)
>>>> +		dev_err(cbphy->dev, "RX Adaptation failed!\n");
>>> you want to continue her and not return error?
>> Next step is stopping the Adaptation, it should be done in both error and
>> success case.
> Again documenting this helps, pls add some comments on this behaviour
Comments are already in place, mentioning Start and Stop of Rx 
Adaptation. And Stop is being is done as Start is triggered, so not 
needed to mention error and success.

Regards,
Dilip
>