Hi

On 2/14/2020 10:10 AM, Srinivas Pandruvada wrote:

On Thu, 2020-02-13 at 15:56 +0100, Benjamin Tissoires wrote:

quoted

Hi,

On Wed, Feb 12, 2020 at 3:45 PM Hans de Goede [off-list ref]
wrote:

quoted

Hi,

On 2/12/20 3:56 AM, Sandeep Singh wrote:

quoted

From: Sandeep Singh <redacted>

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.

Thanks for the valuable input.

quoted

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_READ
Y_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_MU
LTIPLIER;
+               acc_input.in_accel_y_value =
(int)sensor_virt_addr[1] /
+                                               AMD_SFH_FIRMWARE_MU
LTIPLIER;
+               acc_input.in_accel_z_value
=  (int)sensor_virt_addr[2] /
+                                               AMD_SFH_FIRMWARE_MU
LTIPLIER;
+               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?
Hard to say without knowing AMD roadmap for that. If they intend to
have an ISH-like approach in the end with reports and descriptors
provided by the firmwares, then it makes sense to keep this
architecture for the first revision of devices.
If not, then yes, this is probably overkill compared to what needs to
be done.

I suggested this approach to follow something like Chrome-OS EC based
hub, but looks like in longer run this may come from firmware. That's
why they may have decided.

Thanks,
Srinivas
 

quoted

Sandeep, can you explain to us why you think using HID is the best
way?

On a side note, I don't necessarily like patch 4/5 with the debugfs
interface. It's adding a kernel API for no gain, and we should
already
have the debug API available in the various subsystems involved.

Cheers,
Benjamin

Yes today, the  HID Reports are getting generated in driver. But, we would like to have HID based driver as we may go for HID based firmware in future . Hence keeping that in mind current AMD SFH design.

So, kindly consider our design w.r.t HID for this patch series.

For the debugfs part,currently it is really handy for us to debug raw values coming from firmware.But if guys feel that it is not necessary, we can remove it.

Thanks
Nehal Shah