Received-SPF: pass (google.com: domain of linux-kernel+bounces-203559-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 5 Jun 2024 22:04:16 -0500
From: Jeff LaBundy <jeff@labundy.com>
To: wangshuaijie@awinic.com
Cc: dmitry.torokhov@gmail.com, robh@kernel.org, krzk+dt@kernel.org,
	conor+dt@kernel.org, linux-input@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	liweilei@awinic.com, kangjiajun@awinic.com
Subject: Re: [PATCH V2 0/5] Add support for Awinic SAR sensor.
Message-ID: <ZmEnMPhKzqkJssQE@nixie71>
References: <20240605091143.163789-1-wangshuaijie@awinic.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240605091143.163789-1-wangshuaijie@awinic.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?xQaJU0f/7P6F+4OtSwrLoODQOG2DVakqvYeFAo0Bvgyvd0+VQ8ehUptsvKfC?=
 =?us-ascii?Q?FZ5P/RmuIREnKLz58bImVnuPzECEmXZGC81f+bcuD/YC8/OzlU4YnBPeq1ML?=
 =?us-ascii?Q?WNcfL7rLdXP0q+Gy7QzA+XZPlAF7kg5hgtP4wp0hzkhBFFiAXUKkOXzYpeMG?=
 =?us-ascii?Q?Fq8SIarM/CpdFn9KJOVbUg3eHfljKLOF9oPDbUu+QNIx/i3Aj0LPaW4CKUKu?=
 =?us-ascii?Q?rIGWKa3h/8r5EhRXugiop+uxzcZJhICv+FQmuYkHKJlQULT33gBJLb68rhEF?=
 =?us-ascii?Q?3a1WqBavhlHD+MeLf1KoB0q+KHIC/QYqVwBkuFXR/HWPGxzWRP8DRKAFAsW3?=
 =?us-ascii?Q?oeOrr+L0c+lpRJzJKg6hE+NSGGN+qgQmpye9FniTcIdDLpoPA2gLR+BSLjcf?=
 =?us-ascii?Q?DAqYpKT1fU/iVKRuyg85COLTEzYA3KlGdTiFXJbYKsGdT2WD5/jtY7Un1wyu?=
 =?us-ascii?Q?2fDaM9S5DQZyQ2HD5Ew+dSzI33HwhPJux7zMzCwUF9vVGi08OHo0iifLXSbo?=
 =?us-ascii?Q?N6UCObNP7lGm8DP2TSjEvTKzYB/eoYezXRVG3ebpx+GuwiZjBJYadIP4bfiq?=
 =?us-ascii?Q?h7ERd3hDrCaH60s/jO1ClSgj79yKfL07lNkYJsJN4EBkf35ZNQnv2YH9IkOk?=
 =?us-ascii?Q?FXYEX87z+osBUEhiLA+phlLISGx1YjjBlV3Yrx3mureF5rvMPoj1vG9i6IRe?=
 =?us-ascii?Q?4wOxVWX7EWb4rA4KvcerWBh10Sk6o4n0HSS424LI7z7xsnA926zYTeSMa3gQ?=
 =?us-ascii?Q?vQHnHtndq0Xk9g5KjFByowb7pafrOUbzmAJLeyziUt9P+bPahoV/Wqyls8G2?=
 =?us-ascii?Q?woKz4AH8HpZ8lfshCJQrclYkAByvBPofjKlCKbMXM0/9lyYihXUi/pzQVVUd?=
 =?us-ascii?Q?rWL02xkMAoWzJPmperkrBpwLWAqJkwUxbUUvJTZiIvCG6gKHWttfQT+DiYbH?=
 =?us-ascii?Q?PodVXhJMQ1oHKI/457BMV8juPOj31XSbHymp6ozNkeDIui2aniEno+4kRQMd?=
 =?us-ascii?Q?hgRdmdrTWVfifBT8wz0mZPUVgmH9LDFR22AbDJvX8QJuZZaE2rOc90GKdCkW?=
 =?us-ascii?Q?8CVI6ra35Qp0N8THa+llZQZ0V5v10/4mcJUZdtODX38hnmh7Zf76JvkDEieL?=
 =?us-ascii?Q?yc5+tisTcDTIq6qMqyQdtN2E5H+uAmcTXyrKixHODHr9S+awXOi837Tq1Mjh?=
 =?us-ascii?Q?uwTibC4yBiWMlLaydt2ZX0XfxOVU/WcZ56iiQG1RmpZGugbHRO/O61rdUTu6?=
 =?us-ascii?Q?vACZz4CiH2tFxCsz+aUEBdSUQBHsqvdXAYNG3XLykvhp53OdprHnOYFE61hn?=
 =?us-ascii?Q?PaDsAMnzZk5+qYT025yagAI+ybYZQUPP2fxro/lbrD3I2qSOZ7AJPee/13ib?=
 =?us-ascii?Q?mNC6i9r3h9cFsiH51by6ZsTmBS8kJwKkKD2dX9E7eIfcfEmVatVedkJN5+Pd?=
 =?us-ascii?Q?j6tOaJ0wDIowLCxPeJnJep3bx9JpKoJqMhe3fKesd/2DKz2Su+BS4UaapZs9?=
 =?us-ascii?Q?XUg/xB5+BoDN3Ob0ghKfnx7GtUOQ54BVz9iSE1RhAMfE2GUjHsljwOnJfpMr?=
 =?us-ascii?Q?UXE9bGN3NMxedRgYfFUXl9KuG8fXoldyRAh/mFfd?=
X-OriginatorOrg: labundy.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0f06f197-7907-4d15-7332-08dc85d56032
X-MS-Exchange-CrossTenant-AuthSource: BN7PR08MB3937.namprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2024 03:04:27.1666
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 00b69d09-acab-4585-aca7-8fb7c6323e6f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nUP3r9KJ4eFeE28FmH3INDvw309C/DRdqci5MC+LgIFI/2nFnlWY6byfT/hZbWXFrCsSFndjjMjnYIhOAYHpoA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR08MB6289

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