From: Jiri Pirko <jiri@resnulli.us>
Sent: Saturday, June 10, 2023 10:46 AM

Fri, Jun 09, 2023 at 02:18:50PM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

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.

quoted

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:

quoted

+
+#define MAX_NETLIST_SIZE	10

Prefix perhaps?

Fixed.

[...]

quoted

+/**
+ * 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.

quoted

+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.

quoted

+	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"

quoted

+		: 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.

[...]

quoted

+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;

quoted

+	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

quoted

+
+	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;
+}
+

[...]