>From: Jiri Pirko <jiri@xxxxxxxxxxx>
>Sent: Saturday, June 10, 2023 10:46 AM
>
>Fri, Jun 09, 2023 at 02:18:50PM CEST, arkadiusz.kubalewski@xxxxxxxxx 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?
>
Sure, will do.
>
>>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@xxxxxxxxx>
>
>I just skimmed over this, not really give it much of a time. Couple of
>nits:
>
>
>>+
>>+#define MAX_NETLIST_SIZE 10
>
>Prefix perhaps?
>
Fixed.
>[...]
>
>
>>+/**
>>+ * 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?
>
Sure, fixed.
>
>[...]
>
>
>
>>+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...
>
Fixed.
Thank you!
Arkadiusz
>
>>+
>>+ 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;
>>+}
>>+
>
>[...]
