Date:   Tue, 11 Aug 2020 15:54:51 +0200
From:   Marco Felsch <m.felsch@...gutronix.de>
To:     Sandeep Singh <Sandeep.Singh@....com>
Cc:     jikos@...nel.org, benjamin.tissoires@...hat.com,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        srinivas.pandruvada@...ux.intel.com, jic23@...nel.org,
        linux-iio@...r.kernel.org, hdegoede@...hat.com,
        Nehal-bakulchandra.Shah@....com, andy.shevchenko@...il.com,
        mail@...hard-neumann.de, rdunlap@...radead.org,
        Shyam-sundar.S-k@....com
Subject: Re: [PATCH v7 2/4] SFH: PCIe driver to add support of AMD sensor
 fusion hub

Hi Sandeep,

On 20-08-10 21:30, Sandeep Singh wrote:
> From: Sandeep Singh <sandeep.singh@....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 Firmware and provide that data into DRAM

IMO we should reword the commit message since it is a bit odd.

> diff --git a/drivers/hid/amd-sfh-hid/Makefile b/drivers/hid/amd-sfh-hid/Makefile
> new file mode 100644
> index 000000000000..a163c7f62b32
> --- /dev/null
> +++ b/drivers/hid/amd-sfh-hid/Makefile
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Makefile - AMD SFH HID drivers
> +# Copyright (c) 2019-2020, Advanced Micro Devices, Inc.
> +#
> +#
> +
> +ccflags-m := -Werror

Hm.. is this really needed? Don't get me wrong I'm a fan of no-warnings
but only a few drivers set this flag.

> +   obj-$(CONFIG_AMD_SFH_HID) +=amd-sfhtp-hid.o
> +   amd-sfhtp-hid-objs := amdsfh_hid.o
> +   amd-sfhtp-hid-objs+= amdsfh_hid_client.o
> +   amd-sfhtp-hid-objs+= amd_mp2_pcie.o
> +   amd-sfhtp-hid-objs+= hid_descriptor/amd_sfh_hid_descriptor.o

IMHO a patch should be self-contained. By that I mean that if uspstream
apply just that patch it won't break things. This isn't the case here
since you already added the support the other driver parts.

Also please check indentation and whitespaces.

> +
> +ccflags-y += -I$(srctree)/$(src)/
> diff --git a/drivers/hid/amd-sfh-hid/amd_mp2_pcie.c b/drivers/hid/amd-sfh-hid/amd_mp2_pcie.c
> new file mode 100644
> index 000000000000..898157f4240b
> --- /dev/null
> +++ b/drivers/hid/amd-sfh-hid/amd_mp2_pcie.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * AMD MP2 PCIe communication driver
> + * Copyright 2020 Advanced Micro Devices, Inc.
> + *
> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@....com>
> + *	    Sandeep Singh <Sandeep.singh@....com>
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/types.h>
> +#include "amd_mp2_pcie.h"
> +
> +#define DRIVER_NAME	"pcie_mp2_amd"
> +#define DRIVER_DESC	"AMD(R) PCIe MP2 Communication Driver"
> +
> +#define ACEL_EN		BIT(accel_idx)
> +#define GYRO_EN		BIT(gyro_idx)
> +#define MAGNO_EN	BIT(mag_idx)
> +#define ALS_EN		BIT(als_idx)
> +
> +void amd_start_sensor(struct amd_mp2_dev *privdata, struct amd_mp2_sensor_info info)
> +{
> +	union sfh_cmd_param cmd_param;
> +	union sfh_cmd_base cmd_base;
> +
> +	/* fill up command register */
> +	cmd_base.ul = 0;

Why not memset()? Please see my below comments on the header.

> +	cmd_base.s.cmd_id = enable_sensor;
> +	cmd_base.s.period = info.period;
> +	cmd_base.s.sensor_id = info.sensor_idx;
> +
> +	/* fill up command param register */
> +	cmd_param.ul = 0;
> +	cmd_param.s.buf_layout = 1;
> +	cmd_param.s.buf_length = 16;
> +
> +	writeq(info.phys_address, privdata->mmio + AMD_C2P_MSG2);
> +	writel(cmd_param.ul, privdata->mmio + AMD_C2P_MSG1);
> +	writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
> +}
> +
> +void amd_stop_sensor(struct amd_mp2_dev *privdata, u16 sensor_idx)

