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: > > > > +config PHY_MVEBU_CP110_COMPHY > > + tristate "Marvell CP110 comphy driver" > > + depends on ARCH_MVEBU && OF > > (ARCH_MVEBU || COMPILE_TEST) above..
Sure, I'll update.
> > +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?
> > +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.
> > +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
signature.asc
Description: PGP signature
