Re: [PATCH 5/5] net: mtip: The L2 switch driver for imx287

[Andrew Lunn <andrew@lunn.ch>]

> > > +config FEC_MTIP_L2SW
> > > +	tristate "MoreThanIP L2 switch support to FEC driver"
> > > +	depends on OF
> > > +	depends on NET_SWITCHDEV
> > > +	depends on BRIDGE
> > > +	depends on ARCH_MXS || ARCH_MXC  
> > 
> > Missing compile test
> 
> Could you be more specific?

config FEC
        tristate "FEC ethernet controller (of ColdFire and some i.MX CPUs)"
        depends on (M523x || M527x || M5272 || M528x || M520x || M532x || \
                   ARCH_MXC || ARCH_S32 || SOC_IMX28 || COMPILE_TEST)

|| COMPILE_TEST

So that it also gets build on amd64, s390, powerpc etc. The code
should cleanly build on all architectures. It if does not, it probably
means the code is using the kernel abstractions wrong. Also, most
developers build test on amd64, not arm, so if they are making tree
wide changes, you want this driver to build on amd64 so such tree wide
changes get build tested.

> There have been attempts to upstream this driver since 2020...
> The original code is from - v4.4 for vf610 and 2.6.35 for imx287.
> 
> It has been then subsequently updated/rewritten for:
> 
> - 4.19-cip
> - 5.12 (two versions for switchdev and DSA)
> - 6.6 LTS
> - net-next.
> 
> The pr_err() were probably added by me as replacement for
> "printk(KERN_ERR". I can change them to dev_* or netdev_*. This shall
> not be a problem.

With Ethernet switches, you need to look at the context. Is it
something specific to one interface? The netdev_err(). If it is global
to the whole switch, then dev_err().

> > > +	pr_info("Ethernet Switch Version %s\n", VERSION);  
> > 
> > Drivers use dev, not pr. Anyway drop. Drivers do not have versions and
> > should be silent on success.
> 
> As I've written above - there are several "versions" on this particular
> driver. Hence the information.

There is only one version in mainline, this version (maybe). Mainline
does not care about other versions. Such version information is also
useless, because once it is merged, it never changes. What you
actually want to know is the kernel git hash, so you can find the
exact sources. ethtool -I will return that, assuming your ethtool code
is correct.

> > > +	pr_info("Ethernet Switch HW rev 0x%x:0x%x\n",
> > > +		MCF_MTIP_REVISION_CUSTOMER_REVISION(rev),
> > > +		MCF_MTIP_REVISION_CORE_REVISION(rev));  
> > 
> > Drop
> 
> Ok.

You can report this via ethtool -I. But i suggest you leave that for
later patches.

> > > +	fep->reg_phy = devm_regulator_get(&pdev->dev, "phy");
> > > +	if (!IS_ERR(fep->reg_phy)) {
> > > +		ret = regulator_enable(fep->reg_phy);
> > > +		if (ret) {
> > > +			dev_err(&pdev->dev,
> > > +				"Failed to enable phy regulator:
> > > %d\n", ret);
> > > +			goto failed_regulator;
> > > +		}
> > > +	} else {
> > > +		if (PTR_ERR(fep->reg_phy) == -EPROBE_DEFER) {
> > > +			ret = -EPROBE_DEFER;
> > > +			goto failed_regulator;
> > > +		}
> > > +		fep->reg_phy = NULL;  
> > 
> > I don't understand this code. Do you want to re-implement
> > get_optional? But why?
> 
> Here the get_optional() shall be used.

This is the problem with trying to use old code. It needs more work
than just making it compile. It needs to be brought up to HEAD of
mainline standard, which often nearly ends in a re-write.

> > > +	fep->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > +	if (IS_ERR(fep->clk_ipg))
> > > +		fep->clk_ipg = NULL;  
> > 
> > Why?
> 
> I will update the code.

FYI: NULL is a valid clock. But do you actually want _optional()?

This is the sort of thing a 10 year old codebase will be missing, and
you need to re-write. You might also be able to use the clk _bulk_
API?

	Andrew