linux-kernel - Re: [PATCH V10 net-next 03/10] net: hibmcge: Add mdio and hardware configuration supported in this module

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <19edbeff-7658-4302-9d40-d3cf5a1a64b3@wanadoo.fr>
Date: Sun, 15 Sep 2024 17:36:36 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Jijie Shao <shaojijie@...wei.com>, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Cc: shenjian15@...wei.com, wangpeiyang1@...wei.com, liuyonglong@...wei.com,
 chenhao418@...wei.com, sudongming1@...wei.com, xujunsheng@...wei.com,
 shiyongbang@...wei.com, libaihan@...wei.com, andrew@...n.ch,
 jdamato@...tly.com, horms@...nel.org, kalesh-anakkur.purayil@...adcom.com,
 jonathan.cameron@...wei.com, shameerali.kolothum.thodi@...wei.com,
 salil.mehta@...wei.com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V10 net-next 03/10] net: hibmcge: Add mdio and hardware
 configuration supported in this module

Le 12/09/2024 à 04:51, Jijie Shao a écrit :
> Implements the C22 read and write PHY registers interfaces.
> 
> Some hardware interfaces related to the PHY are also implemented
> in this patch.
> 
> Signed-off-by: Jijie Shao <shaojijie@...wei.com>

Hi,

a few nitpick, should there be a v11.

> +static void hbg_hw_init_transmit_control(struct hbg_priv *priv)
> +{
> +	u32 control = 0;
> +
> +	control |= FIELD_PREP(HBG_REG_TRANSMIT_CONTROL_AN_EN_B, HBG_STATUS_ENABLE);
> +	control |= FIELD_PREP(HBG_REG_TRANSMIT_CONTROL_CRC_ADD_B, HBG_STATUS_ENABLE);
> +	control |= FIELD_PREP(HBG_REG_TRANSMIT_CONTROL_PAD_EN_B, HBG_STATUS_ENABLE);
> +
> +	hbg_reg_write(priv, HBG_REG_TRANSMIT_CONTROL_ADDR, control);
> +}
> +
> +static void hbg_hw_init_rx_ctrl(struct hbg_priv *priv)
> +{
> +	u32 ctrl = 0;

Nitpick: the same kind of variable is called 'control' just above.
Same in the function name.

Having the same pattern everywhere would look more consistent.

> +
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_RX_GET_ADDR_MODE_B, HBG_STATUS_ENABLE);
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_TIME_INF_EN_B, HBG_STATUS_DISABLE);
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_RXBUF_1ST_SKIP_SIZE_M, HBG_RX_SKIP1);
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_RXBUF_1ST_SKIP_SIZE2_M, HBG_RX_SKIP2);
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_RX_ALIGN_NUM_M, NET_IP_ALIGN);
> +	ctrl |= FIELD_PREP(HBG_REG_RX_CTRL_PORT_NUM, priv->dev_specs.mac_id);
> +
> +	hbg_reg_write(priv, HBG_REG_RX_CTRL_ADDR, ctrl);
> +}

...

> +static int hbg_mdio_init_hw(struct hbg_priv *priv)

Nitpick: This could return void.

> +{
> +	u32 freq = priv->dev_specs.mdio_frequency;
> +	struct hbg_mac *mac = &priv->mac;
> +	u32 cmd = 0;
> +
> +	cmd |= FIELD_PREP(HBG_REG_MDIO_COMMAND_ST_M, HBG_MDIO_C22_MODE);
> +	cmd |= FIELD_PREP(HBG_REG_MDIO_COMMAND_AUTO_SCAN_B, HBG_STATUS_DISABLE);
> +
> +	/* freq use two bits, which are stored in clk_sel and clk_sel_exp */
> +	cmd |= FIELD_PREP(HBG_REG_MDIO_COMMAND_CLK_SEL_B, freq & 0x1);
> +	cmd |= FIELD_PREP(HBG_REG_MDIO_COMMAND_CLK_SEL_EXP_B, (freq >> 1) & 0x1);
> +
> +	hbg_mdio_set_command(mac, cmd);
> +	return 0;
> +}

...

CJ