Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6573a50b-e414-9eba-5a70-f1993c7924c2@starfivetech.com>
Date:   Sun, 25 Jun 2023 10:11:44 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v5 2/3] phy: starfive: Add mipi dphy rx support
To:     Vinod Koul <vkoul@kernel.org>
CC:     Kishon Vijay Abraham I <kishon@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Emil Renner Berthing <kernel@esmil.dk>,
        Conor Dooley <conor@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Jack Zhu <jack.zhu@starfivetech.com>,
        <linux-phy@lists.infradead.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-riscv@lists.infradead.org>
References: <20230529121503.3544-1-changhuang.liang@starfivetech.com>
 <20230529121503.3544-3-changhuang.liang@starfivetech.com>
 <ZJLtVoi2qpKOR1l6@matsya>
Content-Language: en-US
From:   Changhuang Liang <changhuang.liang@starfivetech.com>
In-Reply-To: <ZJLtVoi2qpKOR1l6@matsya>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2023/6/21 20:30, Vinod Koul wrote:
> On 29-05-23, 05:15, Changhuang Liang wrote:
>> +static int stf_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
>> +{
>> +	struct stf_dphy *dphy = phy_get_drvdata(phy);
>> +	const struct stf_dphy_info *info = dphy->info;
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(stf_dphy_init_list); i++)
>> +		writel(stf_dphy_init_list[i].val,
>> +		       dphy->regs + stf_dphy_init_list[i].addr);
>> +
>> +	writel(FIELD_PREP(STF_DPHY_DA_CDPHY_R100_CTRL0_2D1C_EFUSE_EN, 1) |
>> +	       FIELD_PREP(STF_DPHY_DA_CDPHY_R100_CTRL0_2D1C_EFUSE_IN, 0x1b) |
>> +	       FIELD_PREP(STF_DPHY_DA_CDPHY_R100_CTRL1_2D1C_EFUSE_EN, 1) |
>> +	       FIELD_PREP(STF_DPHY_DA_CDPHY_R100_CTRL1_2D1C_EFUSE_IN, 0x1b),
>> +	       dphy->regs + STF_DPHY_APBCFGSAIF_SYSCFG(0));
>> +
>> +	writel(FIELD_PREP(STF_DPHY_DATA_BUS16_8, 0) |
>> +	       FIELD_PREP(STF_DPHY_DEBUG_MODE_SEL, 0x5a),
>> +	       dphy->regs + STF_DPHY_APBCFGSAIF_SYSCFG(184));
> 
> bunch of magic numbers here and previous one..?
> 

magic numbers is "1" "0x1b" "0" "0x5a"? 
If so,  I will move this two registers into stf_dphy_init_list[].
And maybe also have some magic numbers below, I will replace them will macro.

>> +
>> +	writel(FIELD_PREP(STF_DPHY_ENABLE_CLK, 1) |
>> +	       FIELD_PREP(STF_DPHY_ENABLE_CLK1, 1) |
>> +	       FIELD_PREP(STF_DPHY_ENABLE_LAN0, 1) |
>> +	       FIELD_PREP(STF_DPHY_ENABLE_LAN1, 1) |
>> +	       FIELD_PREP(STF_DPHY_ENABLE_LAN2, 1) |
>> +	       FIELD_PREP(STF_DPHY_ENABLE_LAN3, 1) |
>> +	       FIELD_PREP(STF_DPHY_GPI_EN, 0) |
>> +	       FIELD_PREP(STF_DPHY_HS_FREQ_CHANGE_CLK, 0) |
>> +	       FIELD_PREP(STF_DPHY_HS_FREQ_CHANGE_CLK1, 0) |
>> +	       FIELD_PREP(STF_DPHY_LANE_SWAP_CLK, info->maps[0]) |
>> +	       FIELD_PREP(STF_DPHY_LANE_SWAP_CLK1, info->maps[5]) |
>> +	       FIELD_PREP(STF_DPHY_LANE_SWAP_LAN0, info->maps[1]) |
>> +	       FIELD_PREP(STF_DPHY_LANE_SWAP_LAN1, info->maps[2]),
>> +	       dphy->regs + STF_DPHY_APBCFGSAIF_SYSCFG(188));
[...]
>> +
>> +	writel(FIELD_PREP(STF_DPHY_PRECOUNTER_IN_LAN3, 7) |
>> +	       FIELD_PREP(STF_DPHY_RX_1C2C_SEL, 0),
>> +	       dphy->regs + STF_DPHY_APBCFGSAIF_SYSCFG(200));
>> +
>> +	return 0;
>> +}
>> +
>> +static int stf_dphy_power_on(struct phy *phy)
>> +{
>> +	struct stf_dphy *dphy = phy_get_drvdata(phy);
>> +	int ret;
>> +
>> +	pm_runtime_get_sync(dphy->dev);
> 
> no error check?
> 

I will replace pm_runtime_get_sync with pm_runtime_resume_and_get to handle error:

	ret = pm_runtime_resume_and_get(dphy->dev);
	if (ret < 0)
		return ret;

>> +
>> +	ret = regulator_enable(dphy->mipi_0p9);
>> +	if (ret)
>> +		return ret;
> 
> should you not drop the pm reference on error here?
> 

I will add pm_runtime_put in this error handle.

Thanks for your comments.