Why has amd_stop_sensor() a complete different API than
amd_start_sensor(). IMHO I would keep it in sync. Also u16 sensor_idx is
to large. Just use the struct amd_mp2_sensor_info.

> +{
> +	union sfh_cmd_base cmd_base;
> +
> +	/* fill up command register */
> +	cmd_base.ul = 0;
> +	cmd_base.s.cmd_id = disable_sensor;
> +	cmd_base.s.period = 0;
> +	cmd_base.s.sensor_id = sensor_idx;
> +
> +	writeq(0x0, privdata->mmio + AMD_C2P_MSG2);
> +	writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
> +}
> +
> +void amd_stop_all_sensors(struct amd_mp2_dev *privdata)
> +{
> +	union sfh_cmd_base cmd_base;
> +
> +	/* fill up command register */
> +	cmd_base.ul = 0;
> +	cmd_base.s.cmd_id = stop_all_sensors;
> +	cmd_base.s.period = 0;
> +	cmd_base.s.sensor_id = 0;
> +
> +	writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG0);
> +}
> +
> +int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id)
> +{

Nit: Why not amd_get_sensor_num() to keep naming scheme? Also I would
keep the API and make use of 'struct amd_mp2_sensor_info'.

> +	int activestatus, num_of_sensors = 0;
> +
> +	if (!sensor_id)
> +		return -EINVAL;
> +
> +	privdata->activecontrolstatus = readl(privdata->mmio + AMD_P2C_MSG3);
> +	activestatus = privdata->activecontrolstatus >> 4;

Magical shift here? Please make use of FIELD_GET().

> +	if (ACEL_EN  & activestatus)
> +		sensor_id[num_of_sensors++] = accel_idx;
> +
> +	if (GYRO_EN & activestatus)
> +		sensor_id[num_of_sensors++] = gyro_idx;
> +
> +	if (MAGNO_EN & activestatus)
> +		sensor_id[num_of_sensors++] = mag_idx;
> +
> +	if (ALS_EN & activestatus)
> +		sensor_id[num_of_sensors++] = als_idx;
> +
> +	return num_of_sensors;
> +}
> +
> +static int amd_mp2_pci_init(struct amd_mp2_dev *privdata, struct pci_dev *pdev)
> +{
> +	int rc;
> +
> +	pci_set_drvdata(pdev, privdata);
> +	rc = pcim_enable_device(pdev);
> +	if (rc)
> +		return rc;
> +	pcim_iomap_regions(pdev, BIT(2), DRIVER_NAME);
> +
> +	privdata->mmio = pcim_iomap_table(pdev)[2];
> +	pci_set_master(pdev);
> +
> +	rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
> +	if (rc)
> +		rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32));
> +	return rc;
> +}
> +
> +static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct amd_mp2_dev *privdata;
> +	int rc;
> +
> +	privdata = devm_kzalloc(&pdev->dev, sizeof(*privdata), GFP_KERNEL);
> +	if (!privdata)
> +		return -ENOMEM;
> +	privdata->pdev = pdev;

You don't need the pdev in this driver, why do you store it?

> +	rc = amd_mp2_pci_init(privdata, pdev);
> +	if (rc)
> +		return rc;
> +	rc = amd_sfh_hid_client_init(privdata);

As I said, this is not self-contained.

> +	if (rc)
> +		return rc;
> +	return 0;
> +}
> +
> +static void amd_mp2_pci_remove(struct pci_dev *pdev)
> +{
> +	struct amd_mp2_dev *privdata = pci_get_drvdata(pdev);
> +
> +	amd_sfh_hid_client_deinit(privdata);
> +	amd_stop_all_sensors(privdata);

This actions can be done in a devm_add_action_or_reset() and avoids
the remove() callback.

> +}
> +
> +static const struct pci_device_id amd_mp2_pci_tbl[] = {
> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_MP2) },
> +	{},

Missing whitespace:
        { },
