Messages in this thread | | | Date | Sat, 10 Jun 2023 10:46:24 +0200 | From | Jiri Pirko <> | Subject | Re: [RFC PATCH v8 07/10] ice: add admin commands to access cgu configuration |
| |
Fri, Jun 09, 2023 at 02:18:50PM CEST, arkadiusz.kubalewski@intel.com wrote: >Add firmware admin command to access clock generation unit >configuration, it is required to enable Extended PTP and SyncE features >in the driver.
Empty line here perhaps?
>Add definitions of possible hardware variations of input and output pins >related to clock generation unit and functions to access the data. > >Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
I just skimmed over this, not really give it much of a time. Couple of nits:
>+ >+#define MAX_NETLIST_SIZE 10
Prefix perhaps?
[...]
>+/** >+ * convert_s48_to_s64 - convert 48 bit value to 64 bit value >+ * @signed_48: signed 64 bit variable storing signed 48 bit value >+ * >+ * Convert signed 48 bit value to its 64 bit representation. >+ * >+ * Return: signed 64 bit representation of signed 48 bit value. >+ */ >+static inline
Never use "inline" in a c file.
>+s64 convert_s48_to_s64(s64 signed_48) >+{ >+ const s64 MASK_SIGN_BITS = GENMASK_ULL(63, 48);
variable with capital letters? Not nice. Define? You have that multiple times in the patch.
>+ const s64 SIGN_BIT_47 = BIT_ULL(47); >+ >+ return ((signed_48 & SIGN_BIT_47) ? (s64)(MASK_SIGN_BITS | signed_48)
Pointless cast, isn't it?
You don't need () around "signed_48 & SIGN_BIT_47"
>+ : signed_48);
Return is not a function. Drop the outer "()"s.
The whole fuction can look like: static s64 convert_s48_to_s64(s64 signed_48) { return signed_48 & BIT_ULL(47) ? signed_48 | GENMASK_ULL(63, 48) : signed_48; }
Nicer?
[...]
>+int ice_get_pf_c827_idx(struct ice_hw *hw, u8 *idx) >+{ >+ struct ice_aqc_get_link_topo cmd; >+ u8 node_part_number; >+ u16 node_handle; >+ int status; >+ u8 ctx; >+ >+ if (hw->mac_type != ICE_MAC_E810) >+ return -ENODEV; >+ >+ if (hw->device_id != ICE_DEV_ID_E810C_QSFP) { >+ *idx = C827_0; >+ return 0; >+ } >+ >+ memset(&cmd, 0, sizeof(cmd)); >+ >+ ctx = ICE_AQC_LINK_TOPO_NODE_TYPE_PHY << ICE_AQC_LINK_TOPO_NODE_TYPE_S; >+ ctx |= ICE_AQC_LINK_TOPO_NODE_CTX_PORT << ICE_AQC_LINK_TOPO_NODE_CTX_S; >+ cmd.addr.topo_params.node_type_ctx = ctx; >+ cmd.addr.topo_params.index = 0;
You zeroed the struct 4 lines above...
>+ >+ status = ice_aq_get_netlist_node(hw, &cmd, &node_part_number, >+ &node_handle); >+ if (status || node_part_number != ICE_ACQ_GET_LINK_TOPO_NODE_NR_C827) >+ return -ENOENT; >+ >+ if (node_handle == E810C_QSFP_C827_0_HANDLE) >+ *idx = C827_0; >+ else if (node_handle == E810C_QSFP_C827_1_HANDLE) >+ *idx = C827_1; >+ else >+ return -EIO; >+ >+ return 0; >+} >+
[...]
| |