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.
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@xxxxxxxxx>
-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
>
>
