Hi Kishon,

On Tue, Aug 29, 2017 at 04:34:17PM +0530, Kishon Vijay Abraham I wrote:

On Monday 28 August 2017 08:27 PM, Antoine Tenart wrote:

quoted

 
+config PHY_MVEBU_CP110_COMPHY
+	tristate "Marvell CP110 comphy driver"
+	depends on ARCH_MVEBU && OF

(ARCH_MVEBU || COMPILE_TEST) above..

Sure, I'll update.

quoted

+static const struct mvebu_comhy_conf mvebu_comphy_cp110_modes[] = {
+	/* lane 0 */
+	MVEBU_COMPHY_CONF(0, 1, PHY_MODE_SGMII, 0x1),
+	/* lane 1 */
+	MVEBU_COMPHY_CONF(1, 2, PHY_MODE_SGMII, 0x1),
+	/* lane 2 */
+	MVEBU_COMPHY_CONF(2, 0, PHY_MODE_SGMII, 0x1),
+	MVEBU_COMPHY_CONF(2, 0, PHY_MODE_10GKR, 0x1),
+	/* lane 3 */
+	MVEBU_COMPHY_CONF(3, 1, PHY_MODE_SGMII, 0x2),
+	/* lane 4 */
+	MVEBU_COMPHY_CONF(4, 0, PHY_MODE_SGMII, 0x2),
+	MVEBU_COMPHY_CONF(4, 0, PHY_MODE_10GKR, 0x2),
+	MVEBU_COMPHY_CONF(4, 1, PHY_MODE_SGMII, 0x1),
+	/* lane 5 */
+	MVEBU_COMPHY_CONF(5, 2, PHY_MODE_SGMII, 0x1),
+};

IMHO all the lane and mode configuration should come from dt. That would make
it more reusable when comphy is configured differently.

These connexions between engines and the comphy lanes are inside the
SoC. They won't change for a given SoC, and the actual configuration is
at the board level to know what is connected to the output of a given
lane, which is already described into the dt (the lane phandle).

So I think we can keep this inside the driver, and we'll had other
tables if the same comphy is ever used in another SoC.

What do you think?

quoted

+static const struct phy_ops mvebu_comphy_ops = {
+	.power_on	= mvebu_comphy_power_on,
+	.power_off	= mvebu_comphy_power_off,
+	.set_mode	= mvebu_comphy_set_mode,

missing .owner

I'll fix that.

quoted

+static struct phy *mvebu_comphy_xlate(struct device *dev,
+				      struct of_phandle_args *args)
+{
+	struct mvebu_comphy_priv *priv = dev_get_drvdata(dev);
+	struct mvebu_comphy_lane *lane;
+	int i;
+
+	if (WARN_ON(args->args[0] >= MVEBU_COMPHY_PORTS))
+		return ERR_PTR(-EINVAL);
+
+	for (i = 0; i < MVEBU_COMPHY_LANES; i++) {
+		if (!priv->phys[i])
+			continue;
+
+		lane = phy_get_drvdata(priv->phys[i]);
+		if (priv->phys[i] && args->np == lane->of_node)
+			break;
+	}

You should be able to directly use of_phy_simple_xlate to get the phy pointer.
(For that to work child node pointer should be passed in devm_phy_create).

Good idea, I'll look into this and update.

Thanks!
Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com