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
