Linux Input Archive on lore.kernel.org
 help / color / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
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, Nehal-bakulchandra.Shah@amd.com
Cc: Shyam-sundar.S-k@amd.com
Subject: Re: [PATCH v3 0/5] SFH: Add Support for AMD Sensor Fusion Hub
Date: Wed, 12 Feb 2020 15:45:17 +0100
Message-ID: <1ce6f591-1e8b-8291-7f18-48876fd70e10@redhat.com> (raw)
In-Reply-To: <1581476197-25854-1-git-send-email-Sandeep.Singh@amd.com>

Hi,

On 2/12/20 3:56 AM, Sandeep Singh wrote:
> From: Sandeep Singh <sandeep.singh@amd.com>
> 
> AMD SFH(Sensor Fusion Hub) is HID based driver.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.The driver functionalities are
> divided  into three parts:-
> 
> 1: amd-mp2-pcie:-       This module will communicate with MP2 FW and
>                          provide that data into DRAM.
> 2: Client driver :-     This part for driver will use dram data and
>                          convert that data into HID format based on
>                          HID reports.
> 3: Transport driver :-  This part of driver will communicate with
>                          HID core. Communication between devices and
>                          HID core is mostly done via HID reports
> 
> In terms of architecture it is much more reassembles like
> ISH(Intel Integrated Sensor Hub). However the major difference
> is all the hid reports are generated as part of kernel driver.
> AMD SFH driver taken reference from ISH in terms of
> design and functionalities at fewer location.
> 
> AMD sensor fusion Hub is part of a SOC 17h family based platforms.
> The solution is working well on several OEM products.
> AMD SFH uses HID over PCIe bus.

I started looking at this patch because of the phoronix' news item on it.

First of all I want to say that it is great that AMD is working on
getting the Sensor Fusion Hub supported on Linux and that you are
working on a driver for this.

But, I've taken a quick look, mainly at the
"[PATCH v3 5/5] SFH: Create HID report to Enable support of AMD sensor fusion Hub (SFH)"
patch.

AFAIK with the Intel ISH the sensor-hub itself is actually providing
HID descriptors and HID input reports.

Looking at the AMD code, that does not seem to be the case, it seems
the values come directly from the AMD sensor-hub without being in any
HID specific form, e.g.:

