Re: [PATCH v6 2/4] SFH: PCIe driver to add support of AMD sensor fusion

From: Randy Dunlap <rdunlap@infradead.org>
To: Sandeep Singh <Sandeep.Singh@amd.com>,
	jikos@kernel.org, benjamin.tissoires@redhat.com,
	linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
	srinivas.pandruvada@linux.intel.com, jic23@kernel.org,
	linux-iio@vger.kernel.org, hdegoede@redhat.com,
	Nehal-bakulchandra.Shah@amd.com, andy.shevchenko@gmail.com,
	mail@richard-neumann.de, m.felsch@pengutronix.de
Cc: Shyam-sundar.S-k@amd.com
Subject: Re: [PATCH v6 2/4] SFH: PCIe driver to add support of AMD sensor fusion
Date: Sun, 9 Aug 2020 06:59:14 -0700	[thread overview]
Message-ID: <3ef30a57-08b4-5908-b13e-c1a52cdc135d@infradead.org> (raw)
In-Reply-To: <20200809102511.2657644-3-Sandeep.Singh@amd.com>

On 8/9/20 3:25 AM, Sandeep Singh 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
> 
> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
> Signed-off-by: Sandeep Singh <sandeep.singh@amd.com>
> ---
>  drivers/hid/Kconfig                    |   2 +
>  drivers/hid/Makefile                   |   2 +
>  drivers/hid/amd-sfh-hid/Kconfig        |  21 ++++
>  drivers/hid/amd-sfh-hid/Makefile       |  15 +++
>  drivers/hid/amd-sfh-hid/amd_mp2_pcie.c | 162 +++++++++++++++++++++++++
>  drivers/hid/amd-sfh-hid/amd_mp2_pcie.h |  83 +++++++++++++
>  6 files changed, 285 insertions(+)
>  create mode 100644 drivers/hid/amd-sfh-hid/Kconfig
>  create mode 100644 drivers/hid/amd-sfh-hid/Makefile
>  create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.c
>  create mode 100644 drivers/hid/amd-sfh-hid/amd_mp2_pcie.h

Hi,

> diff --git a/drivers/hid/amd-sfh-hid/Kconfig b/drivers/hid/amd-sfh-hid/Kconfig
> new file mode 100644
> index 000000000000..e73cf9fe1324
> --- /dev/null
> +++ b/drivers/hid/amd-sfh-hid/Kconfig
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +menu "AMD SFH HID support"
> +	depends on X86_64 || COMPILE_TEST
> +	depends on PCI
> +
> +config AMD_SFH_HID
> +	tristate "AMD Sensor Fusion Hub"
> +	select HID

How about
	depends on HID

We try hard not to select/enable entire subsystems just because one driver
wants it.

Also, HID depends on INPUT, so it's not safe to select HID unless INPUT is
already enabled.

> +	help
> +	If you say yes to this option, support will be included for the AMD
> +	Sensor Fusion Hub.
> +	This driver will enable sensors functionality to user through HID
> +	framework. Basically this driver will get data from MP2 FW

s/FW/firmware/
or is it "framework" ?

> +	and provide that data to HID framework.
> +	MP2 which is an ARM® Cortex-M4 core based co-processor to x86.
> +
> +	This driver can also be built as modules. If so, the modules will

	                        built as a module. If so, the module will

> +	be  called amd-sfhtp-hid.
> +	Say Y or M here if you want to support AMD SFH. If unsure, say N.
> +
> +endmenu

thanks.
-- 
~Randy