Messages in this thread | | | Subject | Re: [PATCH v4 2/4] SFH: PCI driver to add support of AMD sensor fusion Hub using HID framework | From | "Singh, Sandeep" <> | Date | Mon, 30 Mar 2020 10:42:50 +0530 |
| |
Thanks Andy for review comments i will look into it.
On 3/27/2020 8:25 PM, Andy Shevchenko wrote: > [CAUTION: External Email] > > On Thu, Feb 27, 2020 at 7:01 AM Sandeep Singh <Sandeep.Singh@amd.com> wrote: >> From: Sandeep Singh <sandeep.singh@amd.com> >> >> 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
Regards
Sandeep
| |