Re: [PATCH v6 net-next 3/5] drivers/net/phy: add connection between ethtool and phylib for PLCA

[Russell King (Oracle)]

On Mon, Dec 12, 2022 at 10:51:18AM +0100, Piergiorgio Beruto wrote:
> On Sun, Dec 11, 2022 at 08:34:14PM +0000, Russell King (Oracle) wrote:
> > Please see Documentation/doc-guide/kernel-doc.rst
> > 
> > "Function parameters
> > ~~~~~~~~~~~~~~~~~~~
> > 
> > Each function argument should be described in order, immediately following
> > the short function description.  Do not leave a blank line between the
> > function description and the arguments, nor between the arguments."
> > 
> > Note the last sentence - there should _not_ be a blank line, so please
> > follow this for new submissions. I don't think we care enough to fix
> > what's already there though.
> Fair enough, I'll change this. However, I would suggest to write these
> kind of rules (about following the new style vs keeping consistency with
> what it's there already) to help newcomers like me understanding what
> the policy actually is. I got different opinions about that.

phy.c has two different formats for docbook comments - some of them
are to the documented format, others with the extra blank line. Given
that correct form is without the blank line, new docbook comments
should conform to the standard format.

> > This is a review comment I've made already, but you seem to have ignored
> > it. Please ensure that new contributions are safe. Yes, existing code
> > may not be, and that's something we should fix, but your contribution
> > should at least be safer than the existing code.
> > 
> Russle, I did not actually ignore your comment. Looking back at the
> history, you were commenting on the functions in plca.c and we were
> talking about the global rtnl lock.

Thanks for checking back, you're correct. You can ignore this comment
as it won't make any difference.

Looking at phy_remove(), it wouldn't make any difference anyway, so
please keep the code as-is.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!