Re: [PATCH v4 2/4] SFH: PCI driver to add support of AMD sensor fusion Hub using HID framework

From: Richard Neumann <mail@richard-neumann.de>
To: "Shah, Nehal-bakulchandra" <nehal-bakulchandra.shah@amd.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Sandeep Singh <Sandeep.Singh@amd.com>,
	Shyam-sundar.S-k@amd.com, Jiri Kosina <jikos@kernel.org>,
	Benjamin Tissoires <benjamin.tissoires@redhat.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-input <linux-input@vger.kernel.org>,
	Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
	Jonathan Cameron <jic23@kernel.org>,
	linux-iio <linux-iio@vger.kernel.org>,
	Hans de Goede <hdegoede@redhat.com>
Subject: Re: [PATCH v4 2/4] SFH: PCI driver to add support of AMD sensor fusion Hub using HID framework
Date: Tue, 21 Apr 2020 23:18:23 +0200	[thread overview]
Message-ID: <c6831429000eeb2132c68c44d4d171063fa53d56.camel@richard-neumann.de> (raw)
In-Reply-To: <f4c08c96-bbaf-c42e-0dff-a02c5bbed6ed@amd.com>

[-- Attachment #1: Type: text/plain, Size: 2798 bytes --]

Am Mittwoch, den 22.04.2020, 00:01 +0530 schrieb Shah, Nehal-
bakulchandra:
> Hi Richard,
> 
> Thanks for the refactoring  the patch. The .raw_request in
> hid_ll_driver is not correct, the output of sensor is not raw but it
> is processed one,
> 
> so better to move it to .request function. Regarding your question
> for sensor position well it comes from firmware. So i am not sure
> about the firmware
> 
> for sensor , what you have.Also, regarding the IOMMU, we figured out
> the issue, earlier patch series has bug that during DMA allocation
> pdev was passed
> 
> from platform driver context, where in for IOMMU needs context PCI
> pdev. So that is taken care in your refactored patch hence issue is
> disappeared.
> 
> 
> Now regarding the patch (with your changes) i need to validate at our
> side. Due to lockdown i dont have access to the system, so may be we
> can wait on that.
> 
> 
> Thanks
> 
> Nehal Shah

Hi Nehal

and thank you for the feedback.

Regarding the raw_request / request, I think the "raw" here is meant to
indicate, that the implementation is expected to write "raw bytes" [1]
into a buffer rather than handling a "struct hid_report", which is
exactly what the get_feature_report() and get_input_report() from the
original patch seem to be intended to do.
However it should be easy to migrate this to ".report", since it'd
probably look a lot like __hid_request() [2], which is currently doing
the magic automatically for me.
Furthermore a ll_driver needs a raw_request implementation anyways as
it's being tested for in hid_add_device() [3].

Regarding the validation on your side: Of course! I do in no way want
to undermine your work on this in any way. I'm just offering what I've
learned so far and what is working for me. If you find parts of it
worth taking into a refactored version for upstream, awesome. If you
don't agree with other parts, that's fine, too. I'm quite new to this
and eager to learn. Also I am in no rush. Good things take time.

In case you're missing my patch on gist.github.com, I decided to
further develop the driver in a forked linux source tree [4].

It's on the branch amd-sfh. You can just diff to the master branch to
get the entire patch. The changed files are still under
drivers/hid/amd-sfh-hid and Documentation/hid respectively.

You'll find that in the meantime I did some more work / learning and
implemented some rudimentary IRQ handling.

Kind regards,

Richard

[1] https://elixir.bootlin.com/linux/latest/source/include/linux/hid.h#L1070
[2] https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-core.c#L1688
[3] https://elixir.bootlin.com/linux/latest/source/drivers/hid/hid-core.c#L2386
[3] https://github.com/conqp/linux/tree/amd-sfh

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]