Re: [PATCH 1/3] mctp pcc: Implement MCTP over PCC Transport

+struct mctp_pcc_hdr {
+	u32 signature;
+	u32  flags;

There looks to be an extra space here, or a tab vs space issue.

+	u32 length;
+	char mctp_signature[4];
+};
+
+struct mctp_pcc_packet {
+	struct mctp_pcc_hdr pcc_header;
+	union {
+		struct mctp_hdr     mctp_header;

and more here. I would expect checkpatch to point these out.

+struct mctp_pcc_hw_addr {
+	int inbox_index;
+	int outbox_index;
+};
+	physical_link_addr.inbox_index =
+		htonl(mctp_pcc_dev->hw_addr.inbox_index);

These are {in|out}box_index are u32s right? Otherwise you would not be
using htonl() on them. Maybe specify the type correctly.

+	physical_link_addr.outbox_index =
+		htonl(mctp_pcc_dev->hw_addr.outbox_index);

You should also mark the physical_link_addr members as being big
endian so sparse can check you are not missing any byte swaps.

+	dev_addr_set(ndev, (const u8 *)&physical_link_addr);
+	rc = register_netdev(ndev);
+	if (rc)
+		goto cleanup_in_channel;
+	list_add_tail(&mctp_pcc_dev->head, &mctp_pcc_ndevs);
+	return 0;
+cleanup_in_channel:

It would be normal to add a blink line after the return, just to make
it easier to see where the error cleanup code starts.

+	mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
+cleanup_out_channel:
+	mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
+free_netdev:
+	unregister_netdev(ndev);

Can you get here with the ndev actually registered?

+static acpi_status lookup_pcct_indices(struct acpi_resource *ares, void *context)
+{
+	struct acpi_resource_address32 *addr;
+	struct lookup_context *luc = context;
+
+	switch (ares->type) {
+	case 0x0c:
+	case 0x0a:

Please replace these magic numbers of #defines.

+static int mctp_pcc_driver_add(struct acpi_device *adev)
+{
+	int inbox_index;
+	int outbox_index;
+	acpi_handle dev_handle;
+	acpi_status status;
+	struct lookup_context context = {0, 0, 0};
+
+	dev_info(&adev->dev, "Adding mctp_pcc device for HID  %s\n", acpi_device_hid(adev));

It would be better to not spam the logs when a driver probes, unless
there is an actual error.

+	dev_handle = acpi_device_handle(adev);
+	status = acpi_walk_resources(dev_handle, "_CRS", lookup_pcct_indices, &context);
+	if (ACPI_SUCCESS(status)) {
+		inbox_index = context.inbox_index;
+		outbox_index = context.outbox_index;
+		return create_mctp_pcc_netdev(adev, &adev->dev, inbox_index, outbox_index);
+	}
+	dev_err(&adev->dev, "FAILURE to lookup PCC indexes from CRS");
+	return -EINVAL;
+};
+
+/* pass in adev=NULL to remove all devices
+ */
+static void mctp_pcc_driver_remove(struct acpi_device *adev)
+{
+	struct mctp_pcc_ndev *mctp_pcc_dev = NULL;
+	struct list_head *ptr;
+	struct list_head *tmp;
+
+	list_for_each_safe(ptr, tmp, &mctp_pcc_ndevs) {
+		mctp_pcc_dev = list_entry(ptr, struct mctp_pcc_ndev, head);
+		if (!adev || mctp_pcc_dev->acpi_device == adev) {
+			struct net_device *ndev;
+
+			mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->out_chan);
+			mctp_pcc_dev->cleanup_channel(mctp_pcc_dev->in_chan);
+			ndev = mctp_pcc_dev->mdev.dev;
+			if (ndev)
+				mctp_unregister_netdev(ndev);
+			list_del(ptr);
+			if (adev)
+				break;
+		}
+	}
+};
+
+static const struct acpi_device_id mctp_pcc_device_ids[] = {
+	{ "DMT0001", 0},
+	{ "", 0},
+};
+
+static struct acpi_driver mctp_pcc_driver = {
+	.name = "mctp_pcc",
+	.class = "Unknown",
+	.ids = mctp_pcc_device_ids,
+	.ops = {
+		.add = mctp_pcc_driver_add,
+		.remove = mctp_pcc_driver_remove,
+		.notify = NULL,
+	},
+	.owner = THIS_MODULE,
+
+};
+
+static int __init mctp_pcc_mod_init(void)
+{
+	int rc;
+
+	pr_info("initializing MCTP over PCC\n");

More useless log spamming... pr_dbg(), or remove altogether.

	Andrew