+u8 get_input_report(int sensor_idx, int report_id,
+		    u8 *input_report, u32 *sensor_virt_addr)
+{
+	u8 report_size = 0;
+	struct accel3_input_report acc_input;
+	struct gyro_input_report gyro_input;
+	struct magno_input_report magno_input;
+	struct als_input_report als_input;
+
+	if (!sensor_virt_addr || !input_report)
+		return report_size;
+
+	switch (sensor_idx) {
+	case ACCEL_IDX: /* accel */
+		acc_input.common_property.report_id = report_id;
+		acc_input.common_property.sensor_state =
+					HID_USAGE_SENSOR_STATE_READY_ENUM;
+		acc_input.common_property.event_type =
+				HID_USAGE_SENSOR_EVENT_DATA_UPDATED_ENUM;
+		acc_input.in_accel_x_value = (int)sensor_virt_addr[0] /
+						AMD_SFH_FIRMWARE_MULTIPLIER;
+		acc_input.in_accel_y_value = (int)sensor_virt_addr[1] /
+						AMD_SFH_FIRMWARE_MULTIPLIER;
+		acc_input.in_accel_z_value =  (int)sensor_virt_addr[2] /
+						AMD_SFH_FIRMWARE_MULTIPLIER;
+		memcpy(input_report, &acc_input, sizeof(acc_input));
+		report_size = sizeof(acc_input);
+		break;

And the descriptors are hardcoded in the driver so as to fake a HID
device.

So going through the HID subsystem seems like an unnecessary detour,
which just makes things needlessly complex and harder to debug
(and extend).

The HID devices which the current patch-set is creating ultimately
will result in a number of devices being created under

/sys/bus/iio/devices

And this are the devices which userspace uses to get the sensor data.

IMHO instead of going through the HID subsys the AMD Sensor Fusion Hub
driver should simply register 4 (*) iio-devices itself and directly
pass the data through at the iio subsys level rather then going the
long way around by creating a fake HID device which then gets
attached to by the hid-sensor driver to ultimately create the same
iio-devices.

There are examples of e.g. various iio accel drivers under:
drivers/iio/accel/ you could start with a simple driver supporting
just the accelerometer bits and then extend things from there.

Benjamin, Jiri, Jonathan, what is your take on this?

Regards,

Hans


*) One for accel, gyra, magneto and light each


> Sandeep Singh (5):
>    SFH: Add maintainers and documentation for AMD SFH based on HID
>      framework
>    SFH: PCI driver to add support of AMD sensor fusion Hub using HID
>      framework
>    SFH: Transport Driver to add support of AMD Sensor Fusion Hub (SFH)
>    SFH: Add debugfs support to AMD Sensor Fusion Hub
>    SFH: Create HID report to Enable support of AMD sensor fusion Hub
>      (SFH)
> 
> Changes since v1:
>          -Fix auto build test warnings
>          -Fix warnings captured using smatch
>          -Changes suggested by Dan Carpenter
> 
> Links of the review comments for v1:
>          [1] https://patchwork.kernel.org/patch/11325163/
>          [2] https://patchwork.kernel.org/patch/11325167/
>          [3] https://patchwork.kernel.org/patch/11325171/
>          [4] https://patchwork.kernel.org/patch/11325187/
> 
> 
> Changes since v2:
>          -Debugfs divided into another patch
>          -Fix some cosmetic changes
>          -Fix for review comments
>           Reported and Suggested by:-  Srinivas Pandruvada
> 
> Links of the review comments for v2:
>          [1] https://patchwork.kernel.org/patch/11355491/
>          [2] https://patchwork.kernel.org/patch/11355495/
>          [3] https://patchwork.kernel.org/patch/11355499/
>          [4] https://patchwork.kernel.org/patch/11355503/
> 
> 
>   Documentation/hid/amd-sfh-hid.rst                  | 160 +++++
>   MAINTAINERS                                        |   8 +
>   drivers/hid/Kconfig                                |   2 +
>   drivers/hid/Makefile                               |   1 +
>   drivers/hid/amd-sfh-hid/Kconfig                    |  20 +
>   drivers/hid/amd-sfh-hid/Makefile                   |  17 +
>   drivers/hid/amd-sfh-hid/amd_mp2_pcie.c             | 243 ++++++++
>   drivers/hid/amd-sfh-hid/amd_mp2_pcie.h             | 177 ++++++
>   drivers/hid/amd-sfh-hid/amdsfh-debugfs.c           | 250 ++++++++
>   drivers/hid/amd-sfh-hid/amdsfh-debugfs.h           |  14 +
>   drivers/hid/amd-sfh-hid/amdsfh-hid-client.c        | 260 +++++++++
>   drivers/hid/amd-sfh-hid/amdsfh-hid.c               | 179 ++++++
>   drivers/hid/amd-sfh-hid/amdsfh-hid.h               |  85 +++
>   .../hid_descriptor/amd_sfh_hid_descriptor.c        | 275 +++++++++
>   .../hid_descriptor/amd_sfh_hid_descriptor.h        | 125 ++++
>   .../hid_descriptor/amd_sfh_hid_report_descriptor.h | 642 +++++++++++++++++++++
>   16 files changed, 2458 insertions(+)
>   create mode 100644 Documentation/hid/amd-sfh-hid.rst
>   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
>   create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-debugfs.c
>   create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-debugfs.h
>   create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid-client.c
>   create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid.c
>   create mode 100644 drivers/hid/amd-sfh-hid/amdsfh-hid.h
>   create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_descriptor.c
>   create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_descriptor.h
>   create mode 100644 drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_descriptor.h
> 


  parent reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-12  2:56 Sandeep Singh
2020-02-12  2:56 ` [PATCH v3 1/5] SFH: Add maintainers and documentation for AMD SFH based on HID framework Sandeep Singh
2020-02-12  2:56 ` [PATCH v3 2/5] SFH: PCI driver to add support of AMD sensor fusion Hub using " Sandeep Singh
2020-02-12  2:56 ` [PATCH v3 3/5] SFH: Transport Driver to add support of AMD Sensor Fusion Hub (SFH) Sandeep Singh
2020-02-12  2:56 ` [PATCH v3 4/5] SFH: Add debugfs support to AMD Sensor Fusion Hub Sandeep Singh
2020-02-12  2:56 ` [PATCH v3 5/5] SFH: Create HID report to Enable support of AMD sensor fusion Hub (SFH) Sandeep Singh
2020-02-12 14:45 ` Hans de Goede [this message]
2020-02-13 14:56   ` [PATCH v3 0/5] SFH: Add Support for AMD Sensor Fusion Hub Benjamin Tissoires
2020-02-14  4:40     ` Srinivas Pandruvada
2020-02-14 10:04       ` Shah, Nehal-bakulchandra
2020-02-14 11:11         ` Benjamin Tissoires
2020-02-19 15:04           ` Shah, Nehal-bakulchandra
2020-02-21 12:47             ` Jiri Kosina
2020-02-14 11:11         ` Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1ce6f591-1e8b-8291-7f18-48876fd70e10@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=Nehal-bakulchandra.Shah@amd.com \
    --cc=Sandeep.Singh@amd.com \
    --cc=Shyam-sundar.S-k@amd.com \
    --cc=benjamin.tissoires@redhat.com \
    --cc=jic23@kernel.org \
    --cc=jikos@kernel.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=srinivas.pandruvada@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git