Re: [PATCH net-next 02/13] phy: add the mvebu cp110 comphy driver

Antoine Tenart
2017-08-24T13:44:57Z

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

Hi Andrew,

On Thu, Aug 24, 2017 at 03:39:22PM +0200, Andrew Lunn wrote:
> > +static const struct mvebu_comhy_conf mvebu_comphy_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),
> > +};
>
> Do other Marvell SoCs re-use this IP? Maybe add cp110 to the name here
> to indicate what SoC this configuration belongs to? I guess at some
> point, the compatible string will be used to select the correct table
> for the hardware variant.

OK, I'll rename the variable mvebu_comphy_cp110_modes.

> > +static const struct of_device_id mvebu_comphy_of_match_table[] = {
> > + { .compatible = "marvell,comphy-cp110" },
>
> Is that specific enough? It seems like this table is easy to change in
> the VHDL. Could there be another cp110 with a different configuration?

As far as I understand CP110 is the name of the CP, should there be
another one it should be named differently. But I can't be 100% sure,
you never know what comes next :)

How would you name it if not "comphy-cp110"?

Thanks!
Antoine

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]