Hi Andrew,
On Thu, Apr 13, 2023 at 06:12:28PM +0200, Andrew Lunn wrote:
> > num_interfaces = cvmx_helper_get_number_of_interfaces();
> > for (interface = 0; interface < num_interfaces; interface++) {
> > + int num_ports, port_index;
> > + const struct net_device_ops *ops;
> > + const char *name;
> > + phy_interface_t phy_mode = PHY_INTERFACE_MODE_NA;
> > cvmx_helper_interface_mode_t imode =
> > - cvmx_helper_interface_get_mode(interface);
> > - int num_ports = cvmx_helper_ports_on_interface(interface);
> > - int port_index;
> > + cvmx_helper_interface_get_mode(interface);
> > +
> > + switch (imode) {
> > + case CVMX_HELPER_INTERFACE_MODE_NPI:
> > + ops = &cvm_oct_npi_netdev_ops;
> > + name = "npi%d";
>
> In general, the kernel does not give the interface names other than
> ethX. userspace can rename them, e.g. systemd with its persistent
> names. So as part of getting this driver out of staging, i would throw
> this naming code away.
That would break all userspace (which is often running vendor's kernel).
But since driver is in staging and noone cares about vendor's kernel
I guess it is okay...
> > + num_ports = cvmx_helper_ports_on_interface(interface);
> > for (port_index = 0,
> > port = cvmx_helper_get_ipd_port(interface, 0);
> > port < cvmx_helper_get_ipd_port(interface, num_ports);
> > port_index++, port++) {
> > struct octeon_ethernet *priv;
> > struct net_device *dev =
> > - alloc_etherdev(sizeof(struct octeon_ethernet));
> > + alloc_etherdev(sizeof(struct octeon_ethernet));
>
> Please try to avoid white space changed. Put such white space changes
> into a patch of their own, with a commit message saying it just
> contains whitespace cleanup.
Sorry, I overlooked this.
> > if (!dev) {
> > pr_err("Failed to allocate ethernet device for port %d\n",
> > port);
> > @@ -830,7 +875,12 @@ static int cvm_oct_probe(struct platform_device *pdev)
> > priv->port = port;
> > priv->queue = cvmx_pko_get_base_queue(priv->port);
> > priv->fau = fau - cvmx_pko_get_num_queues(port) * 4;
> > - priv->phy_mode = PHY_INTERFACE_MODE_NA;
> > + priv->phy_mode = phy_mode;
>
> You should be getting phy_mode from DT.
>
> Ideally, you want lots of small patches which are obviously
> correct. So i would try to break this up into smaller changes.
>
> I also wounder if you are addresses issues in the correct order. This
> driver is in staging for a reason. It needs a lot of work. You might
> be better off first cleaning it up. And then consider moving it to
> phylink.
I was asking this question myself and then came to this:
Converting driver to phylink makes separating different macs easier as
this driver is splitted between staging and arch/mips/cavium-octeon/executive/
However I'll provide changes spotted previously as separate preparational
patches. Would that work for you?
> Andrew
Thank you,
ladis
