linux-kernel - Re: [PATCH 2/2] Input sx951x: add Semtech SX9512/SX9513 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <siwaccc2rwbeggi7aq6rapm2rjqz6ceed3hp2zkwx5axmc56oi@jtkbjzdcraqy>
Date: Mon, 5 May 2025 22:26:15 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: David Bauer <mail@...id-bauer.net>
Cc: linux-kernel@...r.kernel.org, linux-input@...r.kernel.org
Subject: Re: [PATCH 2/2] Input sx951x: add Semtech SX9512/SX9513 driver

Hi David,

On Mon, May 05, 2025 at 10:38:45PM +0200, David Bauer wrote:
> The Semtech SX9512/SX9513 is a family of capacitive touch-keyboard
> controllers.
> 
> All chips offer 8 channel touch sensitive inputs with one LED driver per
> output channel.
> 
> The also SX9512 supports proximity detection which is currently not
> supported with the driver.
> 
> This chip can be found on the Genexis Pulse EX400 repeater platform.
> 
> Link: https://www.mouser.com/datasheet/2/761/SEMTS05226_1-2575172.pdf
> 
> Signed-off-by: David Bauer <mail@...id-bauer.net>
> ---
>  drivers/input/keyboard/Kconfig  |  11 +
>  drivers/input/keyboard/Makefile |   1 +
>  drivers/input/keyboard/sx951x.c | 490 ++++++++++++++++++++++++++++++++
>  3 files changed, 502 insertions(+)
>  create mode 100644 drivers/input/keyboard/sx951x.c
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 1d0c5f4c0f99..6dc397389c64 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -616,6 +616,17 @@ config KEYBOARD_SUNKBD
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called sunkbd.
>  
> +config KEYBOARD_SX951X
> +	tristate "Semtech SX951X capacitive touch controller"
> +	depends on OF && I2C

I do not believe it should depend on OF. Please have the driver use
generic device properties instead (device_property_*()).

...

> +
> + #include <linux/kernel.h>
> + #include <linux/module.h>
> + #include <linux/input.h>
> + #include <linux/leds.h>
> + #include <linux/of.h>

You will likely need mod_devicetable.h instead of of.h.

> + #include <linux/regmap.h>
> + #include <linux/i2c.h>
> + #include <linux/gpio/consumer.h>
> + #include <linux/bitfield.h>
> +
> + /* Generic properties */
> +#define SX951X_I2C_ADDRESS		0x2b
> +#define SX951XB_I2C_ADDRESS_		0x2d

Why underscore at the end?

...

> +
> +#ifdef CONFIG_LEDS_CLASS
> +		error = sx951x_led_init(priv, child, reg);
> +		if (error) {
> +			dev_err(dev, "Failed to initialize LED %d: %d\n",
> +				reg, error);
> +			return error;
> +		}
> +#endif

Please provide stub for sx951x_led_init() instead of using #ifdef here. 

...
> +
> +	priv = devm_kzalloc(dev,
> +			    sizeof(struct sx951x_priv),

sizeof(*priv)

Thanks.

-- 
Dmitry