linux-kernel - Re: [PATCH V2 0/5] Add support for Awinic SAR sensor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240725124831.891084-1-wangshuaijie@awinic.com>
Date: Thu, 25 Jul 2024 12:48:31 +0000
From: wangshuaijie@...nic.com
To: krzk@...nel.org
Cc: conor+dt@...nel.org,
	devicetree@...r.kernel.org,
	dmitry.torokhov@...il.com,
	jeff@...undy.com,
	kangjiajun@...nic.com,
	krzk+dt@...nel.org,
	linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	liweilei@...nic.com,
	robh@...nel.org,
	wangshuaijie@...nic.com
Subject: Re: [PATCH V2 0/5] Add support for Awinic SAR sensor

Hi Krzysztof,

On Fri, 12 Jul 2024 14:02:40 +0200, krzk@...nel.org wrote:
>On 12/07/2024 11:49, wangshuaijie@...nic.com wrote:
>> Hi Jeff，
>> 
>> Thank you very much for your valuable suggestions. They are indeed a great help to me. 
>> 
>> There are some issues with this driver, but I will do my utmost to improve it 
>> based on your advice. I will change the input subsystem in the driver to the 
>> IIO subsystem and place it in the IIO/proximity directory. I will also modify 
>> the structure of the driver to make it appear more reasonable.
>> 
>> On Wed, 5 Jun 2024 22:04:16 -0500, jeff@...undy.com wrote:
>>> Hi Shuaijie,
>>>
>>> On Wed, Jun 05, 2024 at 09:11:38AM +0000, wangshuaijie@...nic.com wrote:
>>>> From: shuaijie wang <wangshuaijie@...nic.com>
>>>>
>>>> Add drivers that support Awinic SAR (Specific Absorption Rate)
>>>> sensors to the Linux kernel.
>>>>
>>>> The AW9610X series and AW963XX series are high-sensitivity
>>>> capacitive proximity detection sensors.
>>>>
>>>> This device detects human proximity and assists electronic devices
>>>> in reducing SAR to pass SAR related certifications.
>>>>
>>>> The device reduces RF power and reduces harm when detecting human proximity.
>>>> Increase power and improve signal quality when the human body is far away.
>>>>
>>>> This patch implements device initialization, registration,
>>>> I/O operation handling and interrupt handling, and passed basic testing.
>>>
>>> Thank you for your submission! It's always great to see new devices
>>> introduced to the kernel. Maybe I can give some high-level feedback
>>> first.
>>>
>>> Unfortunately, I don't think we can review this driver in its current
>>> form; the style and structure are simply too different from what is
>>> expected in mainline. Many of these problems can be identified with
>>> checkpatch [1].
>>>
>>> To that point, I don't think this driver belongs as an input driver.
>>> The input subsystem tends to be a catch-all for sensors in downstream
>>> kernels, and some bespoke SOC vendor HALs tend to follow this approach,
>>> but that does not necessarily mean input is always the best choice.
>>>
>>> SAR devices are a special case where an argument could be made for the
>>> driver to be an input driver, or an IIO/proximity driver. If the device
>>> emits binary near/far events, then an input driver is a good choice;
>>> typically the near/far event could be mapped to a switch code such as
>>> SW_FRONT_PROXIMITY.
>>>
>>> If the device emits continuous proximity data (in arbitrary units or
>>> otherwise), however, IIO/proximity seems like a better choice here. This
>>> driver seems to report proximity using ABS_DISTANCE, which is kind of an
>>> abuse of the input subsystem, and a strong indicator that this driver
>>> should really be an IIO/proximity driver. If you disagree, I think we
>>> at least need some compelling reasoning in the commit message.
>>>
>>> Regardless of this choice, this driver should really only be 2-3 patches
>>> (not counting cover letter): one for the binding, and one for a single,
>>> homogenous driver for each of the two devices, unless they have enough
>>> in common that they can be supported by a single driver. Mainline tends
>>> to avoid vendor-specific (and especially part-specific) entire directories.
>>>
>>> I agree with Krzysztof's advice in one of the other patches; I think it
>>> would be best to study some existing drivers in mainline to gain a
>>> better sense of how they are organized, then use those as a model. If I
>>> may suggest, consider referring to drivers such as [2] and its cousins
>>> in the same directory; these are capacitive proximity sensors that can
>>> be used as buttons, but SAR devices tend to be built upon the same principle.
>
>Not much improved in v3 in this regard.
>
>Sorry, this code is not ready for review. There are so many trivial
>style issues, it's like someone sends us Windows drivers for Linux.
>
>Best regards,
>Krzysztof

Thank you very much for your suggestion. I will try my best to optimize
the code and make it look more appropriate.

Kind regards,
Wang Shuaijie