netdev - Re: [PATCH net-next 1/6] net: stmmac: add stmmac_set_tx_clk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <j64xmkplk2kkb4esteaic3hsofex3eishxxr3z6hppnm6heoz5@5fyj4x5qouc3>
Date: Thu, 14 Sep 2023 17:54:09 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
Cc: Alexandre Torgue <alexandre.torgue@...s.st.com>, 
	Jose Abreu <joabreu@...opsys.com>, Alexei Starovoitov <ast@...nel.org>, bpf@...r.kernel.org, 
	Daniel Borkmann <daniel@...earbox.net>, "David S. Miller" <davem@...emloft.net>, 
	Emil Renner Berthing <kernel@...il.dk>, Eric Dumazet <edumazet@...gle.com>, 
	Fabio Estevam <festevam@...il.com>, Jakub Kicinski <kuba@...nel.org>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, 
	linux-arm-kernel@...ts.infradead.org, linux-stm32@...md-mailman.stormreply.com, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org, NXP Linux Team <linux-imx@....com>, 
	Paolo Abeni <pabeni@...hat.com>, Pengutronix Kernel Team <kernel@...gutronix.de>, 
	Samin Guo <samin.guo@...rfivetech.com>, Sascha Hauer <s.hauer@...gutronix.de>, 
	Shawn Guo <shawnguo@...nel.org>
Subject: Re: [PATCH net-next 1/6] net: stmmac: add stmmac_set_tx_clk_gmii()

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@...linux.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@...il.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
> 
>