Linux Input Archive on lore.kernel.org
 help / color / Atom feed
From: "Shah, Nehal-bakulchandra" <nehal-bakulchandra.shah@amd.com>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Sandeep Singh <Sandeep.Singh@amd.com>,
	Jiri Kosina <jikos@kernel.org>,
	lkml <linux-kernel@vger.kernel.org>,
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	linux-iio@vger.kernel.org, Shyam-sundar.S-k@amd.com
Subject: Re: [PATCH v3 0/5] SFH: Add Support for AMD Sensor Fusion Hub
Date: Wed, 19 Feb 2020 20:34:52 +0530
Message-ID: <79506ad3-5172-be36-cecc-19d97bfa8b36@amd.com> (raw)
In-Reply-To: <CAO-hwJJj8uHVr_BTG0rcBchiEttuQTr7ovrtTQ=Cp5vJ2eeoNg@mail.gmail.com>

Hi

On 2/14/2020 4:41 PM, Benjamin Tissoires wrote:
> On Fri, Feb 14, 2020 at 11:04 AM Shah, Nehal-bakulchandra
> <nehal-bakulchandra.shah@amd.com> wrote:
>> Hi
>>
>> On 2/14/2020 10:10 AM, Srinivas Pandruvada wrote:
>>> On Thu, 2020-02-13 at 15:56 +0100, Benjamin Tissoires wrote:
>>>> Hi,
>>>>
>>>> On Wed, Feb 12, 2020 at 3:45 PM Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>>>> 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.
>> Thanks for the valuable input.
>>>> 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
>>>
>>>
>>>> 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.
> OK, that's good enough for me. Jiri, are you fine with that too?
>
>> 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.
>>
> 2 problems here:
> - patch 3/5 references this debugfs interface which is only added in 4/5.
> - you are creating a new sysfs set of file for debug purpose only, but
> as soon as we start shipping those, some other people will find it
> more convenient to use that directly instead or IIO, and you won't be
> able to change anything there.
>
> So I would strongly advocate against having this debugfs, and suggest you to:
> - either keep this debugfs as a downstream patch
> - either play with eBPF or kprobes to retrieve the same information
> without changing the kernel.
>
> For reference, I recently tried to replicate the hidraw functionality
> with eBPF[0] without changing the kernel code, and it was working.
> Well, there was no filtering on the source of the HID event, but
> still, I got the same data directly from the kernel just by adding
> instrumentation in a couple of functions.
>
> Cheers,
> Benjamin

If Jiri is Ok, then we will push the next patch as per suggestion here i.e. taking out debugfs.

Thanks

Nehal Shah

>

  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 ` [PATCH v3 0/5] SFH: Add Support for AMD Sensor Fusion Hub Hans de Goede
2020-02-13 14:56   ` 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 [this message]
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=79506ad3-5172-be36-cecc-19d97bfa8b36@amd.com \
    --to=nehal-bakulchandra.shah@amd.com \
    --cc=Sandeep.Singh@amd.com \
    --cc=Shyam-sundar.S-k@amd.com \
    --cc=benjamin.tissoires@redhat.com \
    --cc=hdegoede@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