Re: [PATCH v4 2/4] SFH: PCI driver to add support of AMD sensor fusion Hub using HID framework
From: Andy Shevchenko
Date: Fri Mar 27 2020 - 10:55:32 EST
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