[PATCH v3 0/3] Add Intel ComboPhy driver

[PATCH v3 0/3] Add Intel ComboPhy driver

[Dilip Kota]

This patch series adds Intel Combophy driver, respective yaml schemas
and a kernel API fwnode_to_regmap().
ComboPhy driver calls it to get regmap handle through firmware node.

Dilip Kota (3):
  mfd: syscon: Add fwnode_to_regmap
  dt-bindings: phy: Add YAML schemas for Intel Combophy
  phy: intel: Add driver support for Combophy

 .../devicetree/bindings/phy/intel,combo-phy.yaml   | 123 ++++
 drivers/mfd/syscon.c                               |   8 +
 drivers/phy/intel/Kconfig                          |  13 +
 drivers/phy/intel/Makefile                         |   1 +
 drivers/phy/intel/phy-intel-combo.c                | 672 +++++++++++++++++++++
 include/dt-bindings/phy/phy-intel-combophy.h       |  10 +
 include/linux/mfd/syscon.h                         |   6 +
 7 files changed, 833 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
 create mode 100644 drivers/phy/intel/phy-intel-combo.c
 create mode 100644 include/dt-bindings/phy/phy-intel-combophy.h

-- 
2.11.0

[PATCH v3 1/3] mfd: syscon: Add fwnode_to_regmap

[Dilip Kota]

