On Mon, Apr 22, 2024 at 10:41:47AM +0000, Kwapulinski, Piotr wrote: > >-----Original Message----- > >From: Simon Horman <[email protected]> > >Sent: Saturday, April 20, 2024 8:18 PM > >To: Kwapulinski, Piotr <[email protected]> > >Cc: [email protected]; [email protected]; Gomes, > >Vinicius <[email protected]>; Wegrzyn, Stefan > ><[email protected]>; Jagielski, Jedrzej > ><[email protected]>; Sokolowski, Jan <[email protected]> > >Subject: Re: [PATCH iwl-next v2 2/5] ixgbe: Add support for E610 device > >capabilities detection > > > >On Mon, Apr 15, 2024 at 12:34:32PM +0200, Piotr Kwapulinski wrote: > >> Add low level support for E610 device capabilities detection. The > >> capabilities are discovered via the Admin Command Interface. Discover > >> the following capabilities: > >> - function caps: vmdq, dcb, rss, rx/tx qs, msix, nvm, orom, reset > >> - device caps: vsi, fdir, 1588 > >> - phy caps > >> > >> Co-developed-by: Stefan Wegrzyn <[email protected]> > >> Signed-off-by: Stefan Wegrzyn <[email protected]> > >> Co-developed-by: Jedrzej Jagielski <[email protected]> > >> Signed-off-by: Jedrzej Jagielski <[email protected]> > >> Reviewed-by: Jan Sokolowski <[email protected]> > >> Signed-off-by: Piotr Kwapulinski <[email protected]> > > > >Hi Pitor, > > > >A few minor nits from my side. > >No need to respin just because of these. > > > >> --- > >> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 517 > >> ++++++++++++++++++ drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h | > >> 11 + > >> 2 files changed, 528 insertions(+) > >> > >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c > >> b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c > > > >... > > > >> +/** > >> + * ixgbe_get_num_per_func - determine number of resources per PF > >> + * @hw: pointer to the HW structure > >> + * @max: value to be evenly split between each PF > >> + * > >> + * Determine the number of valid functions by going through the > >> +bitmap returned > >> + * from parsing capabilities and use this to calculate the number of > >> +resources > >> + * per PF based on the max value passed in. > >> + * > >> + * Return: the number of resources per PF or 0, if no PH are available. > >> + */ > >> +static u32 ixgbe_get_num_per_func(struct ixgbe_hw *hw, u32 max) { > >> + const u32 IXGBE_CAPS_VALID_FUNCS_M = 0xFF; > > > >nit: Maybe this could simply be a #define? > Hello, > will do > > > > >> + u8 funcs = hweight8(hw->dev_caps.common_cap.valid_functions & > >> + IXGBE_CAPS_VALID_FUNCS_M); > > > >nit: Please consider using reverse xmas tree order - longest line to > >shortest - > > for local variables in new Networking code > Will do > > > > >> + > >> + return funcs ? (max / funcs) : 0; > >> +} > > > >... > > > >> +/** > >> + * ixgbe_aci_disable_rxen - disable RX > >> + * @hw: pointer to the HW struct > >> + * > >> + * Request a safe disable of Receive Enable using ACI command (0x000C). > >> + * > >> + * Return: the exit code of the operation. > >> + */ > >> +int ixgbe_aci_disable_rxen(struct ixgbe_hw *hw) { > >> + struct ixgbe_aci_cmd_disable_rxen *cmd; > >> + struct ixgbe_aci_desc desc; > >> + > >> + cmd = &desc.params.disable_rxen; > >> + > >> + ixgbe_fill_dflt_direct_cmd_desc(&desc, ixgbe_aci_opc_disable_rxen); > >> + > >> + cmd->lport_num = (u8)hw->bus.func; > > > >nit: This cast seems unnecessary. > > AFAICT the type of hw->bus.func is u8. > Will do
Thanks. FWIIW, I think I noticed a similar cast at least once more elsewhere in the patchset > > > > >> + > >> + return ixgbe_aci_send_cmd(hw, &desc, NULL, 0); } > > > >... > Thank you for review > Piotr >
