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!
