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
