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
