[CAUTION: External Email]
On Thu, Feb 27, 2020 at 7:01 AM Sandeep Singh <Sandeep.Singh@xxxxxxx> wrote:
From: Sandeep Singh <sandeep.singh@xxxxxxx>
AMD SFH uses HID over PCIe bus.SFH fw is part of MP2
processor (MP2 which is an ARM® Cortex-M4 core based
co-processor to x86) and it runs on MP2 where in driver resides
on X86. This part of module will communicate with MP2 FW and
provide that data into DRAM
You asked for review, here you are.
TL,DR; it requires a bit of work.
...
+ depends on (X86_64 || COMPILE_TEST) && PCI
For better maintenance
depends on X86_64 || COMPILE_TEST
depends on PCI
...
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile - AMD SFH HID drivers
+# Copyright (c) 2020-2021, Advanced Micro Devices, Inc.
+#
+#
Perhaps simple blank line instead?
+ccflags-y += -I$(srctree)/$(src)/
Why?
...
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
Keep in order?
+ blank line?
Missed bits.h, types.h.
+#include "amd_mp2_pcie.h"
...
+ write64((u64)info.phy_address, privdata->mmio + AMD_C2P_MSG2);
Why explicit cast?
...
+ /*fill up command register */
Space is missed.
...
+ if (!sensor_id)
+ return -ENOMEM;
I can say -EINVAL as per its definition, but why do you need this at all?
...
+static int amd_mp2_pci_init(struct amd_mp2_dev *privdata, struct pci_dev *pdev)
+{
+ int rc;
+ int bar_index = 2;
+ resource_size_t size, base;
+ pci_set_drvdata(pdev, privdata);
Better to assign when you are sure (to some extend in both of them):
a) it's needed
b) driver is going to be probed correctly
...
+ rc = pcim_iomap_regions(pdev, 1 >> 2, DRIVER_NAME);
What 1 >> 2 means? Shouldn't be simple BIT(2)?
How was this been tested?
+ if (rc)
+ goto err_pci_regions;
...
+ base = pci_resource_start(pdev, bar_index);
+ size = pci_resource_len(pdev, bar_index);
+ dev_dbg(ndev_dev(privdata), "Base addr:%llx size:%llx\n",
+ (unsigned long long)base, (unsigned long long)size);
Read printk-formats.rst.
Now, when you get familiar with it, find proper specifier and drop
these ugly castings.
But wait, why do you need this? `dmesg` will show it anyway during
boot / hotplug event time.
...
+ privdata->mmio = ioremap(base, size);
+ if (!privdata->mmio) {
+ rc = -EIO;
+ goto err_dma_mask;
+ }
Why?!
...
+err_dma_mask:
+ pci_clear_master(pdev);
+err_pci_regions:
+ pci_disable_device(pdev);
Are you using devres or not? Please, be consistent.
+err_pci_enable:
+ pci_set_drvdata(pdev, NULL);
I think it's some like 5 to 10 years that we don't need this.
+ return rc;
+}
...
+ pci_iounmap(pdev, privdata->mmio);
+
+ pci_clear_master(pdev);
+ pci_disable_device(pdev);
+ pci_set_drvdata(pdev, NULL);
Ditto as above two comments.
...
+ dev_info(&pdev->dev, "MP2 device found [%04x:%04x] (rev %x)\n",
+ (int)pdev->vendor, (int)pdev->device, (int)pdev->revision);
Oh, if you use explicit casting for printf(), 99.9% you are doing
something wrong (in kernel space).
On top of that, why this noise is here?
...
+ privdata = devm_kzalloc(&pdev->dev, sizeof(*privdata), GFP_KERNEL);
+
No need for this blank line.
+ if (!privdata) {
+ }
...
+ rc = amd_mp2_pci_init(privdata, pdev);
+ if (rc)
+ goto err_pci_init;
+ return 0;
Why its content can't be simple here? I.o.w. why this function is needed?
...
+err_pci_init:
+ return rc;
+err_dev:
+ return rc;
Completely useless code.
+}
...
+static const struct pci_device_id amd_mp2_pci_tbl[] = {
+ {PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_MP2)},
+ {0}
0 is not needed, but it's up to you.
+};
...
+static int __init amd_mp2_pci_driver_init(void)
+{
+ return pci_register_driver(&amd_mp2_pci_driver);
+}
+module_init(amd_mp2_pci_driver_init);
+
+static void __exit amd_mp2_pci_driver_exit(void)
+{
+ pci_unregister_driver(&amd_mp2_pci_driver);
+}
+module_exit(amd_mp2_pci_driver_exit);
module_pci_driver()
We have it for years.
...
+#include <linux/pci.h>
I don't see users of it, but missed headers
types.h
...
+#define PCI_DEVICE_ID_AMD_MP2 0x15E4
Why it's not in C file?
...
+#define AMD_P2C_MSG0 0x10680 /*Do not use*/
+#define AMD_P2C_MSG1 0x10684
+#define AMD_P2C_MSG2 0x10688
+#define AMD_P2C_MSG3 0x1068C /*MP2 debug info*/
+#define AMD_P2C_MSG_INTEN 0x10690 /*MP2 int gen register*/
+#define AMD_P2C_MSG_INTSTS 0x10694 /*Interrupt sts*/
Missed spaces.
...
+#define write64 amdsfh_write64
+static inline void amdsfh_write64(u64 val, void __iomem *mmio)
+{
+ writel(val, mmio);
+ writel(val >> 32, mmio + sizeof(u32));
+}
NIH of lo_hi_writeq().
+#define read64 amdsfh_read64
+static inline u64 amdsfh_read64(void __iomem *mmio)
+{
+ u64 low, high;
+
+ low = readl(mmio);
+ high = readl(mmio + sizeof(u32));
+ return low | (high << 32);
+}
NIH of lo_hi_readq().
...
+struct sfh_command_register {
+ union sfh_cmd_base cmd_base;
+ union sfh_command_parameter cmd_param;
+ phys_addr_t phy_addr;
Are you sure? This type is flexible. And by name of the struct I think
it operates with hardware, so, fix it accordingly.
+};
...
+enum response_type {
+ non_operationevent,
+ command_success,
+ command_failed,
+ sfi_dataready_event,
+ invalid_response = 0xff,
GENMASK()
+};
UPPER CASE?
+enum status_type {
+ cmd_success,
+ invalid_data_payload,
+ invalid_data_length,
+ invalid_sensor_id,
+ invalid_dram_addr,
+ invalid_command,
+ sensor_enabled,
+ sensor_disabled,
+ status_end,
+};
+
+enum command_id {
+ non_operation = 0,
+ enable_sensor = 1,
+ disable_sensor = 2,
+ dump_sensorinfo = 3,
+ numberof_sensordiscovered = 4,
+ who_am_i_regchipid = 5,
+ set_dcd_data = 6,
+ get_dcd_data = 7,
+ stop_all_sensors = 8,
+ invalid_cmd = 0xf,
+};
Ditto.
...
+enum sensor_idx {
Do you need names for enums like this?
+ ACCEL_IDX = 0,
+ GYRO_IDX = 1,
+ MAG_IDX = 2,
+ AMBIENT_LIGHT_IDX = 19,
+ NUM_ALL_SENSOR_CONSUMERS
+};
...
+struct amd_mp2_dev {
+ struct pci_dev *pdev;
+ void __iomem *mmio;
Header for __iomem?
+ struct delayed_work work;
Header for this?
+ void *ctx;
+ void *cl_data;
+};
...
+struct amd_mp2_sensor_info {
+ u8 sensor_idx;
+ u32 period;
+ phys_addr_t phy_address;
Same comment as per above use of phys_addr_t type.
+};
...
+#define ndev_pdev(ndev) ((ndev)->pdev)
+#define ndev_name(ndev) pci_name(ndev_pdev(ndev))
+#define ndev_dev(ndev) (&ndev_pdev(ndev)->dev)
Why? What's the benefit?
--
With Best Regards,
Andy Shevchenko
