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

[wangshuaijie]

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.
>
>[1] https://docs.kernel.org/dev-tools/checkpatch.html
>[2] drivers/iio/proximity/sx9500.c
>
>> 
>> shuaijie wang (5):
>>   dt-bindings: input: Add YAML to Awinic sar sensor.
>>   Add universal interface for the aw_sar driver.
>>   Add aw9610x series related interfaces to the aw_sar driver.
>>   Add aw963xx series related interfaces to the aw_sar driver.
>>   Add support for Awinic sar sensor.
>> 
>>  .../bindings/input/awinic,aw_sar.yaml         |  125 +
>>  drivers/input/misc/Kconfig                    |    9 +
>>  drivers/input/misc/Makefile                   |    1 +
>>  drivers/input/misc/aw_sar/Makefile            |    2 +
>>  drivers/input/misc/aw_sar/aw9610x/aw9610x.c   |  884 +++++++
>>  drivers/input/misc/aw_sar/aw9610x/aw9610x.h   |  327 +++
>>  drivers/input/misc/aw_sar/aw963xx/aw963xx.c   |  974 ++++++++
>>  drivers/input/misc/aw_sar/aw963xx/aw963xx.h   |  753 ++++++
>>  drivers/input/misc/aw_sar/aw_sar.c            | 2036 +++++++++++++++++
>>  drivers/input/misc/aw_sar/aw_sar.h            |   15 +
>>  .../misc/aw_sar/comm/aw_sar_chip_interface.h  |   27 +
>>  .../misc/aw_sar/comm/aw_sar_comm_interface.c  |  639 ++++++
>>  .../misc/aw_sar/comm/aw_sar_comm_interface.h  |  172 ++
>>  drivers/input/misc/aw_sar/comm/aw_sar_type.h  |  396 ++++
>>  14 files changed, 6360 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/input/awinic,aw_sar.yaml
>>  create mode 100644 drivers/input/misc/aw_sar/Makefile
>>  create mode 100644 drivers/input/misc/aw_sar/aw9610x/aw9610x.c
>>  create mode 100644 drivers/input/misc/aw_sar/aw9610x/aw9610x.h
>>  create mode 100644 drivers/input/misc/aw_sar/aw963xx/aw963xx.c
>>  create mode 100644 drivers/input/misc/aw_sar/aw963xx/aw963xx.h
>>  create mode 100644 drivers/input/misc/aw_sar/aw_sar.c
>>  create mode 100644 drivers/input/misc/aw_sar/aw_sar.h
>>  create mode 100644 drivers/input/misc/aw_sar/comm/aw_sar_chip_interface.h
>>  create mode 100644 drivers/input/misc/aw_sar/comm/aw_sar_comm_interface.c
>>  create mode 100644 drivers/input/misc/aw_sar/comm/aw_sar_comm_interface.h
>>  create mode 100644 drivers/input/misc/aw_sar/comm/aw_sar_type.h
>> 
>> 
>> base-commit: 32f88d65f01bf6f45476d7edbe675e44fb9e1d58
>> -- 
>> 2.45.1
>> 
>
>Kind regards,
>Jeff LaBundy

Best regards,
Wang Shuaijie