Received: by 2002:a25:1506:0:0:0:0:0 with SMTP id 6csp6141740ybv; Wed, 12 Feb 2020 06:47:26 -0800 (PST) X-Google-Smtp-Source: APXvYqwoqU9VMhf5r/gcejB6arhnsdl2hrjnRxItQGbu8AaCi3Btvc62PmKVwNnW5ntASYfYJBq6 X-Received: by 2002:a05:6830:18ce:: with SMTP id v14mr9283113ote.36.1581518846519; Wed, 12 Feb 2020 06:47:26 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1581518846; cv=none; d=google.com; s=arc-20160816; b=Bbr2jpatRmLcqTnzFCsm1bkOOJwHL24a3lCK3Du56zUD8/TuXZzp6tLcNPsCYuPt2C QyvvklGirZBEd5cknfPaLvvJME/CHA2OoIgsCXDy3ZvfzMtd95mlyF2cBdqOiztMqNTk 7tWz7/B9rVm+s9m8F6ybww8wHz2DRoK0JN6zkqUDhS6mmLqy6Pv41tAlWU2ApA2UenNK d3QSLNwG+QB6cpTyEwNiubdZXZAmxmfRSBngTpUL0ZUfAMGX/v2wirQVMQ6YYBT7CgUl UA+zLymPDJK8yCb3ch1BGtvDkWhtBprzzHvHcPafvkb0qosghWu3NCYGoXhvNvCAk5Ip ABAw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:references:cc:to:subject:from:dkim-signature; bh=5yxu7FAFWwnIWX0SIGmGmSoK4AorTJs9H07QQ92CKAE=; b=aeSaFApovHeSkTXkkDfY/lY1pPIkKRHf+QAE//ZuxRHeykyt6S9/+GVmMtZDP72MR4 dg2n8+FBxO8p+ufx+xxagLhFwvgu5N5Qo5rt+tokkDuFwZgZwuCDdl60uE3FsCHaVjFj Pv2rFcBeBaVim4WsG5RhzJVLIdwCVzTAROapR73qoMibt+DzvFo4B+ojkW7PTHC5Pyzw W0PaHopyb7HeMbbVqZ0OmY9XXai3rrYXBJsZakMJFqdhvEDiwlgrF/5XlwBji/pG1zrD r8Vwg03H3oBobT2N8dnW5SyKC2mvrXeOgb+812GAc7JXlF/gcq4PVKF4Cx0d6WD4BO+l S6iQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=i4MtEetD; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id w9si311743otk.91.2020.02.12.06.47.03; Wed, 12 Feb 2020 06:47:26 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=i4MtEetD; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728294AbgBLOpa (ORCPT + 99 others); Wed, 12 Feb 2020 09:45:30 -0500 Received: from us-smtp-delivery-1.mimecast.com ([205.139.110.120]:45323 "EHLO us-smtp-1.mimecast.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728207AbgBLOp3 (ORCPT ); Wed, 12 Feb 2020 09:45:29 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1581518728; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5yxu7FAFWwnIWX0SIGmGmSoK4AorTJs9H07QQ92CKAE=; b=i4MtEetD5RhwTIUKPQVuzOY0P9ku3syFHu78K3hsLtI6kFkNrGY4N5pJM8RBCIzTNn/c1J g7+cwzj3VCEpqBwd/GHsr68sgPiqgLvM0QvNSYvkxVp5z4JDdxic0bvLOaU9UBBac4rndb mbHOD53kotEi3jcEcI0AnbFYHzFatHw= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-412-_HjGUhyPP1-LU4NhapCOvw-1; Wed, 12 Feb 2020 09:45:21 -0500 X-MC-Unique: _HjGUhyPP1-LU4NhapCOvw-1 Received: by mail-wm1-f71.google.com with SMTP id 7so796426wmf.9 for ; Wed, 12 Feb 2020 06:45:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=5yxu7FAFWwnIWX0SIGmGmSoK4AorTJs9H07QQ92CKAE=; b=ZuVXJfR6I9WvPWKsOMEcjhh3mufMcyj3cHOKAXijH1YPNk84R4xNTKpURdPxN7LZ2m iGn5K2fVgNTJk5wK2XrE5xncsby3JWW7n5lAFVyjvJn3XJCKw2RTcC41ANq4BqPMJ3V2 kwRPx/Gpsaoizmn9D5T4yTNfG26mPcpkW2TiFdP1JyRVP0IHucV9U2Kuff7LqTomXoaY Fn2SPl+DBMQk624BBv+d9bJlmWUnL7l26FANUVFtp5MuN9tEeinrpiZcOlEPKDm9w5ZQ ixY30OX8YlcmVKZCmq/qMWNFm8of63v/g0Me0xVj3QK4gP4SitPh5U2/jEH4dmUYPlkl Kpxg== X-Gm-Message-State: APjAAAVCN2QhhORzKLihB8zhwhUIJ7Mtp/hsj9YY9ztkgoAVfDsaucjm nPeQqca1+gWSbgiBy8WLj4sWGT8LkExkwUyJvcT2XcS0KqdxGvrkI72+jru9EqUPONKLUj5bivB DqrmzsOVai2QC6HgSjd3qcolr X-Received: by 2002:a05:600c:2207:: with SMTP id z7mr13285285wml.138.1581518719658; Wed, 12 Feb 2020 06:45:19 -0800 (PST) X-Received: by 2002:a05:600c:2207:: with SMTP id z7mr13285259wml.138.1581518719307; Wed, 12 Feb 2020 06:45:19 -0800 (PST) Received: from x1.localdomain (2001-1c00-0c0c-fe00-fc7e-fd47-85c1-1ab3.cable.dynamic.v6.ziggo.nl. [2001:1c00:c0c:fe00:fc7e:fd47:85c1:1ab3]) by smtp.gmail.com with ESMTPSA id m9sm893216wrx.55.2020.02.12.06.45.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 Feb 2020 06:45:18 -0800 (PST) From: Hans de Goede Subject: Re: [PATCH v3 0/5] SFH: Add Support for AMD Sensor Fusion Hub To: Sandeep Singh , 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 References: <1581476197-25854-1-git-send-email-Sandeep.Singh@amd.com> Message-ID: <1ce6f591-1e8b-8291-7f18-48876fd70e10@redhat.com> Date: Wed, 12 Feb 2020 15:45:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <1581476197-25854-1-git-send-email-Sandeep.Singh@amd.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 2/12/20 3:56 AM, Sandeep Singh wrote: > From: Sandeep Singh > > 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 >