> Subject: Re: [PATCH net-next 4/5] net: phy: add Lynx PCS MDIO module
>
> 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.
This is what I was proposing. I can of course take the indirection away
and just export the functions.
Are there really instances where the ethernet driver has to manage multiple
different types of PCSs? I am not sure this type of snippet of code is really
going to occur.
>
> 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?
>
I am not sure how this would work with Felix and DSA drivers in general since
the
DSA core is hiding the phylink_pcs_ops from the actual switch driver.
> 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.
I get what you are saying but I do not know of any drivers that actually need
this
distinction at runtime.
Ioana
>
> > 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.
>
