On Thu, Jun 18, 2020 at 04:17:56PM +0000, Ioana Ciornei wrote:

quoted

quoted

+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!