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

[wangshuaijie]

Hi Krzysztof,

On Fri, 12 Jul 2024 14:02:40 +0200, krzk@xxxxxxxxxx wrote:
>On 12/07/2024 11:49, wangshuaijie@xxxxxxxxxx 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@xxxxxxxxxxx wrote:
>>> Hi Shuaijie,
>>>
>>> On Wed, Jun 05, 2024 at 09:11:38AM +0000, wangshuaijie@xxxxxxxxxx wrote:
>>>> From: shuaijie wang <wangshuaijie@xxxxxxxxxx>
>>>>
>>>> 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