| Message ID | E1qgmka-007Z4Z-1E@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: stmmac: add and use library for setting clock | expand |
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@armlinux.org.uk> > --- > .../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. 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. Irrespective to the nitpicks above the change looks good: Reviewed-by: Serge Semin <fancer.lancer@gmail.com> -Serge(y) > + > int stmmac_get_platform_resources(struct platform_device *pdev, > struct stmmac_resources *stmmac_res) > { > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h > index c5565b2a70ac..8dc2287c6724 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h > @@ -11,6 +11,7 @@ > > #include "stmmac.h" > > +int stmmac_set_tx_clk_gmii(struct clk *tx_clk, unsigned int speed); > struct plat_stmmacenet_data * > stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac); > struct plat_stmmacenet_data * > -- > 2.30.2 > >
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@armlinux.org.uk> > > --- > > .../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. > 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... If it's that important, maybe add some /* Comments */ to state that there are separate sections to the file?
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@armlinux.org.uk> > > > --- > > > .../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!
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); + int stmmac_get_platform_resources(struct platform_device *pdev, struct stmmac_resources *stmmac_res) { diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h index c5565b2a70ac..8dc2287c6724 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h @@ -11,6 +11,7 @@ #include "stmmac.h" +int stmmac_set_tx_clk_gmii(struct clk *tx_clk, unsigned int speed); struct plat_stmmacenet_data * stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac); struct plat_stmmacenet_data *
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@armlinux.org.uk> --- .../ethernet/stmicro/stmmac/stmmac_platform.c | 25 +++++++++++++++++++ .../ethernet/stmicro/stmmac/stmmac_platform.h | 1 + 2 files changed, 26 insertions(+)