Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Thu, Jun 18, 2020 at 04:17:56PM +0000, Ioana Ciornei wrote:
> > > +struct mdio_lynx_pcs *mdio_lynx_pcs_create(struct mdio_device
> > > +*mdio_dev) {
> > > +	struct mdio_lynx_pcs *pcs;
> > > +
> > > +	if (WARN_ON(!mdio_dev))
> > > +		return NULL;
> > > +
> > > +	pcs = kzalloc(sizeof(*pcs), GFP_KERNEL);
> > > +	if (!pcs)
> > > +		return NULL;
> > > +
> > > +	pcs->dev = mdio_dev;
> > > +	pcs->an_restart = lynx_pcs_an_restart;
> > > +	pcs->get_state = lynx_pcs_get_state;
> > > +	pcs->link_up = lynx_pcs_link_up;
> > > +	pcs->config = lynx_pcs_config;
> > 
> > We really should not have these private structure interfaces.  Private structure-
> > based driver specific interfaces really don't constitute a sane approach to
> > interface design.
> > 
> > Would it work if there was a "struct mdio_device" add to the phylink_config
> > structure, and then you could have the phylink_pcs_ops embedded into this
> > driver?
> 
> I think that would restrict too much the usage.
> I am afraid there will be instances where the PCS is not recognizable by PHY_ID,
> thus no way of knowing which driver to probe which mdio_device.
> Also, I would leave to the driver the choice of using (or not) the functions 
> exported by Lynx.

I think you've taken my point way too far.  What I'm complaining about
is the indirection of lynx_pcs_an_restart() et.al. through a driver-
private structure.

In order to access lynx_pcs_an_restart(), we need to dereference
struct mdio_lynx_pcs, which is a structure specific to this lynx PCS
driver.  What this leads to is users doing this:

	if (pcs_is_lynx) {
		struct mdio_lynx_pcs *pcs = foo->bar;

		pcs->an_restart(...);
	} else if (pcs_is_something_else) {
		struct mdio_somethingelse_pcs *pcs = foo->bar;

		pcs->an_restart(...);
	}

which really does not scale.  A step forward would be:

	if (pcs_is_lynx) {
		lynx_pcs_an_restart(...);
	} else if (pcs_is_something_else) {
		something_else_pcs_an_restart(...);
	}

but that also scales horribly.

Even better would be to have a generic set of operations for PCS
devices that can be declared in the lynx PCS driver and used
externally... like, maybe struct phylink_pcs_ops, which is made
globally visible for MAC drivers to use with phylink_add_pcs().

Or maybe a function in this lynx PCS driver that calls phylink_add_pcs()
itself with its own PCS operations, and maybe also sets a field in
struct phylink_config for the PCS mdio device?

Or something like that - just some a way that doesn't force us down
a path that we end up forcing people into code that has to decide
what sort of PCS we have at runtime in all these method paths.

> What if we directly export the helper functions directly as symbols which can
> be used by the driver without any mdio_lynx_pcs in the middle
> (just the mdio_device passed to the function).
> This would be exactly as phylink_mii_c22_pcs_[an_restart/config] are currently
> used.

The difference is that phylink_mii_c22_pcs_* are designed as library
functions - functions that are very likely to be re-used for multiple
different PCS (because the format, location, and access method of
these registers is defined by IEEE 802.3).  It's a bit like phylib's
configuration of autoneg - we don't have all the individual drivers
doing that, we have core code that does that for us in the form of
helpers.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!