On Thu, Sep 14, 2023 at 04:12:13PM +0100, Russell King (Oracle) wrote:
> On Thu, Sep 14, 2023 at 05:54:09PM +0300, Serge Semin wrote:
> > On Thu, Sep 14, 2023 at 02:51:20PM +0100, Russell King (Oracle) wrote:
> > > Add a helper function for setting the transmit clock for GMII
> > > interfaces. This handles 1G, 100M and 10M using the standard clock
> > > rates of 125MHz, 25MHz and 2.5MHz.
> > >
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@xxxxxxxxxxxxxxx>
> > > ---
> > > .../ethernet/stmicro/stmmac/stmmac_platform.c | 25 +++++++++++++++++++
> > > .../ethernet/stmicro/stmmac/stmmac_platform.h | 1 +
> > > 2 files changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > > index 0f28795e581c..f7635ed2b255 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > > @@ -700,6 +700,31 @@ EXPORT_SYMBOL_GPL(stmmac_probe_config_dt);
> > > EXPORT_SYMBOL_GPL(devm_stmmac_probe_config_dt);
> > > EXPORT_SYMBOL_GPL(stmmac_remove_config_dt);
> > >
> >
> > > +int stmmac_set_tx_clk_gmii(struct clk *tx_clk, unsigned int speed)
> > > +{
> > > + unsigned long rate;
> > > +
> > > + switch (speed) {
> > > + case SPEED_1000:
> > > + rate = 125000000;
> > > + break;
> > > +
> > > + case SPEED_100:
> > > + rate = 25000000;
> > > + break;
> > > +
> > > + case SPEED_10:
> > > + rate = 2500000;
> > > + break;
> > > +
> > > + default:
> > > + return -ENOTSUPP;
> > > + }
> > > +
> > > + return clk_set_rate(tx_clk, rate);
> > > +}
> > > +EXPORT_SYMBOL_GPL(stmmac_set_tx_clk_gmii);
> >
> > As I already noted in v1 normally the switch-case operations are
> > defined with no additional line separating the cases. I would have
> > dropped them here too especially seeing the stmmac core driver mainly
> > follow that implicit convention.
>
> It's rather haphazard whether there are blank lines or not between
> case statements.
Is it haphazard in the STMMAC core driver too? The only exception is
the HWtstamp switch-case statements which just a bit bulky. So having
additional empty lines there rather weakly but is still justified by
that.
In anyway my comment is just a nitpick inferred from the implicit
local convention. It's totally IMO and isn't implied to be considered
as a strong request to be implemented. I repeated my comment just
because you didn't respond to it in v1. It looked as if you just
missed it.
>
> > Additionally I suggest to move the method to being defined at the head
> > of the file. Thus a more natural order normally utilized in the kernel
> > drivers would be preserved: all functional implementations go first,
> > the platform-specific things are placed below like probe()/remove()
> > and their sub-functions, suspend()/resume() and PM descriptors,
> > (device IDs table, driver descriptor, etc). stmmac_set_tx_clk_gmii()
> > looks as a functional helper which is normally utilized on the network
> > device open() stage in the framework of the fix_mac_speed() callback.
> > Moreover my suggestion gets to be even more justified seeing you
> > placed the method prototype at the head of the prototypes list in the
> > stmmac_platform.h file.
>
> How is one supposed to know about this? I did my best trying to work
> out where they should've gone...
Well, from my experience submitting the patches to various kernel
subsystems and drivers there are many implicit conventions which
aren't described anywhere, but could be inferred from the code itself.
This one is one of such implicit conventions which isn't mandatory but
a nice-to-have feature for better readability and maintainability (for
instance in order to determine where to put new methods and features
to the already available drivers). In anyway this comment is also a
nitpick, which from my point of view would improve the code
readability. It's normally up to the driver/subsystem maintainers to
define such conventions required.
Regarding the implicit conventions. Some of the subsystem and driver
maintainers imply that such conventions would be preserved (just
recently met that in the PCIe subsystem). When it happens it's so
irritating especially if it concerns a big series.
>
> If it's that important, maybe add some /* Comments */ to state that
> there are separate sections to the file?
Would be nice to have them indeed. Though I normally just stick to
that convention by default if there is no another one could be
inferred from the code itself.
-Serge(y)
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
