| Message ID | 20240605091143.163789-1-wangshuaijie@awinic.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for Awinic SAR sensor. | expand |
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. [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
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. > >[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
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
Hi Krzysztof, On Fri, 12 Jul 2024 14:02:40 +0200, krzk@kernel.org wrote: >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 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
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. 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