Re: [PATCH V2 net-next 1/6] net: wwan: iosm: devlink registration

Small style nits.

On Sun, Sep 19, 2021 at 10:56:18PM +0530, M Chetan Kumar wrote:

+static int ipc_devlink_flash_update(struct devlink *devlink,
+				    struct devlink_flash_update_params *params,
+				    struct netlink_ext_ack *extack)
+{
+	struct iosm_devlink *ipc_devlink = devlink_priv(devlink);
+	enum iosm_flash_comp_type fls_type;
+	u32 rc = -EINVAL;

This should be an int.

+	u8 *mdm_rsp;
+
+	if (!params->component)
+		return rc;

It's more readable to return literals.  "return -EINVAL;"

+
+	mdm_rsp = kzalloc(IOSM_EBL_DW_PACK_SIZE, GFP_KERNEL);
+	if (!mdm_rsp)
+		return -ENOMEM;
+
+	fls_type = ipc_devlink_get_flash_comp_type(params->component,
+						   strlen(params->component));
+
+	switch (fls_type) {
+	case FLASH_COMP_TYPE_PSI:
+		rc = ipc_flash_boot_psi(ipc_devlink, params->fw);
+		break;
+	case FLASH_COMP_TYPE_EBL:
+		rc = ipc_flash_boot_ebl(ipc_devlink, params->fw);
+		if (!rc)

Always do error handling.  Never do success handling.  If you do:

	if (rc)
		break;

+			rc = ipc_flash_boot_set_capabilities(ipc_devlink,
+							     mdm_rsp);

Then this fits on one line.

		rc = ipc_flash_boot_set_capabilities(ipc_devlink, mdm_rsp);

+		if (!rc)
+			rc = ipc_flash_read_swid(ipc_devlink, mdm_rsp);
+		break;
+	case FLASH_COMP_TYPE_FLS:
+		rc = ipc_flash_send_fls(ipc_devlink, params->fw, mdm_rsp);
+		break;
+	default:
+		devlink_flash_update_status_notify(devlink, "Invalid component",
+						   params->component, 0, 0);
+		break;
+	}
+
+	if (!rc)
+		devlink_flash_update_status_notify(devlink, "Flashing success",
+						   params->component, 0, 0);
+	else
+		devlink_flash_update_status_notify(devlink, "Flashing failed",
+						   params->component, 0, 0);
+
+	kfree(mdm_rsp);
+	return rc;
+}

[ snip ]

+static int ipc_devlink_coredump_snapshot(struct devlink *dl,
+					 const struct devlink_region_ops *ops,
+					 struct netlink_ext_ack *extack,
+					 u8 **data)
+{
+	struct iosm_devlink *ipc_devlink = devlink_priv(dl);
+	struct iosm_coredump_file_info *cd_list = ops->priv;
+	u32 region_size;
+	int rc;
+
+	dev_dbg(ipc_devlink->dev, "Region:%s, ID:%d", ops->name,
+		cd_list->entry);
+	region_size = cd_list->default_size;
+	rc = ipc_coredump_collect(ipc_devlink, data, cd_list->entry,
+				  region_size);
+	if (rc) {
+		dev_err(ipc_devlink->dev, "Fail to create snapshot,err %d", rc);
+		goto coredump_collect_err;
+	}
+
+	/* Send coredump end cmd indicating end of coredump collection */
+	if (cd_list->entry == (IOSM_NOF_CD_REGION - 1))
+		ipc_coredump_get_list(ipc_devlink, rpsi_cmd_coredump_end);
+
+	return rc;

Return literals if possible.

	return 0;

Put a blank line after the "return 0;"

+coredump_collect_err:
+	ipc_coredump_get_list(ipc_devlink, rpsi_cmd_coredump_end);
+	return rc;
+}

[ snip ]

+/**
+ * struct iosm_rpsi_cmd - Structure for RPSI Command
+ * @param:      Used to calculate CRC
+ * @cmd:        Stores the RPSI command
+ * @crc:        Stores the CRC value
+ */
+struct iosm_rpsi_cmd {
+	union iosm_rpsi_param_u param;
+	__le16	cmd;
+	__le16	crc;
+};
+
+/**
+ * ipc_devlink_init - To initialize the devlink to IOSM driver
+ * @ipc_imem:	Pointer to struct iosm_imem
+ *
+ * Returns:	Pointer to iosm_devlink on success and NULL on failure
+ */

These function comments should go in the .c file instead of the .h file.

+struct iosm_devlink *ipc_devlink_init(struct iosm_imem *ipc_imem);
+
+/**
+ * ipc_devlink_deinit - To unintialize the devlink from IOSM driver.
+ * @ipc_devlink:	Devlink instance
+ */

Same.

+void ipc_devlink_deinit(struct iosm_devlink *ipc_devlink);
+
+/**
+ * ipc_devlink_send_cmd - Send command to Modem
+ * @ipc_devlink: Pointer to struct iosm_devlink
+ * @cmd:	 Command to be sent to modem
+ * @entry:	 Command entry number
+ *
+ * Returns:	 0 on success and failure value on error
+ */

Same.

+int ipc_devlink_send_cmd(struct iosm_devlink *ipc_devlink, u16 cmd, u32 entry);
+
+#endif /* _IOSM_IPC_DEVLINK_H */

regards,
dan carpenter