RE: [RFC PATCH v8 07/10] ice: add admin commands to access cgu configuration

[Kubalewski, Arkadiusz]

>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;
>>+}
>>+
>
>[...]