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

[Lukasz Majewski]

Hi Andrew,

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

^^^^^^^^^^^^^^^^^^ - ach ... this "compile test" :-)

(Not that I've posted the code not even compiling ... :-D)

> 
> 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().

Ok.

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

Ok. I will.

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

I will remove it.

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

But you cannot rewrite this code from scratch, as the IP block is not
so well documented, and there maybe are some issues that you are not
aware of.

Moreover, this code is already in production use, and you don't want to
be in situation when regression tests cannot be run.

> 
> > > > +	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()?

Yes, I will use _optional() and _bulk_ if applicable.

> 
> 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?

The goal is to have this driver upstreamed (finally), so the starting
point of further development would be in kernel.

> 
> 	Andrew


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@xxxxxxx

Attachment: pgpv7agEicIbO.pgp
Description: OpenPGP digital signature