Hi Philipp,
On 3/24/2020 2:49 AM, Philipp Zabel wrote:
> Hi Wesley,
>
> On Mon, Mar 23, 2020 at 01:17:10PM -0700, Wesley Cheng wrote:
>> This adds the SNPS FemtoPHY driver used in QCOM SOCs. There
>> are potentially multiple instances of this UTMI PHY on the
>> SOC, all which can utilize this driver.
>>
>> Signed-off-by: Wesley Cheng <wcheng@xxxxxxxxxxxxxx>
>> ---
>> drivers/phy/qualcomm/Kconfig | 10 ++
>> drivers/phy/qualcomm/Makefile | 1 +
>> drivers/phy/qualcomm/phy-qcom-snps-7nm.c | 294 +++++++++++++++++++++++++++++++
>> 3 files changed, 305 insertions(+)
>> create mode 100644 drivers/phy/qualcomm/phy-qcom-snps-7nm.c
>>
> [...]
>> diff --git a/drivers/phy/qualcomm/phy-qcom-snps-7nm.c b/drivers/phy/qualcomm/phy-qcom-snps-7nm.c
>> new file mode 100644
>> index 0000000..8d4ba53
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-snps-7nm.c
>> @@ -0,0 +1,294 @@
> [...]
>> +static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct qcom_snps_hsphy *hsphy;
>> + struct phy_provider *phy_provider;
>> + struct phy *generic_phy;
>> + struct resource *res;
>> + int ret, i;
>> + int num;
>> +
>> + hsphy = devm_kzalloc(dev, sizeof(*hsphy), GFP_KERNEL);
>> + if (!hsphy)
>> + return -ENOMEM;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + hsphy->base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(hsphy->base))
>> + return PTR_ERR(hsphy->base);
>> +
>> + hsphy->ref_clk = devm_clk_get(dev, "ref");
>> + if (IS_ERR(hsphy->ref_clk)) {
>> + ret = PTR_ERR(hsphy->ref_clk);
>> + if (ret != -EPROBE_DEFER)
>> + dev_err(dev, "failed to get ref clk, %d\n", ret);
>> + return ret;
>> + }
>> +
>> + hsphy->phy_reset = devm_reset_control_get_by_index(&pdev->dev, 0);
>> + if (IS_ERR(hsphy->phy_reset)) {
>> + dev_err(dev, "failed to get phy core reset\n");
>> + return PTR_ERR(hsphy->phy_reset);
>> + }
>
> There is only a single reset specified, so there is no need for
> _by_index. Also please explicitly request exclusive reset control
> for this driver, I suggest:
>
> hsphy->phy_reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>
> If you do want to prepare for future addition of other resets to the
> bindings (but if so, why not specify those right now?), you should add
> a reset-names property and request the reset control by id string
> instead.
>
> regards
> Philipp
>
Thanks for the feedback. Sure, I will move to using
devm_reset_control_get_exclusive, as we won't be having multiple resets
for this particular PHY. Will update a new patch series.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
