Messages in this thread | | | Date | Mon, 2 Mar 2020 13:19:01 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v4 3/3] phy: intel: Add driver support for ComboPhy |
| |
On Mon, Mar 02, 2020 at 04:43:25PM +0800, Dilip Kota wrote: > ComboPhy subsystem provides PHYs for various > controllers like PCIe, SATA and EMAC.
Thanks for an update, my (few minor) comments below.
...
> +enum intel_phy_mode { > + PHY_PCIE_MODE = 0, > + PHY_XPCS_MODE,
> + PHY_SATA_MODE
From here it's not visible that above is the only possible values. Maybe in the future you will have another mode. So, I suggest to leave comma here...
> +};
> +enum intel_combo_mode { > + PCIE0_PCIE1_MODE = 0, > + PCIE_DL_MODE, > + RXAUI_MODE, > + XPCS0_XPCS1_MODE,
> + SATA0_SATA1_MODE
...and here...
> +}; > + > +enum aggregated_mode { > + PHY_SL_MODE,
> + PHY_DL_MODE
...and here.
> +};
...
> +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];
Do you really need temporary variable here?
> + > + return phy_cfg(sphy); > +}
...
> + if (!cbphy->init_cnt) {
if (init_cnt) return 0;
?
> + clk_disable_unprepare(cbphy->core_clk); > + intel_cbphy_rst_assert(cbphy); > + } > + > + return 0;
-- With Best Regards, Andy Shevchenko
| |