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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 12/07/2024 11:49, wangshuaijie@awinic.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@labundy.com wrote:
>> Hi Shuaijie,
>>
>> On Wed, Jun 05, 2024 at 09:11:38AM +0000, wangshuaijie@awinic.com wrote:
>>> From: shuaijie wang <wangshuaijie@awinic.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