Traverse regmap handle entry from firmware node handle.

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
 drivers/mfd/syscon.c       | 8 ++++++++
 include/linux/mfd/syscon.h | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index e22197c832e8..08064ffc5394 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -178,6 +178,14 @@ struct regmap *device_node_to_regmap(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(device_node_to_regmap);
 
+struct regmap *fwnode_to_regmap(struct fwnode_handle *fwnode)
+{
+	struct device_node *np = to_of_node(fwnode);
+
+	return device_node_get_regmap(np, false);
+}
+EXPORT_SYMBOL_GPL(fwnode_to_regmap);
+
 struct regmap *syscon_node_to_regmap(struct device_node *np)
 {
 	if (!of_device_is_compatible(np, "syscon"))
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index 112dc66262cc..a024dd88bdd4 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -18,6 +18,7 @@ struct device_node;
 
 #ifdef CONFIG_MFD_SYSCON
 extern struct regmap *device_node_to_regmap(struct device_node *np);
+extern struct regmap *fwnode_to_regmap(struct fwnode_handle *fwnode);
 extern struct regmap *syscon_node_to_regmap(struct device_node *np);
 extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s);
 extern struct regmap *syscon_regmap_lookup_by_phandle(
@@ -29,6 +30,11 @@ static inline struct regmap *device_node_to_regmap(struct device_node *np)
 	return ERR_PTR(-ENOTSUPP);
 }
 
+static inline struct regmap *fwnode_to_regmap(struct fwnode_handle *fwnode)
+{
+	return ERR_PTR(-ENOTSUPP);
+}
+
 static inline struct regmap *syscon_node_to_regmap(struct device_node *np)
 {
 	return ERR_PTR(-ENOTSUPP);
-- 
2.11.0

[PATCH v3 2/3] dt-bindings: phy: Add YAML schemas for Intel Combophy

[Dilip Kota]

Combophy subsystem provides PHY support to various
controllers, viz. PCIe, SATA and EMAC.
Adding YAML schemas for the same.

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
Changes on v3:

 Add include/dt-bindings/phy/phy-intel-combphy.h for phy modes.
 Add SoC specific compatible "intel,combophy-lgm".
 Correct the nodename pattern.
 clocks description removed and add maxItems entry.
 Remove "simple-bus" as it expects minimum one address
  cell and size cell in the children node. Call devm_of_platform_populate()
  in the driver to perform "simple-bus" functionality.

Changes on v2:

 Add custom 'select'
 Pass hardware instance entries with phandles and
   remove cell-index and bid entries
 Clock, register address space, are same for the children.
   So move them to parent node.
 Two PHY instances cannot run in different modes,
   so move the phy-mode entry to parent node.
 Add second child entry in the DT example.

 .../devicetree/bindings/phy/intel,combo-phy.yaml   | 123 +++++++++++++++++++++
 include/dt-bindings/phy/phy-intel-combophy.h       |  10 ++
 2 files changed, 133 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
 create mode 100644 include/dt-bindings/phy/phy-intel-combophy.h

diff --git a/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
new file mode 100644
index 000000000000..f9bae37fab17
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/intel,combo-phy.yaml
@@ -0,0 +1,123 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/intel,combo-phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Intel ComboPhy Subsystem
+
+maintainers:
+  - Dilip Kota <eswara.kota@linux.intel.com>
+
+description: |
+  Intel Combophy subsystem supports PHYs for PCIe, EMAC and SATA
+  controllers. A single Combophy provides two PHY instances.
+
+properties:
+  $nodename:
+    pattern: "combophy(@.*|-[0-9a-f])*$"
+
+  compatible:
+    items:
+      - const: intel,combophy-lgm
+      - const: intel,combo-phy
+
+  clocks:
+    maxItems: 1
+
+  reg:
+    items:
+      - description: ComboPhy core registers
+      - description: PCIe app core control registers
+
+  reg-names:
+    items:
+      - const: core
+      - const: app
+
+  resets:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: phy
+      - const: core
+
+  intel,syscfg:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: Chip configuration registers handle and ComboPhy instance id
+
+  intel,hsio:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: HSIO registers handle and ComboPhy instance id on NOC
+
+  intel,aggregation:
+    type: boolean
+    description: |
+      Specify the flag to configure ComboPHY in dual lane mode.
+
+  intel,phy-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Mode of the two phys in ComboPhy.
+      See dt-bindings/phy/phy-intel-combophy.h for values.
+
+patternProperties:
+  "^phy@[0-9]+$":
+    type: object
+
+    properties:
+      compatible:
+        const: intel,phydev
+
+      "#phy-cells":
+        const: 0
+
+      resets:
+        description: |
+          reset handle according to the PHY mode.
+          See ../reset/reset.txt for details.
+
+    required:
+      - compatible
+      - "#phy-cells"
+
+required:
+  - compatible
+  - clocks
+  - reg
+  - reg-names
+  - intel,syscfg
+  - intel,hsio
+  - intel,phy-mode
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/phy/phy-intel-combophy.h>
+    combophy@d0a00000 {
+        compatible = "intel,combophy-lgm", "intel,combo-phy";
+        clocks = <&cgu0 1>;
+        reg = <0xd0a00000 0x40000>,
+              <0xd0a40000 0x1000>;
+        reg-names = "core", "app";
+        resets = <&rcu0 0x50 6>,
+                 <&rcu0 0x50 17>;
+        reset-names = "phy", "core";
+        intel,syscfg = <&sysconf 0>;
+        intel,hsio = <&hsiol 0>;
+        intel,phy-mode = <COMBO_PHY_PCIE>;
+
+        phy@0 {
+            compatible = "intel,phydev";
+            #phy-cells = <0>;
+            resets = <&rcu0 0x50 23>;
+        };
+
+        phy@1 {
+            compatible = "intel,phydev";
+            #phy-cells = <0>;
+            resets = <&rcu0 0x50 24>;
+        };
+    };
diff --git a/include/dt-bindings/phy/phy-intel-combophy.h b/include/dt-bindings/phy/phy-intel-combophy.h
new file mode 100644
index 000000000000..bd7f6377f4ef
--- /dev/null
+++ b/include/dt-bindings/phy/phy-intel-combophy.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _DT_BINDINGS_INTEL_COMBOPHY
+#define _DT_BINDINGS_INTEL_COMBOPHY
+
+#define COMBO_PHY_PCIE	0
+#define COMBO_PHY_XPCS	1
+#define COMBO_PHY_SATA	2
+
+#endif /* _DT_BINDINGS_INTEL_COMBOPHY*/
-- 
2.11.0

[PATCH v3 3/3] phy: intel: Add driver support for Combophy

[Dilip Kota]

Combophy subsystem provides PHYs for various
controllers like PCIe, SATA and EMAC.

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
Changes on v3:
 Use fwnode_to_regmap()
 Call devm_of_platform_populate() to populate child nodes
 Add SoC specific compatible "intel,combophy-lgm"
 Remove intel_iphy_names
 Remove struct phy entry in struct intel_cbphy_iphy
 Imporve if conditions logic
 Add description for enums
 Remove default case in switch {} intel_cbphy_set_mode() as it
  never happens.
 Use mutex_lock to synchronise ComboPhy initialization across
  two phys.
 Change init_cnt to u32 datatype as it is within mutex lock.
 Correct error handling of
  fwnode_property_read_u32_array(fwnode, "intel,phy-mode", ...)

 drivers/phy/intel/Kconfig           |  13 +
 drivers/phy/intel/Makefile          |   1 +
 drivers/phy/intel/phy-intel-combo.c | 672 ++++++++++++++++++++++++++++++++++++
 3 files changed, 686 insertions(+)
 create mode 100644 drivers/phy/intel/phy-intel-combo.c

diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig
index 4ea6a8897cd7..bbd0792f5b43 100644
--- a/drivers/phy/intel/Kconfig
+++ b/drivers/phy/intel/Kconfig
@@ -2,6 +2,19 @@
 #
 # Phy drivers for Intel Lightning Mountain(LGM) platform
 #
+config PHY_INTEL_COMBO
+	bool "Intel Combo PHY driver"
+	depends on OF && HAS_IOMEM && (X86 || COMPILE_TEST)
+	select MFD_SYSCON
+	select GENERIC_PHY
+	select REGMAP
+	help
+	  Enable this to support Intel combo phy.
+
+	  This driver configures combo phy subsystem on Intel gateway
+	  chipsets which provides PHYs for various controllers, EMAC,
+	  SATA and PCIe.
+
 config PHY_INTEL_EMMC
 	tristate "Intel EMMC PHY driver"
 	select GENERIC_PHY
diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile
index 6b876a75599d..233d530dadde 100644
--- a/drivers/phy/intel/Makefile
+++ b/drivers/phy/intel/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_PHY_INTEL_COMBO)		+= phy-intel-combo.o
 obj-$(CONFIG_PHY_INTEL_EMMC)            += phy-intel-emmc.o
diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c
new file mode 100644
index 000000000000..b75f60834eec
--- /dev/null
+++ b/drivers/phy/intel/phy-intel-combo.c
@@ -0,0 +1,672 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Combo-PHY driver
+ *
+ * Copyright (C) 2019 Intel Corporation.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_platform.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#define PCIE_PHY_GEN_CTRL	0x00
+#define PCIE_PHY_CLK_PAD	BIT(17)
+#define PCIE_PHY_MPLLA_CTRL	0x10
+#define PCIE_PHY_MPLLB_CTRL	0x14
+
+#define PCS_XF_ATE_OVRD_IN_2	0x3008
+#define ADAPT_REQ_MSK		0x30
+#define PCS_XF_RX_ADAPT_ACK	0x3010
+#define RX_ADAPT_ACK_BIT	BIT(0)
+
+#define CR_ADDR(addr, lane)	(((addr) + (lane) * 0x100) << 2)
+#define REG_COMBO_MODE(x)	((x) * 0x200)
+#define REG_CLK_DISABLE(x)	((x) * 0x200 + 0x124)
+
+#define COMBO_PHY_ID(x)		((x)->parent->id)
+#define PHY_ID(x)		((x)->id)
+
+#define CLK_100MHZ		100000000
+#define CLK_156_25MHZ		156250000
+
+static const unsigned long intel_iphy_clk_rates[] = {
+	CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ
+};
+
+enum {
+	PHY_0,
+	PHY_1,
+	PHY_MAX_NUM,
+};
+
+/*
+ * Clock register bitfields to enable clocks
+ * for ComboPhy according to the mode
+ */
+enum intel_phy_mode {
+	PHY_PCIE_MODE = 0,
+	PHY_XPCS_MODE,
+	PHY_SATA_MODE,
+	PHY_MAX_MODE,
+};
+
+/* ComboPhy mode register values */
+enum intel_combo_mode {
+	PCIE0_PCIE1_MODE = 0,
+	PCIE_DL_MODE,
+	RXAUI_MODE,
+	XPCS0_XPCS1_MODE,
+	SATA0_SATA1_MODE,
+	COMBO_PHY_MODE_MAX,
+};
+
+enum aggregated_mode {
+	PHY_SL_MODE,
+	PHY_DL_MODE,
+};
+
+struct intel_combo_phy;
+
+struct intel_cbphy_iphy {
+	struct device		*dev;
+	bool			enable;
+	struct intel_combo_phy	*parent;
+	struct reset_control	*app_rst;
+	u32			id;
+};
+
+struct intel_combo_phy {
+	struct device		*dev;
+	struct clk		*core_clk;
+	unsigned long		clk_rate;
+	void __iomem		*app_base;
+	void __iomem		*cr_base;
+	struct regmap		*syscfg;
+	struct regmap		*hsiocfg;
+	u32			id;
+	u32			bid;
+	struct reset_control	*phy_rst;
+	struct reset_control	*core_rst;
+	struct intel_cbphy_iphy	iphy[PHY_MAX_NUM];
+	enum intel_phy_mode	phy_mode;
+	enum aggregated_mode	aggr_mode;
+	u32			init_cnt;
+	struct mutex		lock;
+};
+
+static int intel_cbphy_iphy_enable(struct intel_cbphy_iphy *iphy, bool set)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	u32 mask = BIT(cbphy->phy_mode * 2 + iphy->id);
+	u32 val;
+
+	/* Register: 0 is enable, 1 is disable */
+	val = set ? 0 : mask;
+
+	return regmap_update_bits(cbphy->hsiocfg, REG_CLK_DISABLE(cbphy->bid),
+				  mask, val);
+}
+
+static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	u32 mask = BIT(cbphy->id * 2 + iphy->id);
+	const u32 pad_dis_cfg_off = 0x174;
+	u32 val;
+
+	/* Register: 0 is enable, 1 is disable */
+	val = set ? 0 : mask;
+
+	return regmap_update_bits(cbphy->syscfg, pad_dis_cfg_off, mask, val);
+}
+
+static inline void combo_phy_w32_off_mask(void __iomem *base, unsigned int reg,
+					  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);
+}
+
+static int intel_cbphy_iphy_cfg(struct intel_cbphy_iphy *iphy,
+				int (*phy_cfg)(struct intel_cbphy_iphy *))
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	struct intel_cbphy_iphy *sphy;
+	int ret;
+
+	ret = phy_cfg(iphy);
+	if (ret)
+		return ret;
+
+	if (cbphy->aggr_mode != PHY_DL_MODE)
+		return 0;
+
+	sphy = &cbphy->iphy[PHY_1];
+	ret = phy_cfg(sphy);
+
+	return ret;
+}
+
+static int intel_cbphy_pcie_en_pad_refclk(struct intel_cbphy_iphy *iphy)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	int ret;
+
+	ret = intel_cbphy_pcie_refclk_cfg(iphy, true);
+	if (ret) {
+		dev_err(iphy->dev, "Failed to enable PCIe pad refclk\n");
+		return ret;
+	}
+
+	if (cbphy->init_cnt)
+		return 0;
+
+	combo_phy_w32_off_mask(cbphy->app_base, PCIE_PHY_GEN_CTRL,
+			       PCIE_PHY_CLK_PAD, 0);
+	/*
+	 * No way to identify gen1/2/3/4 for mppla and mpplb, just delay
+	 * for stable plla(gen1/gen2) or pllb(gen3/gen4)
+	 */
+	usleep_range(50, 100);
+
+	return 0;
+}
+
+static int intel_cbphy_pcie_dis_pad_refclk(struct intel_cbphy_iphy *iphy)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	int ret;
+
+	ret = intel_cbphy_pcie_refclk_cfg(iphy, false);
+	if (ret) {
+		dev_err(iphy->dev, "Failed to disable PCIe pad refclk\n");
+		return ret;
+	}
+
+	if (cbphy->init_cnt)
+		return 0;
+
+	combo_phy_w32_off_mask(cbphy->app_base, PCIE_PHY_GEN_CTRL,
+			       PCIE_PHY_CLK_PAD, 1);
+
+	return 0;
+}
+
+static int intel_cbphy_set_mode(struct intel_combo_phy *cbphy)
+{
+	enum intel_combo_mode cb_mode = COMBO_PHY_MODE_MAX;
+	enum aggregated_mode aggr = cbphy->aggr_mode;
+	struct device *dev = cbphy->dev;
+	enum intel_phy_mode mode;
+	int ret;
+
+	mode = cbphy->phy_mode;
+
+	switch (mode) {
+	case PHY_PCIE_MODE:
+		cb_mode = (aggr == PHY_DL_MODE) ? PCIE_DL_MODE : PCIE0_PCIE1_MODE;
+		break;
+
+	case PHY_XPCS_MODE:
+		cb_mode = (aggr == PHY_DL_MODE) ? RXAUI_MODE : XPCS0_XPCS1_MODE;
+		break;
+
+	case PHY_SATA_MODE:
+		if (aggr == PHY_DL_MODE) {
+			dev_err(dev, "Mode:%u not support dual lane!\n", mode);
+			return -EINVAL;
+		}
+
+		cb_mode = SATA0_SATA1_MODE;
+		break;
+	}
+
+	ret = regmap_write(cbphy->hsiocfg, REG_COMBO_MODE(cbphy->bid), cb_mode);
+	if (ret)
+		dev_err(dev, "Failed to set ComboPhy mode: %d\n", ret);
+
+	return ret;
+}
+
+static void intel_cbphy_rst_assert(struct intel_combo_phy *cbphy)
+{
+	reset_control_assert(cbphy->core_rst);
+	reset_control_assert(cbphy->phy_rst);
+}
+
+static void intel_cbphy_rst_deassert(struct intel_combo_phy *cbphy)
+{
+	reset_control_deassert(cbphy->core_rst);
+	reset_control_deassert(cbphy->phy_rst);
+	/* Delay to ensure reset process is done */
+	usleep_range(10, 20);
+}
+
+static int intel_cbphy_iphy_power_on(struct intel_cbphy_iphy *iphy)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	struct device *dev = iphy->dev;
+	int ret;
+
+	if (cbphy->init_cnt) {
+		ret = clk_prepare_enable(cbphy->core_clk);
+		if (ret) {
+			dev_err(cbphy->dev, "Clock enable failed!\n");
+			return ret;
+		}
+
+		ret = clk_set_rate(cbphy->core_clk, cbphy->clk_rate);
+		if (ret) {
+			dev_err(cbphy->dev, "Clock freq set to %lu failed!\n",
+				cbphy->clk_rate);
+			goto clk_err;
+		}
+
+		intel_cbphy_rst_assert(cbphy);
+		intel_cbphy_rst_deassert(cbphy);
+		ret = intel_cbphy_set_mode(cbphy);
+		if (ret)
+			goto clk_err;
+	}
+
+	ret = intel_cbphy_iphy_enable(iphy, true);
+	if (ret) {
+		dev_err(dev, "Failed enabling Phy core\n");
+		goto clk_err;
+	}
+
+	ret = reset_control_deassert(iphy->app_rst);
+	if (ret) {
+		dev_err(dev, "PHY(%u:%u) phy deassert failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy));
+		goto clk_err;
+	}
+
+	/* Delay to ensure reset process is done */
+	udelay(1);
+
+	return 0;
+
+clk_err:
+	clk_disable_unprepare(cbphy->core_clk);
+
+	return ret;
+}
+
+static int intel_cbphy_iphy_power_off(struct intel_cbphy_iphy *iphy)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	struct device *dev = iphy->dev;
+	int ret;
+
+	ret = reset_control_assert(iphy->app_rst);
+	if (ret) {
+		dev_err(dev, "PHY(%u:%u) core assert failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy));
+		return ret;
+	}
+
+	ret = intel_cbphy_iphy_enable(iphy, false);
+	if (ret) {
+		dev_err(dev, "Failed disabling Phy core\n");
+		return ret;
+	}
+
+	if (!cbphy->init_cnt) {
+		clk_disable_unprepare(cbphy->core_clk);
+		intel_cbphy_rst_assert(cbphy);
+	}
+
+	return 0;
+}
+
+static int intel_cbphy_init(struct phy *phy)
+{
+	struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy);
+	struct intel_combo_phy *cbphy = iphy->parent;
+	int ret;
+
+	mutex_lock(&cbphy->lock);
+	ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_on);
+	if (ret)
+		goto err;
+
+	if (cbphy->phy_mode == PHY_PCIE_MODE) {
+		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_pcie_en_pad_refclk);
+		if (ret)
+			goto err;
+	}
+
+	cbphy->init_cnt++;
+
+err:
+	mutex_unlock(&cbphy->lock);
+
+	return ret;
+}
+
+static int intel_cbphy_exit(struct phy *phy)
+{
+	struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy);
+	struct intel_combo_phy *cbphy = iphy->parent;
+	int ret;
+
+	mutex_lock(&cbphy->lock);
+	cbphy->init_cnt--;
+	if (cbphy->phy_mode == PHY_PCIE_MODE) {
+		ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_pcie_dis_pad_refclk);
+		if (ret)
+			goto err;
+	}
+
+	ret = intel_cbphy_iphy_cfg(iphy, intel_cbphy_iphy_power_off);
+
+err:
+	mutex_unlock(&cbphy->lock);
+
+	return ret;
+}
+
+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;
+	struct device *dev = iphy->dev;
+	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(dev, "RX Adaptation failed!\n");
+	else
+		dev_info(dev, "RX Adaptation success!\n");
+
+	/* Stop RX adaptation */
+	combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id),
+			       ADAPT_REQ_MSK, 0);
+
+	return ret;
+}
+
+static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,
+				     struct fwnode_handle *fwnode, int idx)
+{
+	struct intel_cbphy_iphy *iphy = &cbphy->iphy[idx];
+	struct device *dev;
+	int ret;
+
+	dev = get_dev_from_fwnode(fwnode);
+	iphy->parent = cbphy;
+	iphy->dev = dev;
+	iphy->id = idx;
+
+	iphy->app_rst = devm_reset_control_get_optional(dev, NULL);
+	if (IS_ERR(iphy->app_rst)) {
+		ret = PTR_ERR(iphy->app_rst);
+		if (ret != -EPROBE_DEFER) {
+			dev_err(dev, "PHY[%u:%u] Get reset ctrl Err!\n",
+				COMBO_PHY_ID(iphy), PHY_ID(iphy));
+		}
+
+		return ret;
+	}
+
+	iphy->enable = true;
+
+	return 0;
+}
+
+static int intel_cbphy_dt_sanity_check(struct intel_combo_phy *cbphy)
+{
+	struct intel_cbphy_iphy *iphy0, *iphy1;
+
+	iphy0 = &cbphy->iphy[PHY_0];
+	iphy1 = &cbphy->iphy[PHY_1];
+
+	if (!iphy0->enable && !iphy1->enable) {
+		dev_err(cbphy->dev, "CBPHY%u both PHY disabled!\n", cbphy->id);
+		return -EINVAL;
+	}
+
+	if (cbphy->aggr_mode == PHY_DL_MODE &&
+	    (!iphy0->enable || !iphy1->enable)) {
+		dev_err(cbphy->dev,
+			"Dual lane mode but lane0: %s, lane1: %s\n",
+			iphy0->enable ? "on" : "off",
+			iphy1->enable ? "on" : "off");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int intel_cbphy_dt_parse(struct intel_combo_phy *cbphy)
+{
+	struct fwnode_reference_args ref;
+	struct device *dev = cbphy->dev;
+	struct fwnode_handle *fwnode;
+	struct platform_device *pdev;
+	int i, ret;
+	u32 prop;
+
+	cbphy->core_clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(cbphy->core_clk)) {
+		ret = PTR_ERR(cbphy->core_clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "CBPHY get clk failed:%d!\n", ret);
+		return ret;
+	}
+
+	cbphy->phy_rst = devm_reset_control_get_optional(dev, "phy");
+	if (IS_ERR(cbphy->phy_rst)) {
+		ret = PTR_ERR(cbphy->phy_rst);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Get phy reset err: %d!\n", ret);
+		return ret;
+	}
+
+	cbphy->core_rst = devm_reset_control_get_optional(dev, "core");
+	if (IS_ERR(cbphy->core_rst)) {
+		ret = PTR_ERR(cbphy->core_rst);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Get combo reset err: %d!\n", ret);
+		return ret;
+	}
+
+	pdev = to_platform_device(dev);
+	cbphy->app_base = devm_platform_ioremap_resource_byname(pdev, "app");
+	if (IS_ERR(cbphy->app_base))
+		return PTR_ERR(cbphy->app_base);
+
+	cbphy->cr_base = devm_platform_ioremap_resource_byname(pdev, "core");
+	if (IS_ERR(cbphy->cr_base))
+		return PTR_ERR(cbphy->cr_base);
+
+	fwnode = dev_fwnode(dev);
+	ret = fwnode_property_get_reference_args(fwnode, "intel,syscfg", NULL,
+						 1, 0, &ref);
+	if (ret < 0)
+		return ret;
+
+	cbphy->id = ref.args[0];
+	cbphy->syscfg = fwnode_to_regmap(ref.fwnode);
+	fwnode_handle_put(ref.fwnode);
+
+	ret = fwnode_property_get_reference_args(fwnode, "intel,hsio", NULL, 1,
+						 0, &ref);
+	if (ret < 0)
+		return ret;
+
+	cbphy->bid = ref.args[0];
+	cbphy->hsiocfg = fwnode_to_regmap(ref.fwnode);
+	fwnode_handle_put(ref.fwnode);
+
+	ret = fwnode_property_read_u32_array(fwnode, "intel,phy-mode", &prop, 1);
+	if (ret)
+		return ret;
+
+	if (prop >= PHY_MAX_MODE) {
+		dev_err(dev, "Invalid phy mode: %u\n", prop);
+		return -EINVAL;
+	}
+
+	cbphy->phy_mode = prop;
+	cbphy->clk_rate = intel_iphy_clk_rates[cbphy->phy_mode];
+
+	if (fwnode_property_present(fwnode, "intel,aggregation"))
+		cbphy->aggr_mode = PHY_DL_MODE;
+	else
+		cbphy->aggr_mode = PHY_SL_MODE;
+
+	ret = devm_of_platform_populate(dev);
+	if (ret) {
+		dev_err(dev, "Failed to populate child nodes: %d\n", ret);
+		return ret;
+	}
+
+	i = 0;
+	fwnode_for_each_available_child_node(dev_fwnode(dev), fwnode) {
+		if (i >= PHY_MAX_NUM) {
+			fwnode_handle_put(fwnode);
+			dev_err(dev, "Error: DT child number larger than %d\n",
+				PHY_MAX_NUM);
+			return -EINVAL;
+		}
+
+		ret = intel_cbphy_iphy_dt_parse(cbphy, fwnode, i);
+		if (ret) {
+			fwnode_handle_put(fwnode);
+			return ret;
+		}
+
+		i++;
+	}
+
+	return intel_cbphy_dt_sanity_check(cbphy);
+}
+
+static const struct phy_ops intel_cbphy_ops = {
+	.init =		intel_cbphy_init,
+	.exit =		intel_cbphy_exit,
+	.calibrate =	intel_cbphy_calibrate,
+	.owner =	THIS_MODULE,
+};
+
+static int intel_cbphy_iphy_create(struct intel_cbphy_iphy *iphy)
+{
+	struct intel_combo_phy *cbphy = iphy->parent;
+	struct phy_provider *phy_provider;
+	struct device *dev = iphy->dev;
+	struct phy *phy;
+
+	/* In dual mode skip phy creation for the second phy */
+	if (cbphy->aggr_mode == PHY_DL_MODE && iphy->id)
+		return 0;
+
+	phy = devm_phy_create(dev, NULL, &intel_cbphy_ops);
+	if (IS_ERR(phy)) {
+		dev_err(dev, "PHY[%u:%u]: create PHY instance failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy));
+		return PTR_ERR(phy);
+	}
+
+	phy_set_drvdata(phy, iphy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+	if (IS_ERR(phy_provider)) {
+		dev_err(dev, "PHY[%u:%u]: register phy provider failed!\n",
+			COMBO_PHY_ID(iphy), PHY_ID(iphy));
+	}
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static int intel_cbphy_create(struct intel_combo_phy *cbphy)
+{
+	struct intel_cbphy_iphy *iphy;
+	int i, ret;
+
+	for (i = 0; i < PHY_MAX_NUM; i++) {
+		iphy = &cbphy->iphy[i];
+		if (iphy->enable) {
+			ret = intel_cbphy_iphy_create(iphy);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int intel_cbphy_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_combo_phy *cbphy;
+	int ret;
+
+	cbphy = devm_kzalloc(dev, sizeof(*cbphy), GFP_KERNEL);
+	if (!cbphy)
+		return -ENOMEM;
+
+	cbphy->dev = dev;
+	cbphy->init_cnt = 0;
+	mutex_init(&cbphy->lock);
+	platform_set_drvdata(pdev, cbphy);
+	ret = intel_cbphy_dt_parse(cbphy);
+	if (ret)
+		return ret;
+
+	return intel_cbphy_create(cbphy);
+}
+
+static int intel_cbphy_remove(struct platform_device *pdev)
+{
+	struct intel_combo_phy *cbphy = platform_get_drvdata(pdev);
+
+	intel_cbphy_rst_assert(cbphy);
+	clk_disable_unprepare(cbphy->core_clk);
+	return 0;
+}
+
+static const struct of_device_id of_intel_cbphy_match[] = {
+	{ .compatible = "intel,combo-phy" },
+	{ .compatible = "intel,combophy-lgm" },
+	{}
+};
+
+static struct platform_driver intel_cbphy_driver = {
+	.probe = intel_cbphy_probe,
+	.remove = intel_cbphy_remove,
+	.driver = {
+		.name = "intel-combo-phy",
+		.of_match_table = of_intel_cbphy_match,
+	}
+};
+
+module_platform_driver(intel_cbphy_driver);
+
+MODULE_DESCRIPTION("Intel Combo-phy driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy

[Andy Shevchenko]

On Wed, Feb 26, 2020 at 06:09:53PM +0800, Dilip Kota wrote:
> Combophy subsystem provides PHYs for various
> controllers like PCIe, SATA and EMAC.

Thanks for an update, my comments below.

...

> +config PHY_INTEL_COMBO
> +	bool "Intel Combo PHY driver"

> +	depends on OF && HAS_IOMEM && (X86 || COMPILE_TEST)

I guess it would be better to have like this:

	depends on X86 || COMPILE_TEST
	depends on OF && HAS_IOMEM

But do you still have a dependency to OF?

> +	select MFD_SYSCON
> +	select GENERIC_PHY
> +	select REGMAP

...

> + * Copyright (C) 2019 Intel Corporation.

2019-2020

...

> +static const unsigned long intel_iphy_clk_rates[] = {
> +	CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ

It's good to have comma at the end since this might potentially be extended in
the future.

> +};
> +
> +enum {
> +	PHY_0,
> +	PHY_1,

> +	PHY_MAX_NUM,

But here we don't need it since it's a terminator line.
Ditto for the rest of enumerators with a terminator / max entry.

> +};

...

> +/*
> + * Clock register bitfields to enable clocks
> + * for ComboPhy according to the mode

For multi-line comments use proper English punctuation.

> + */

...

> +static int intel_cbphy_pcie_refclk_cfg(struct intel_cbphy_iphy *iphy, bool set)
> +{
> +	struct intel_combo_phy *cbphy = iphy->parent;
> +	u32 mask = BIT(cbphy->id * 2 + iphy->id);

> +	const u32 pad_dis_cfg_off = 0x174;

This has to be defined in the top.

> +	u32 val;
> +
> +	/* Register: 0 is enable, 1 is disable */
> +	val = set ? 0 : mask;
> +
> +	return regmap_update_bits(cbphy->syscfg, pad_dis_cfg_off, mask, val);
> +}

...

> +	ret = phy_cfg(sphy);
> +
> +	return ret;

	return phy_cfg();

...

> +	/*
> +	 * No way to identify gen1/2/3/4 for mppla and mpplb, just delay
> +	 * for stable plla(gen1/gen2) or pllb(gen3/gen4)
> +	 */

Use proper abbreviation rules, i.e. capitalize appropriately. (I think at least
PLL is quite common way to do it, the rest depends to the documentation / your
intentions).


...

> +	ret = reset_control_assert(iphy->app_rst);
> +	if (ret) {
> +		dev_err(dev, "PHY(%u:%u) core assert failed!\n",
> +			COMBO_PHY_ID(iphy), PHY_ID(iphy));
> +		return ret;
> +	}
> +
> +	ret = intel_cbphy_iphy_enable(iphy, false);
> +	if (ret) {
> +		dev_err(dev, "Failed disabling Phy core\n");

Phy -> PHY for sake of the consistency

> +		return ret;
> +	}

...

> +	/* 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(dev, "RX Adaptation failed!\n");
> +	else

> +		dev_info(dev, "RX Adaptation success!\n");

Sounds more likely like dev_dbg().

...

> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,

dt -> fwnode
Ditto for other similar function names.

> +				     struct fwnode_handle *fwnode, int idx)
> +{

> +	dev = get_dev_from_fwnode(fwnode);

I don't see where you drop reference count to the struct device object.

> +}

...

> +	if (!iphy0->enable && !iphy1->enable) {

	if (!(iphy0->enable || iphy1->enable)) {

?

> +		dev_err(cbphy->dev, "CBPHY%u both PHY disabled!\n", cbphy->id);

> +		return -EINVAL;

-ENODEV ?

> +	}
> +
> +	if (cbphy->aggr_mode == PHY_DL_MODE &&
> +	    (!iphy0->enable || !iphy1->enable)) {

	if (cbphy->aggr_mode == PHY_DL_MODE && !(iphy0->enable && iphy1->enable)) {

?

> +		dev_err(cbphy->dev,
> +			"Dual lane mode but lane0: %s, lane1: %s\n",
> +			iphy0->enable ? "on" : "off",
> +			iphy1->enable ? "on" : "off");
> +		return -EINVAL;
> +	}

...

> +	struct fwnode_reference_args ref;
> +	struct device *dev = cbphy->dev;
> +	struct fwnode_handle *fwnode;
> +	struct platform_device *pdev;
> +	int i, ret;
> +	u32 prop;

I guess the following would be better:

	struct device *dev = cbphy->dev;
	struct platform_device *pdev = to_platform_device(dev);
	struct fwnode_handle *fwnode = dev_fwnode(dev);
	struct fwnode_reference_args ref;
	int i, ret;
	u32 prop;

> +	pdev = to_platform_device(dev);

See above.

> +	fwnode = dev_fwnode(dev);

See above.

> +	ret = fwnode_property_read_u32_array(fwnode, "intel,phy-mode", &prop, 1);
> +	if (ret)
> +		return ret;
> +
> +	if (prop >= PHY_MAX_MODE) {
> +		dev_err(dev, "Invalid phy mode: %u\n", prop);
> +		return -EINVAL;
> +	}
> +
> +	cbphy->phy_mode = prop;

I don't see why you need temporary variable at all?


...

> +	i = 0;
> +	fwnode_for_each_available_child_node(dev_fwnode(dev), fwnode) {

> +		if (i >= PHY_MAX_NUM) {
> +			fwnode_handle_put(fwnode);
> +			dev_err(dev, "Error: DT child number larger than %d\n",
> +				PHY_MAX_NUM);
> +			return -EINVAL;
> +		}

Logically this part is better to put after i++; line...

> +		ret = intel_cbphy_iphy_dt_parse(cbphy, fwnode, i);
> +		if (ret) {
> +			fwnode_handle_put(fwnode);
> +			return ret;
> +		}
> +

> +		i++;

...here.

> +	}
> +
> +	return intel_cbphy_dt_sanity_check(cbphy);
> +}

...

> +	.init =		intel_cbphy_init,
> +	.exit =		intel_cbphy_exit,
> +	.calibrate =	intel_cbphy_calibrate,
> +	.owner =	THIS_MODULE,

Usual way is to format by =, i.e.

	.init		= intel_cbphy_init,
	.exit		= intel_cbphy_exit,
	.calibrate	= intel_cbphy_calibrate,
	.owner		= THIS_MODULE,

...

> +	for (i = 0; i < PHY_MAX_NUM; i++) {
> +		iphy = &cbphy->iphy[i];

> +		if (iphy->enable) {

		if (!iphy->enable)
			continue;

?

> +			ret = intel_cbphy_iphy_create(iphy);
> +			if (ret)
> +				return ret;
> +		}
> +	}

...

> +	platform_set_drvdata(pdev, cbphy);

I guess it makes sense a bit later to do...

> +	ret = intel_cbphy_dt_parse(cbphy);
> +	if (ret)
> +		return ret;

...here?

> +
> +	return intel_cbphy_create(cbphy);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy

[Andy Shevchenko]

On Wed, Feb 26, 2020 at 04:41:47PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 26, 2020 at 06:09:53PM +0800, Dilip Kota wrote:

> > +	i = 0;
> > +	fwnode_for_each_available_child_node(dev_fwnode(dev), fwnode) {
> 
> > +		if (i >= PHY_MAX_NUM) {
> > +			fwnode_handle_put(fwnode);
> > +			dev_err(dev, "Error: DT child number larger than %d\n",
> > +				PHY_MAX_NUM);
> > +			return -EINVAL;
> > +		}
> 
> Logically this part is better to put after i++; line...

Ah, dismiss this, I forgot the fwnode_handle_put() part along with amount of
accessible children.

> > +		ret = intel_cbphy_iphy_dt_parse(cbphy, fwnode, i);
> > +		if (ret) {
> > +			fwnode_handle_put(fwnode);
> > +			return ret;
> > +		}
> > +
> 
> > +		i++;
> 
> ...here.
> 
> > +	}
> > +
> > +	return intel_cbphy_dt_sanity_check(cbphy);
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy

[Dilip Kota]

Thanks Andy for reviewing and giving the inputs.
I will update them as per your comments, but for couple of cases of i 
have a different opinion. Please check and give your inputs.

On 2/26/2020 10:41 PM, Andy Shevchenko wrote:
> On Wed, Feb 26, 2020 at 06:09:53PM +0800, Dilip Kota wrote:
>> Combophy subsystem provides PHYs for various
>> controllers like PCIe, SATA and EMAC.
> Thanks for an update, my comments below.
>
> ...
>
>> +config PHY_INTEL_COMBO
>> +	bool "Intel Combo PHY driver"
>> +	depends on OF && HAS_IOMEM && (X86 || COMPILE_TEST)
> I guess it would be better to have like this:
>
> 	depends on X86 || COMPILE_TEST
> 	depends on OF && HAS_IOMEM
>
> But do you still have a dependency to OF?
Yes, OF is not required. I will remove it.
>
>> +	select MFD_SYSCON
>> +	select GENERIC_PHY
>> +	select REGMAP
> ...
>
>> + * Copyright (C) 2019 Intel Corporation.
> 2019-2020
My bad. I will update it.
>
> ...
>
...
>> +};
>> +
>> +enum {
>> +	PHY_0,
>> +	PHY_1,
>> +	PHY_MAX_NUM,
> But here we don't need it since it's a terminator line.
> Ditto for the rest of enumerators with a terminator / max entry.

Sure i will remove them.

To be meaningful, i will remove the max entry for the enums representing 
the value of register bitfields.

...
> ...
>
>> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,
> dt -> fwnode
> Ditto for other similar function names.
Sure, it looks appropriate for intel_cbphy_iphy_dt_parse() -> 
intel_cbphy_iphy_fwnode_parse().
Whereas for intel_cbphy_dt_parse() i will keep it unchanged, because it 
is calling devm_*, devm_platform_*, fwnode_* APIs to traverse dt node.
>
>> +				     struct fwnode_handle *fwnode, int idx)
>> +{
>> +	dev = get_dev_from_fwnode(fwnode);
> I don't see where you drop reference count to the struct device object.

I will add it. Thanks for pointing it.

...

> ...
>
>> +	struct fwnode_reference_args ref;
>> +	struct device *dev = cbphy->dev;
>> +	struct fwnode_handle *fwnode;
>> +	struct platform_device *pdev;
>> +	int i, ret;
>> +	u32 prop;
> I guess the following would be better:
In the v2 patch, for int i = 0 you mentioned to do initialization at the 
user, instead of doing at declaration.
So i followed the same for "pdev" and "fwnode" which are being used 
after few lines of the code . It looked good in the perspective of code 
readability.
>
> 	struct device *dev = cbphy->dev;
> 	struct platform_device *pdev = to_platform_device(dev);
> 	struct fwnode_handle *fwnode = dev_fwnode(dev);
> 	struct fwnode_reference_args ref;
> 	int i, ret;
> 	u32 prop;
>
>> +	pdev = to_platform_device(dev);
> See above.
>
>> +	fwnode = dev_fwnode(dev);
> See above.
>
>
Regards,
Dilip

Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy

[Andy Shevchenko]

On Thu, Feb 27, 2020 at 9:54 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
> Thanks Andy for reviewing and giving the inputs.
> I will update them as per your comments, but for couple of cases of i
> have a different opinion. Please check and give your inputs.

...

> >> +enum {
> >> +    PHY_0,
> >> +    PHY_1,
> >> +    PHY_MAX_NUM,
> > But here we don't need it since it's a terminator line.
> > Ditto for the rest of enumerators with a terminator / max entry.
>
> Sure i will remove them.
>
> To be meaningful, i will remove the max entry for the enums representing
> the value of register bitfields.

It will work.

...

> >> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,
> > dt -> fwnode
> > Ditto for other similar function names.
> Sure, it looks appropriate for intel_cbphy_iphy_dt_parse() ->
> intel_cbphy_iphy_fwnode_parse().
> Whereas for intel_cbphy_dt_parse() i will keep it unchanged, because it
> is calling devm_*, devm_platform_*, fwnode_* APIs to traverse dt node.

How do you know that it will be DT node?
I can't say it from the function parameters: Is any of them takes of_node?

> >> +                                 struct fwnode_handle *fwnode, int idx)

...

> >> +    struct fwnode_reference_args ref;
> >> +    struct device *dev = cbphy->dev;
> >> +    struct fwnode_handle *fwnode;
> >> +    struct platform_device *pdev;
> >> +    int i, ret;
> >> +    u32 prop;
> > I guess the following would be better:
> In the v2 patch, for int i = 0 you mentioned to do initialization at the
> user, instead of doing at declaration.

> So i followed the same for "pdev" and "fwnode" which are being used
> after few lines of the code . It looked good in the perspective of code
> readability.

No, it is different. For the loop counter is better to have closer to
the loop, for the more global thingy like platform device it makes it
actually harder to find.
When you do assignments you have to think about the variable meaning
and scope. Scope is different for loop counter versus the mentioned
rest.

> >       struct device *dev = cbphy->dev;
> >       struct platform_device *pdev = to_platform_device(dev);
> >       struct fwnode_handle *fwnode = dev_fwnode(dev);
> >       struct fwnode_reference_args ref;
> >       int i, ret;
> >       u32 prop;
> >
> >> +    pdev = to_platform_device(dev);
> > See above.
> >
> >> +    fwnode = dev_fwnode(dev);
> > See above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/3] phy: intel: Add driver support for Combophy

[Dilip Kota]

On 2/27/2020 5:43 PM, Andy Shevchenko wrote:
> On Thu, Feb 27, 2020 at 9:54 AM Dilip Kota <eswara.kota@linux.intel.com> wrote:
>
...
>>>> +static int intel_cbphy_iphy_dt_parse(struct intel_combo_phy *cbphy,
>>> dt -> fwnode
>>> Ditto for other similar function names.
>> Sure, it looks appropriate for intel_cbphy_iphy_dt_parse() ->
>> intel_cbphy_iphy_fwnode_parse().
>> Whereas for intel_cbphy_dt_parse() i will keep it unchanged, because it
>> is calling devm_*, devm_platform_*, fwnode_* APIs to traverse dt node.
> How do you know that it will be DT node?
> I can't say it from the function parameters: Is any of them takes of_node?
Got it, All the functions are traversing through device only. I will 
change intel_cbphy_dt_parse() to intel_cbphy_fwnode_parse().
(PS: My intention is something different. As the function is fetching 
device tree node entries so kept is as *_dt_parse() )
>
>>>> +                                 struct fwnode_handle *fwnode, int idx)
> ...
>
>>>> +    struct fwnode_reference_args ref;
>>>> +    struct device *dev = cbphy->dev;
>>>> +    struct fwnode_handle *fwnode;
>>>> +    struct platform_device *pdev;
>>>> +    int i, ret;
>>>> +    u32 prop;
>>> I guess the following would be better:
>> In the v2 patch, for int i = 0 you mentioned to do initialization at the
>> user, instead of doing at declaration.
>> So i followed the same for "pdev" and "fwnode" which are being used
>> after few lines of the code . It looked good in the perspective of code
>> readability.
> No, it is different. For the loop counter is better to have closer to
> the loop, for the more global thingy like platform device it makes it
> actually harder to find.
> When you do assignments you have to think about the variable meaning
> and scope. Scope is different for loop counter versus the mentioned
> rest.

Understand. I will follow the same and keep a note for future drivers too.

Thanks for detail explanation.

Regards,
Dilip

>> .