> +};
> +MODULE_DEVICE_TABLE(pci, amd_mp2_pci_tbl);
> +
> +static struct pci_driver amd_mp2_pci_driver = {
> +	.name		= DRIVER_NAME,
> +	.id_table	= amd_mp2_pci_tbl,
> +	.probe		= amd_mp2_pci_probe,
> +	.remove		= amd_mp2_pci_remove,
> +};
> +module_pci_driver(amd_mp2_pci_driver);
> +
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_AUTHOR("Shyam Sundar S K <Shyam-sundar.S-k@....com>");
> +MODULE_AUTHOR("Sandeep Singh <Sandeep.singh@....com>");
> diff --git a/drivers/hid/amd-sfh-hid/amd_mp2_pcie.h b/drivers/hid/amd-sfh-hid/amd_mp2_pcie.h
> new file mode 100644
> index 000000000000..a4ef604c4fe8
> --- /dev/null
> +++ b/drivers/hid/amd-sfh-hid/amd_mp2_pcie.h
> @@ -0,0 +1,83 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * AMD MP2 PCIe communication driver
> + * Copyright 2020 Advanced Micro Devices, Inc.
> + * Authors: Shyam Sundar S K <Shyam-sundar.S-k@....com>
> + *	    Sandeep Singh <Sandeep.singh@....com>
> + */
> +
> +#ifndef PCIE_MP2_AMD_H
> +#define PCIE_MP2_AMD_H
> +
> +#include <linux/pci.h>
> +#include <linux/types.h>
> +
> +#define PCI_DEVICE_ID_AMD_MP2	0x15E4
> +
> +/* MP2 C2P Message Registers */
> +#define AMD_C2P_MSG0	0x10500
> +#define AMD_C2P_MSG1	0x10504
> +#define AMD_C2P_MSG2	0x10508
> +
> +/* MP2 P2C Message Registers */
> +#define AMD_P2C_MSG3	0x1068C /* Supported Sensors info */
> +
> +/* SFH Command register */
> +union sfh_cmd_base {
> +	u32 ul;
> +	struct {
> +		u32 cmd_id : 8;
> +		u32 sensor_id : 8;
> +		u32 period : 16;
> +	} s;
> +};

Why not just:
struct sfh_cmd_base {
	u8 cmd_id;
	u8 sensor_id;
	u16 period;
};

Please use the native data types. Furthermore the code in this driver
assumes that the host(x86) and client(ARM) are using the same
byte-order. I know that ARM is LE too but maybe this changes on newer
ZEN-CPU's.

> +union sfh_cmd_param {
> +	u32 ul;
> +	struct {
> +		u32 buf_layout : 2;
> +		u32 buf_length : 6;
> +		u32 rsvd : 24;
> +	} s;
> +};

This union is only used by amd_start_sensor() and the below struct which
is used nowhere. I would rather drop this here and prepare the
buf_layout and buf_length by FIELD_PREP().

> +struct sfh_cmd_reg {
> +	union sfh_cmd_base cmd_base;
> +	union sfh_cmd_param cmd_param;
> +	phys_addr_t phys_addr;
> +};

This struct is gets never used here.

> +
> +enum command_id {
> +	enable_sensor = 1,
> +	disable_sensor = 2,
> +	stop_all_sensors = 8,
> +	invalid_cmd = 0xf
> +};

Please use CAPITAL_NAMES, e.g.:

enum command_id {
	AMD_MP2_SENSOR_ENABLE = 1,
	AMD_MP2_SENSOR_DISABLE = 2,
	AMD_MP2_SENSOR_STOP_ALL = 8,
}

or use #define.

> +
> +enum sensor_idx {
> +	accel_idx = 0,
> +	gyro_idx = 1,
> +	mag_idx = 2,
> +	als_idx = 19
> +};

Same here.

> +
> +struct amd_mp2_dev {
> +	struct pci_dev *pdev;

pdev is never used.

> +	struct amdtp_cl_data *cl_data;

cl_data gets never set -> useless?

> +	void __iomem *mmio;
> +	u32 activecontrolstatus;

The activecontrolstatus member is also never used.
> +};
> +
> +struct amd_mp2_sensor_info {
> +	u8 sensor_idx;
> +	u32 period;

The sfh_cmd_base defines it as u16.

> +	phys_addr_t phys_address;
> +};
> +
> +void amd_start_sensor(struct amd_mp2_dev *privdata, struct amd_mp2_sensor_info info);
> +void amd_stop_sensor(struct amd_mp2_dev *privdata, u16 sensor_idx);
> +void amd_stop_all_sensors(struct amd_mp2_dev *privdata);

> +int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id);

I would add this 

> +int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata);
> +int amd_sfh_hid_client_deinit(struct amd_mp2_dev *privdata);

Why are those functions included here?

Regards,
  Marco

> +#endif
> -- 
> 2.25.1

Powered by blists - more mailing lists