[PATCH v2 0/3] Add support for Awinic AW86938 haptic driver

[PATCH v2 0/3] Add support for Awinic AW86938 haptic driver

[Griffin Kroah-Hartman]

Add devicetree bindings and a driver for the AW86938 haptic driver chip,
and add it to the devicetree for the Fairphone 6 smartphone.

This driver is very similar to the AW86927, and shares many core
features.

Signed-off-by: Griffin Kroah-Hartman <griffin.kroah@fairphone.com>
---
Changes in v2:
- Added AW86938 specific registers
- Added chip model enum to differentiate chips
- Link to v1: https://lore.kernel.org/r/20251204-aw86938-driver-v1-0-ebd71868df3a@fairphone.com

---
Griffin Kroah-Hartman (3):
      dt-bindings: input: awinic,aw86927: Add Awinic AW86938
      Input: aw86938 - add driver for Awinic AW86938
      arm64: dts: qcom: milos-fairphone-fp6: Add vibrator support

 .../devicetree/bindings/input/awinic,aw86927.yaml  |  4 +-
 arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts   | 19 ++++++-
 drivers/input/misc/aw86927.c                       | 65 ++++++++++++++++++----
 3 files changed, 74 insertions(+), 14 deletions(-)
---
base-commit: 0364de6be161e2360cbb1f26d5aff5b343ef7bb0
change-id: 20251113-aw86938-driver-b4fa0d3228a2

Best regards,
-- 
Griffin Kroah-Hartman <griffin.kroah@fairphone.com>

[PATCH v2 1/3] dt-bindings: input: awinic,aw86927: Add Awinic AW86938

[Griffin Kroah-Hartman]

Add bindings for the Awinic AW86938 haptic chip which can be found in
smartphones. These two chips require a similar devicetree configuration,
but have a register layout that's not 100% compatible.
Still, we can document them in the same file.

Signed-off-by: Griffin Kroah-Hartman <griffin.kroah@fairphone.com>
---
 Documentation/devicetree/bindings/input/awinic,aw86927.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/awinic,aw86927.yaml b/Documentation/devicetree/bindings/input/awinic,aw86927.yaml
index b7252916bd727486c1a98913d4ec3ef12422e4bd..c3dee660422192720da3cf63851cea27db819742 100644
--- a/Documentation/devicetree/bindings/input/awinic,aw86927.yaml
+++ b/Documentation/devicetree/bindings/input/awinic,aw86927.yaml
@@ -11,7 +11,9 @@ maintainers:
 
 properties:
   compatible:
-    const: awinic,aw86927
+    enum:
+      - awinic,aw86927
+      - awinic,aw86938
 
   reg:
     maxItems: 1

-- 
2.43.0

[PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[Griffin Kroah-Hartman]

Add support for the I2C-connected Awinic AW86938 LRA haptic driver.

The AW86938 has a similar but slightly different register layout. In
particular, the boost mode register values.
The AW86938 also has some extra features that aren't implemented
in this driver yet.

Signed-off-by: Griffin Kroah-Hartman <griffin.kroah@fairphone.com>
---
 drivers/input/misc/aw86927.c | 65 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 53 insertions(+), 12 deletions(-)

diff --git a/drivers/input/misc/aw86927.c b/drivers/input/misc/aw86927.c
index 8ad361239cfe3a888628b15e4dbdeed0c9ca3d1a..28ec6e42fd452a0edf1e1b9a9614e2723c6d9f93 100644
--- a/drivers/input/misc/aw86927.c
+++ b/drivers/input/misc/aw86927.c
@@ -43,6 +43,12 @@
 #define AW86927_PLAYCFG1_BST_VOUT_VREFSET_MASK	GENMASK(6, 0)
 #define AW86927_PLAYCFG1_BST_8500MV		0x50
 
+#define AW86938_PLAYCFG1_REG			0x06
+#define AW86938_PLAYCFG1_BST_MODE_MASK		GENMASK(5, 5)
+#define AW86938_PLAYCFG1_BST_MODE_BYPASS	0
+#define AW86938_PLAYCFG1_BST_VOUT_VREFSET_MASK	GENMASK(4, 0)
+#define AW86938_PLAYCFG1_BST_7000MV		0x11
+
 #define AW86927_PLAYCFG2_REG			0x07
 
 #define AW86927_PLAYCFG3_REG			0x08
@@ -140,6 +146,7 @@
 #define AW86927_CHIPIDH_REG			0x57
 #define AW86927_CHIPIDL_REG			0x58
 #define AW86927_CHIPID				0x9270
+#define AW86938_CHIPID				0x9380
 
 #define AW86927_TMCFG_REG			0x5b
 #define AW86927_TMCFG_UNLOCK			0x7d
@@ -173,7 +180,13 @@ enum aw86927_work_mode {
 	AW86927_RAM_MODE,
 };
 
+enum aw86927_model {
+	AW86927,
+	AW86938,
+};
+
 struct aw86927_data {
+	enum aw86927_model model;
 	struct work_struct play_work;
 	struct device *dev;
 	struct input_dev *input_dev;
@@ -377,7 +390,7 @@ static int aw86927_play_sine(struct aw86927_data *haptics)
 		return err;
 
 	/* set gain to value lower than 0x80 to avoid distorted playback */
-	err = regmap_write(haptics->regmap, AW86927_PLAYCFG2_REG, 0x7c);
+	err = regmap_write(haptics->regmap, AW86927_PLAYCFG2_REG, 0x45);
 	if (err)
 		return err;
 
@@ -565,13 +578,26 @@ static int aw86927_haptic_init(struct aw86927_data *haptics)
 	if (err)
 		return err;
 
-	err = regmap_update_bits(haptics->regmap,
-				 AW86927_PLAYCFG1_REG,
-				 AW86927_PLAYCFG1_BST_VOUT_VREFSET_MASK,
-				 FIELD_PREP(AW86927_PLAYCFG1_BST_VOUT_VREFSET_MASK,
-					    AW86927_PLAYCFG1_BST_8500MV));
-	if (err)
-		return err;
+	switch (haptics->model) {
+	case AW86927:
+		err = regmap_update_bits(haptics->regmap,
+				AW86927_PLAYCFG1_REG,
+				AW86927_PLAYCFG1_BST_VOUT_VREFSET_MASK,
+				FIELD_PREP(AW86927_PLAYCFG1_BST_VOUT_VREFSET_MASK,
+					AW86927_PLAYCFG1_BST_8500MV));
+		if (err)
+			return err;
+		break;
+	case AW86938:
+		err = regmap_update_bits(haptics->regmap,
+				AW86938_PLAYCFG1_REG,
+				AW86938_PLAYCFG1_BST_VOUT_VREFSET_MASK,
+				FIELD_PREP(AW86938_PLAYCFG1_BST_VOUT_VREFSET_MASK,
+					AW86938_PLAYCFG1_BST_7000MV));
+		if (err)
+			return err;
+		break;
+	}
 
 	err = regmap_update_bits(haptics->regmap,
 				 AW86927_PLAYCFG3_REG,
@@ -599,6 +625,9 @@ static int aw86927_ram_init(struct aw86927_data *haptics)
 				 FIELD_PREP(AW86927_SYSCTRL3_EN_RAMINIT_MASK,
 					    AW86927_SYSCTRL3_EN_RAMINIT_ON));
 
+	/* AW86938 wants a 1ms delay here */
+	usleep_range(1000, 1500);
+
 	/* Set base address for the start of the SRAM waveforms */
 	err = regmap_write(haptics->regmap,
 			   AW86927_BASEADDRH_REG, AW86927_BASEADDRH_VAL);
@@ -717,9 +746,19 @@ static int aw86927_detect(struct aw86927_data *haptics)
 
 	chip_id = be16_to_cpu(read_buf);
 
-	if (chip_id != AW86927_CHIPID) {
-		dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
-		return -ENODEV;
+	switch (haptics->model) {
+	case AW86927:
+		if (chip_id != AW86927_CHIPID) {
+			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
+			return -ENODEV;
+		}
+		break;
+	case AW86938:
+		if (chip_id != AW86938_CHIPID) {
+			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
+			return -ENODEV;
+		}
+		break;
 	}
 
 	return 0;
@@ -736,6 +775,7 @@ static int aw86927_probe(struct i2c_client *client)
 
 	haptics->dev = &client->dev;
 	haptics->client = client;
+	haptics->model = (enum aw86927_model)device_get_match_data(&client->dev);
 
 	i2c_set_clientdata(client, haptics);
 
@@ -825,7 +865,8 @@ static int aw86927_probe(struct i2c_client *client)
 }
 
 static const struct of_device_id aw86927_of_id[] = {
-	{ .compatible = "awinic,aw86927" },
+	{ .compatible = "awinic,aw86927", .data = (void *)AW86927 },
+	{ .compatible = "awinic,aw86938", .data = (void *)AW86938 },
 	{ /* sentinel */ }
 };
 

-- 
2.43.0

[PATCH v2 3/3] arm64: dts: qcom: milos-fairphone-fp6: Add vibrator support

[Griffin Kroah-Hartman]

Add the required node for haptic playback (Awinic AW86938)

Signed-off-by: Griffin Kroah-Hartman <griffin.kroah@fairphone.com>
---
 arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts b/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
index 52895dd9e4fa117aef6822df230ebf644e5f02ba..881239d22fa97685206d1fa3a70723c5b77a339c 100644
--- a/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
+++ b/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
@@ -625,7 +625,17 @@ vreg_l7p: ldo7 {
 	};
 
 	/* VL53L3 ToF @ 0x29 */
-	/* AW86938FCR vibrator @ 0x5a */
+
+	vibrator@5a {
+		compatible = "awinic,aw86938";
+		reg = <0x5a>;
+
+		interrupts-extended = <&tlmm 80 IRQ_TYPE_EDGE_FALLING>;
+		reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
+
+		pinctrl-0 = <&aw86938_int_default>;
+		pinctrl-names = "default";
+	};
 };
 
 &pm8550vs_c {
@@ -755,6 +765,13 @@ sdc2_card_det_n: sdc2-card-det-state {
 		bias-pull-up;
 	};
 
+	aw86938_int_default: aw86938-int-default-state {
+		pins = "gpio80";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-pull-up;
+	};
+
 	pm8008_int_default: pm8008-int-default-state {
 		pins = "gpio125";
 		function = "gpio";

-- 
2.43.0

Re: [PATCH v2 3/3] arm64: dts: qcom: milos-fairphone-fp6: Add vibrator support

[Dmitry Baryshkov]

On Wed, Jan 28, 2026 at 04:51:15PM +0100, Griffin Kroah-Hartman wrote:
> Add the required node for haptic playback (Awinic AW86938)
> 
> Signed-off-by: Griffin Kroah-Hartman <griffin.kroah@fairphone.com>
> ---
>  arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

Re: [PATCH v2 3/3] arm64: dts: qcom: milos-fairphone-fp6: Add vibrator support

[Konrad Dybcio]

On 1/28/26 4:51 PM, Griffin Kroah-Hartman wrote:
> Add the required node for haptic playback (Awinic AW86938)
> 
> Signed-off-by: Griffin Kroah-Hartman <griffin.kroah@fairphone.com>
> ---
>  arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts b/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
> index 52895dd9e4fa117aef6822df230ebf644e5f02ba..881239d22fa97685206d1fa3a70723c5b77a339c 100644
> --- a/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
> +++ b/arch/arm64/boot/dts/qcom/milos-fairphone-fp6.dts
> @@ -625,7 +625,17 @@ vreg_l7p: ldo7 {
>  	};
>  
>  	/* VL53L3 ToF @ 0x29 */
> -	/* AW86938FCR vibrator @ 0x5a */
> +
> +	vibrator@5a {
> +		compatible = "awinic,aw86938";
> +		reg = <0x5a>;
> +
> +		interrupts-extended = <&tlmm 80 IRQ_TYPE_EDGE_FALLING>;
> +		reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
> +
> +		pinctrl-0 = <&aw86938_int_default>;

Ideally there'd also be a config for the reset GPIO, but otherwise

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Konrad

Re: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[kernel test robot]

Hi Griffin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 0364de6be161e2360cbb1f26d5aff5b343ef7bb0]

url:    https://github.com/intel-lab-lkp/linux/commits/Griffin-Kroah-Hartman/dt-bindings-input-awinic-aw86927-Add-Awinic-AW86938/20260129-000753
base:   0364de6be161e2360cbb1f26d5aff5b343ef7bb0
patch link:    https://lore.kernel.org/r/20260128-aw86938-driver-v2-2-b51ee086aaf5%40fairphone.com
patch subject: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938
config: sparc64-allmodconfig (https://download.01.org/0day-ci/archive/20260131/202601311117.t00gEixW-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 9b8addffa70cee5b2acc5454712d9cf78ce45710)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260131/202601311117.t00gEixW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601311117.t00gEixW-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/input/misc/aw86927.c:778:19: warning: cast to smaller integer type 'enum aw86927_model' from 'const void *' [-Wvoid-pointer-to-enum-cast]
     778 |         haptics->model = (enum aw86927_model)device_get_match_data(&client->dev);
         |                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +778 drivers/input/misc/aw86927.c

   766	
   767	static int aw86927_probe(struct i2c_client *client)
   768	{
   769		struct aw86927_data *haptics;
   770		int err;
   771	
   772		haptics = devm_kzalloc(&client->dev, sizeof(struct aw86927_data), GFP_KERNEL);
   773		if (!haptics)
   774			return -ENOMEM;
   775	
   776		haptics->dev = &client->dev;
   777		haptics->client = client;
 > 778		haptics->model = (enum aw86927_model)device_get_match_data(&client->dev);
   779	
   780		i2c_set_clientdata(client, haptics);
   781	
   782		haptics->regmap = devm_regmap_init_i2c(client, &aw86927_regmap_config);
   783		if (IS_ERR(haptics->regmap))
   784			return dev_err_probe(haptics->dev, PTR_ERR(haptics->regmap),
   785						"Failed to allocate register map\n");
   786	
   787		haptics->input_dev = devm_input_allocate_device(haptics->dev);
   788		if (!haptics->input_dev)
   789			return -ENOMEM;
   790	
   791		haptics->reset_gpio = devm_gpiod_get(haptics->dev, "reset", GPIOD_OUT_HIGH);
   792		if (IS_ERR(haptics->reset_gpio))
   793			return dev_err_probe(haptics->dev, PTR_ERR(haptics->reset_gpio),
   794					     "Failed to get reset gpio\n");
   795	
   796		/* Hardware reset */
   797		aw86927_hw_reset(haptics);
   798	
   799		/* Software reset */
   800		err = regmap_write(haptics->regmap, AW86927_RSTCFG_REG, AW86927_RSTCFG_SOFTRST);
   801		if (err)
   802			return dev_err_probe(haptics->dev, err,	"Failed Software reset\n");
   803	
   804		/* Wait ~3ms until I2C is accessible */
   805		usleep_range(3000, 3500);
   806	
   807		err = aw86927_detect(haptics);
   808		if (err)
   809			return dev_err_probe(haptics->dev, err, "Failed to find chip\n");
   810	
   811		/* IRQ config */
   812		err = regmap_write(haptics->regmap, AW86927_SYSCTRL4_REG,
   813				   FIELD_PREP(AW86927_SYSCTRL4_INT_MODE_MASK,
   814					      AW86927_SYSCTRL4_INT_MODE_EDGE) |
   815					FIELD_PREP(AW86927_SYSCTRL4_INT_EDGE_MODE_MASK,
   816						   AW86927_SYSCTRL4_INT_EDGE_MODE_POS));
   817		if (err)
   818			return dev_err_probe(haptics->dev, err, "Failed to configure interrupt modes\n");
   819	
   820		err = regmap_write(haptics->regmap, AW86927_SYSINTM_REG,
   821				   AW86927_SYSINTM_BST_OVPM |
   822					AW86927_SYSINTM_FF_AEM |
   823					AW86927_SYSINTM_FF_AFM |
   824					AW86927_SYSINTM_DONEM);
   825		if (err)
   826			return dev_err_probe(haptics->dev, err, "Failed to configure interrupt masks\n");
   827	
   828		err = devm_request_threaded_irq(haptics->dev, client->irq, NULL,
   829						aw86927_irq, IRQF_ONESHOT, NULL, haptics);
   830		if (err)
   831			return dev_err_probe(haptics->dev, err, "Failed to request threaded irq\n");
   832	
   833		INIT_WORK(&haptics->play_work, aw86927_haptics_play_work);
   834	
   835		haptics->input_dev->name = "aw86927-haptics";
   836		haptics->input_dev->close = aw86927_close;
   837	
   838		input_set_drvdata(haptics->input_dev, haptics);
   839		input_set_capability(haptics->input_dev, EV_FF, FF_RUMBLE);
   840	
   841		err = input_ff_create_memless(haptics->input_dev, NULL, aw86927_haptics_play);
   842		if (err)
   843			return dev_err_probe(haptics->dev, err, "Failed to create FF dev\n");
   844	
   845		/* Set up registers */
   846		err = aw86927_play_mode(haptics, AW86927_STANDBY_MODE);
   847		if (err)
   848			return dev_err_probe(haptics->dev, err,
   849					     "Failed to enter standby for Haptic init\n");
   850	
   851		err = aw86927_haptic_init(haptics);
   852		if (err)
   853			return dev_err_probe(haptics->dev, err, "Haptic init failed\n");
   854	
   855		/* RAM init, upload the waveform for playback */
   856		err = aw86927_ram_init(haptics);
   857		if (err)
   858			return dev_err_probe(haptics->dev, err, "Failed to init aw86927 sram\n");
   859	
   860		err = input_register_device(haptics->input_dev);
   861		if (err)
   862			return dev_err_probe(haptics->dev, err, "Failed to register input device\n");
   863	
   864		return 0;
   865	}
   866	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[Dmitry Torokhov]

Hi Griffin,

On Wed, Jan 28, 2026 at 04:51:14PM +0100, Griffin Kroah-Hartman wrote:
> @@ -717,9 +746,19 @@ static int aw86927_detect(struct aw86927_data *haptics)
>  
>  	chip_id = be16_to_cpu(read_buf);
>  
> -	if (chip_id != AW86927_CHIPID) {
> -		dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
> -		return -ENODEV;
> +	switch (haptics->model) {
> +	case AW86927:
> +		if (chip_id != AW86927_CHIPID) {
> +			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
> +			return -ENODEV;
> +		}

If we are able to query chip ID why do we need to have separate
compatibles? I would define chip data structure with differences between
variants and assign and use it instead of having separate compatible.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[Konrad Dybcio]

On 2/1/26 2:49 AM, Dmitry Torokhov wrote:
> Hi Griffin,
> 
> On Wed, Jan 28, 2026 at 04:51:14PM +0100, Griffin Kroah-Hartman wrote:
>> @@ -717,9 +746,19 @@ static int aw86927_detect(struct aw86927_data *haptics)
>>  
>>  	chip_id = be16_to_cpu(read_buf);
>>  
>> -	if (chip_id != AW86927_CHIPID) {
>> -		dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
>> -		return -ENODEV;
>> +	switch (haptics->model) {
>> +	case AW86927:
>> +		if (chip_id != AW86927_CHIPID) {
>> +			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
>> +			return -ENODEV;
>> +		}
> 
> If we are able to query chip ID why do we need to have separate
> compatibles? I would define chip data structure with differences between
> variants and assign and use it instead of having separate compatible.

dt-bindings guidelines explicitly call for this, a chipid comparison
then works as a safety net

Konrad

Re: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[Luca Weiss]

Hi Konrad,

On Mon Feb 2, 2026 at 11:12 AM CET, Konrad Dybcio wrote:
> On 2/1/26 2:49 AM, Dmitry Torokhov wrote:
>> Hi Griffin,
>> 
>> On Wed, Jan 28, 2026 at 04:51:14PM +0100, Griffin Kroah-Hartman wrote:
>>> @@ -717,9 +746,19 @@ static int aw86927_detect(struct aw86927_data *haptics)
>>>  
>>>  	chip_id = be16_to_cpu(read_buf);
>>>  
>>> -	if (chip_id != AW86927_CHIPID) {
>>> -		dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
>>> -		return -ENODEV;
>>> +	switch (haptics->model) {
>>> +	case AW86927:
>>> +		if (chip_id != AW86927_CHIPID) {
>>> +			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
>>> +			return -ENODEV;
>>> +		}
>> 
>> If we are able to query chip ID why do we need to have separate
>> compatibles? I would define chip data structure with differences between
>> variants and assign and use it instead of having separate compatible.
>
> dt-bindings guidelines explicitly call for this, a chipid comparison
> then works as a safety net

Are you saying, that

1. we should enforce dt-bindings == CHIP_ID (what's currently done)

or

2. we should have both compatibles with no handling based on compatible,
   but only use CHIP_ID at runtime to change behavior

Regards
Luca

Re: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[Konrad Dybcio]

On 2/2/26 11:14 AM, Luca Weiss wrote:
> Hi Konrad,
> 
> On Mon Feb 2, 2026 at 11:12 AM CET, Konrad Dybcio wrote:
>> On 2/1/26 2:49 AM, Dmitry Torokhov wrote:
>>> Hi Griffin,
>>>
>>> On Wed, Jan 28, 2026 at 04:51:14PM +0100, Griffin Kroah-Hartman wrote:
>>>> @@ -717,9 +746,19 @@ static int aw86927_detect(struct aw86927_data *haptics)
>>>>  
>>>>  	chip_id = be16_to_cpu(read_buf);
>>>>  
>>>> -	if (chip_id != AW86927_CHIPID) {
>>>> -		dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
>>>> -		return -ENODEV;
>>>> +	switch (haptics->model) {
>>>> +	case AW86927:
>>>> +		if (chip_id != AW86927_CHIPID) {
>>>> +			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
>>>> +			return -ENODEV;
>>>> +		}
>>>
>>> If we are able to query chip ID why do we need to have separate
>>> compatibles? I would define chip data structure with differences between
>>> variants and assign and use it instead of having separate compatible.
>>
>> dt-bindings guidelines explicitly call for this, a chipid comparison
>> then works as a safety net
> 
> Are you saying, that
> 
> 1. we should enforce dt-bindings == CHIP_ID (what's currently done)

This

> 
> or
> 
> 2. we should have both compatibles with no handling based on compatible,
>    but only use CHIP_ID at runtime to change behavior

This is spaghetti

Konrad

Re: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[Dmitry Torokhov]

On Mon, Feb 02, 2026 at 11:19:36AM +0100, Konrad Dybcio wrote:
> On 2/2/26 11:14 AM, Luca Weiss wrote:
> > Hi Konrad,
> > 
> > On Mon Feb 2, 2026 at 11:12 AM CET, Konrad Dybcio wrote:
> >> On 2/1/26 2:49 AM, Dmitry Torokhov wrote:
> >>> Hi Griffin,
> >>>
> >>> On Wed, Jan 28, 2026 at 04:51:14PM +0100, Griffin Kroah-Hartman wrote:
> >>>> @@ -717,9 +746,19 @@ static int aw86927_detect(struct aw86927_data *haptics)
> >>>>  
> >>>>  	chip_id = be16_to_cpu(read_buf);
> >>>>  
> >>>> -	if (chip_id != AW86927_CHIPID) {
> >>>> -		dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
> >>>> -		return -ENODEV;
> >>>> +	switch (haptics->model) {
> >>>> +	case AW86927:
> >>>> +		if (chip_id != AW86927_CHIPID) {
> >>>> +			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
> >>>> +			return -ENODEV;
> >>>> +		}
> >>>
> >>> If we are able to query chip ID why do we need to have separate
> >>> compatibles? I would define chip data structure with differences between
> >>> variants and assign and use it instead of having separate compatible.
> >>
> >> dt-bindings guidelines explicitly call for this, a chipid comparison
> >> then works as a safety net
> > 
> > Are you saying, that
> > 
> > 1. we should enforce dt-bindings == CHIP_ID (what's currently done)
> 
> This

No. If there is a compatible chip with different ID (for whatever reason
- maybe there is additional functionality that either board does not
need or the driver does not implement) we absolutely should not refuse
to bind the driver.

Hint: this thing is called _compatible_ for a reason.

> 
> > 
> > or
> > 
> > 2. we should have both compatibles with no handling based on compatible,
> >    but only use CHIP_ID at runtime to change behavior
> 
> This is spaghetti

I really do not understand the aversion of DT maintainers to generic
compatibles. We see this in I2C HID where we keep adding compatibles
for what could be described via device properties.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[Konrad Dybcio]

On 2/2/26 12:04 PM, Dmitry Torokhov wrote:
> On Mon, Feb 02, 2026 at 11:19:36AM +0100, Konrad Dybcio wrote:
>> On 2/2/26 11:14 AM, Luca Weiss wrote:
>>> Hi Konrad,
>>>
>>> On Mon Feb 2, 2026 at 11:12 AM CET, Konrad Dybcio wrote:
>>>> On 2/1/26 2:49 AM, Dmitry Torokhov wrote:
>>>>> Hi Griffin,
>>>>>
>>>>> On Wed, Jan 28, 2026 at 04:51:14PM +0100, Griffin Kroah-Hartman wrote:
>>>>>> @@ -717,9 +746,19 @@ static int aw86927_detect(struct aw86927_data *haptics)
>>>>>>  
>>>>>>  	chip_id = be16_to_cpu(read_buf);
>>>>>>  
>>>>>> -	if (chip_id != AW86927_CHIPID) {
>>>>>> -		dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
>>>>>> -		return -ENODEV;
>>>>>> +	switch (haptics->model) {
>>>>>> +	case AW86927:
>>>>>> +		if (chip_id != AW86927_CHIPID) {
>>>>>> +			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
>>>>>> +			return -ENODEV;
>>>>>> +		}
>>>>>
>>>>> If we are able to query chip ID why do we need to have separate
>>>>> compatibles? I would define chip data structure with differences between
>>>>> variants and assign and use it instead of having separate compatible.
>>>>
>>>> dt-bindings guidelines explicitly call for this, a chipid comparison
>>>> then works as a safety net
>>>
>>> Are you saying, that
>>>
>>> 1. we should enforce dt-bindings == CHIP_ID (what's currently done)
>>
>> This
> 
> No. If there is a compatible chip with different ID (for whatever reason
> - maybe there is additional functionality that either board does not
> need or the driver does not implement) we absolutely should not refuse
> to bind the driver.
> 
> Hint: this thing is called _compatible_ for a reason.

Right, the reason you have in mind is fulfilled by fallback compatibles

(i.e. "vendor,actualchipname", "vendor,similarchipname" where the driver
only considers the latter becuase the software interface hasn't changed)

> 
>>
>>>
>>> or
>>>
>>> 2. we should have both compatibles with no handling based on compatible,
>>>    but only use CHIP_ID at runtime to change behavior
>>
>> This is spaghetti
> 
> I really do not understand the aversion of DT maintainers to generic
> compatibles. We see this in I2C HID where we keep adding compatibles
> for what could be described via device properties.

This is because it's the only way to allow for retroactive changes that
do not require changing firmware. That's why ACPI carries new identifiers
for even very slightly different devices too. Once the firmware containing
(ACPI tables / DTB) is put on a production device, it is generally not
going to ever change.

CHIP_ID registers are a good tool to validate that the author of the
firmware table is doing the right thing, but solely relying on them
encourages creating a "vendor,haptic" compatible, which I'm sure you'll
agree is totally meaningless.

That's especially if the naming scheme makes no sense and you can't
even factor out a common wildcard-name (which also happens to be the case
quite often)

Plus a compatible is used to restrict/modify the set of allowed/required
properties, so having an "actual" compatible is required for schema
validation to work

Konrad

Re: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[Dmitry Torokhov]

On Mon, Feb 02, 2026 at 04:11:51PM +0100, Konrad Dybcio wrote:
> On 2/2/26 12:04 PM, Dmitry Torokhov wrote:
> > On Mon, Feb 02, 2026 at 11:19:36AM +0100, Konrad Dybcio wrote:
> >> On 2/2/26 11:14 AM, Luca Weiss wrote:
> >>> Hi Konrad,
> >>>
> >>> On Mon Feb 2, 2026 at 11:12 AM CET, Konrad Dybcio wrote:
> >>>> On 2/1/26 2:49 AM, Dmitry Torokhov wrote:
> >>>>> Hi Griffin,
> >>>>>
> >>>>> On Wed, Jan 28, 2026 at 04:51:14PM +0100, Griffin Kroah-Hartman wrote:
> >>>>>> @@ -717,9 +746,19 @@ static int aw86927_detect(struct aw86927_data *haptics)
> >>>>>>  
> >>>>>>  	chip_id = be16_to_cpu(read_buf);
> >>>>>>  
> >>>>>> -	if (chip_id != AW86927_CHIPID) {
> >>>>>> -		dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
> >>>>>> -		return -ENODEV;
> >>>>>> +	switch (haptics->model) {
> >>>>>> +	case AW86927:
> >>>>>> +		if (chip_id != AW86927_CHIPID) {
> >>>>>> +			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
> >>>>>> +			return -ENODEV;
> >>>>>> +		}
> >>>>>
> >>>>> If we are able to query chip ID why do we need to have separate
> >>>>> compatibles? I would define chip data structure with differences between
> >>>>> variants and assign and use it instead of having separate compatible.
> >>>>
> >>>> dt-bindings guidelines explicitly call for this, a chipid comparison
> >>>> then works as a safety net
> >>>
> >>> Are you saying, that
> >>>
> >>> 1. we should enforce dt-bindings == CHIP_ID (what's currently done)
> >>
> >> This
> > 
> > No. If there is a compatible chip with different ID (for whatever reason
> > - maybe there is additional functionality that either board does not
> > need or the driver does not implement) we absolutely should not refuse
> > to bind the driver.
> > 
> > Hint: this thing is called _compatible_ for a reason.
> 
> Right, the reason you have in mind is fulfilled by fallback compatibles
> 
> (i.e. "vendor,actualchipname", "vendor,similarchipname" where the driver
> only considers the latter becuase the software interface hasn't changed)

And having chip_id checks will break this...

> 
> > 
> >>
> >>>
> >>> or
> >>>
> >>> 2. we should have both compatibles with no handling based on compatible,
> >>>    but only use CHIP_ID at runtime to change behavior
> >>
> >> This is spaghetti
> > 
> > I really do not understand the aversion of DT maintainers to generic
> > compatibles. We see this in I2C HID where we keep adding compatibles
> > for what could be described via device properties.
> 
> This is because it's the only way to allow for retroactive changes that
> do not require changing firmware. That's why ACPI carries new identifiers
> for even very slightly different devices too. Once the firmware containing
> (ACPI tables / DTB) is put on a production device, it is generally not
> going to ever change.

They are actually solving slightly different problem. In ACPI world they
allocate a new ID to represent a peripheral in a given design, down to
it's firmware behavior. It encodes much more than chip ID that DT
maintainers want to key off of.

> 
> CHIP_ID registers are a good tool to validate that the author of the
> firmware table is doing the right thing, but solely relying on them
> encourages creating a "vendor,haptic" compatible, which I'm sure you'll
> agree is totally meaningless.

Is it? If a piece of hardware speaks i2c-hid protocol why do I need to
know the exact chip that is being used? Depending on the chassis and the
size of the sensing element and the version of the firmware that is
loaded into it the behavior and timings of the same chip may be very
different.

> 
> That's especially if the naming scheme makes no sense and you can't
> even factor out a common wildcard-name (which also happens to be the case
> quite often)
> 
> Plus a compatible is used to restrict/modify the set of allowed/required
> properties, so having an "actual" compatible is required for schema
> validation to work

Yes, in cases where there is not a common set of properties having
different compatibles makes sense. But in cases when the device is
supposed to have vendor-agnostic behavior insisting on myriad
compatibles makes little sense.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[Konrad Dybcio]

On 2/3/26 10:49 AM, Dmitry Torokhov wrote:
> On Mon, Feb 02, 2026 at 04:11:51PM +0100, Konrad Dybcio wrote:
>> On 2/2/26 12:04 PM, Dmitry Torokhov wrote:
>>> On Mon, Feb 02, 2026 at 11:19:36AM +0100, Konrad Dybcio wrote:
>>>> On 2/2/26 11:14 AM, Luca Weiss wrote:
>>>>> Hi Konrad,
>>>>>
>>>>> On Mon Feb 2, 2026 at 11:12 AM CET, Konrad Dybcio wrote:
>>>>>> On 2/1/26 2:49 AM, Dmitry Torokhov wrote:
>>>>>>> Hi Griffin,
>>>>>>>
>>>>>>> On Wed, Jan 28, 2026 at 04:51:14PM +0100, Griffin Kroah-Hartman wrote:
>>>>>>>> @@ -717,9 +746,19 @@ static int aw86927_detect(struct aw86927_data *haptics)
>>>>>>>>  
>>>>>>>>  	chip_id = be16_to_cpu(read_buf);
>>>>>>>>  
>>>>>>>> -	if (chip_id != AW86927_CHIPID) {
>>>>>>>> -		dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
>>>>>>>> -		return -ENODEV;
>>>>>>>> +	switch (haptics->model) {
>>>>>>>> +	case AW86927:
>>>>>>>> +		if (chip_id != AW86927_CHIPID) {
>>>>>>>> +			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
>>>>>>>> +			return -ENODEV;
>>>>>>>> +		}
>>>>>>>
>>>>>>> If we are able to query chip ID why do we need to have separate
>>>>>>> compatibles? I would define chip data structure with differences between
>>>>>>> variants and assign and use it instead of having separate compatible.
>>>>>>
>>>>>> dt-bindings guidelines explicitly call for this, a chipid comparison
>>>>>> then works as a safety net
>>>>>
>>>>> Are you saying, that
>>>>>
>>>>> 1. we should enforce dt-bindings == CHIP_ID (what's currently done)
>>>>
>>>> This
>>>
>>> No. If there is a compatible chip with different ID (for whatever reason
>>> - maybe there is additional functionality that either board does not
>>> need or the driver does not implement) we absolutely should not refuse
>>> to bind the driver.
>>>
>>> Hint: this thing is called _compatible_ for a reason.
>>
>> Right, the reason you have in mind is fulfilled by fallback compatibles
>>
>> (i.e. "vendor,actualchipname", "vendor,similarchipname" where the driver
>> only considers the latter becuase the software interface hasn't changed)
> 
> And having chip_id checks will break this...

Depends on how they're implemented and how different the chips are.

If the software interface is exactly 1:1, with the only difference in the
ID register, something like

if (chipid == SIMILARCHIPNAME_ID || chipid == ACTUALCHIPNAME_ID)
	...

would be fitting.

However, more often than not, like in this case, there's actual differences
that need to be taken into account, meaning we already need to act upon
the "actual" compatible

> 
>>
>>>
>>>>
>>>>>
>>>>> or
>>>>>
>>>>> 2. we should have both compatibles with no handling based on compatible,
>>>>>    but only use CHIP_ID at runtime to change behavior
>>>>
>>>> This is spaghetti
>>>
>>> I really do not understand the aversion of DT maintainers to generic
>>> compatibles. We see this in I2C HID where we keep adding compatibles
>>> for what could be described via device properties.
>>
>> This is because it's the only way to allow for retroactive changes that
>> do not require changing firmware. That's why ACPI carries new identifiers
>> for even very slightly different devices too. Once the firmware containing
>> (ACPI tables / DTB) is put on a production device, it is generally not
>> going to ever change.
> 
> They are actually solving slightly different problem. In ACPI world they
> allocate a new ID to represent a peripheral in a given design, down to
> it's firmware behavior. It encodes much more than chip ID that DT
> maintainers want to key off of.

DT sort of does this to. In the Qualcomm world, how you get to interact
with the platform changes dramatically depending on the firmware flavor
it's flashed with (which I'd hope to see go away one day..) - if you have
a Chrome firmware, you're basically free/required to configure everything
from Linux. With Android firmware, much of that heavy lifting must be done
by the hypervisor or the secure world. With Windows firmware, you get the
Android experience + UEFI services. And at the tail end of the scale,
there's the automotive firmware where you don't even get to toggle clocks,
but instead all peripherals' power and performance states are exposed
through dozens of SCMI servers..

Somehow we can try and be smart, deducing the behavior based on the
properties present in DT, but often times, a separate compatible for
"this SoC except with this firmware" needs to exist, as the OS-accessible
software interface is simply different, as if this wasn't the same SoC
anymore.

> 
>>
>> CHIP_ID registers are a good tool to validate that the author of the
>> firmware table is doing the right thing, but solely relying on them
>> encourages creating a "vendor,haptic" compatible, which I'm sure you'll
>> agree is totally meaningless.
> 
> Is it? If a piece of hardware speaks i2c-hid protocol why do I need to
> know the exact chip that is being used? Depending on the chassis and the
> size of the sensing element and the version of the firmware that is
> loaded into it the behavior and timings of the same chip may be very
> different.

I agree this argument gets overused at times

>>
>> That's especially if the naming scheme makes no sense and you can't
>> even factor out a common wildcard-name (which also happens to be the case
>> quite often)
>>
>> Plus a compatible is used to restrict/modify the set of allowed/required
>> properties, so having an "actual" compatible is required for schema
>> validation to work
> 
> Yes, in cases where there is not a common set of properties having
> different compatibles makes sense. But in cases when the device is
> supposed to have vendor-agnostic behavior insisting on myriad
> compatibles makes little sense.

Some people may be thrown off by the golden rule of implementing standards,
which is to break or bend them immediately, claiming full compatibility.

But for cases like hid-over-i2c, I symphatize with the "no one needs
to know if it's a synaptics a00001 or a synaptics a00002" sentiment

Konrad

Re: [PATCH v2 2/3] Input: aw86938 - add driver for Awinic AW86938

[Krzysztof Kozlowski]

On Mon, Feb 02, 2026 at 11:12:26AM +0100, Konrad Dybcio wrote:
> On 2/1/26 2:49 AM, Dmitry Torokhov wrote:
> > Hi Griffin,
> > 
> > On Wed, Jan 28, 2026 at 04:51:14PM +0100, Griffin Kroah-Hartman wrote:
> >> @@ -717,9 +746,19 @@ static int aw86927_detect(struct aw86927_data *haptics)
> >>  
> >>  	chip_id = be16_to_cpu(read_buf);
> >>  
> >> -	if (chip_id != AW86927_CHIPID) {
> >> -		dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
> >> -		return -ENODEV;
> >> +	switch (haptics->model) {
> >> +	case AW86927:
> >> +		if (chip_id != AW86927_CHIPID) {
> >> +			dev_err(haptics->dev, "Unexpected CHIPID value 0x%x\n", chip_id);
> >> +			return -ENODEV;
> >> +		}
> > 
> > If we are able to query chip ID why do we need to have separate
> > compatibles? I would define chip data structure with differences between
> > variants and assign and use it instead of having separate compatible.
> 
> dt-bindings guidelines explicitly call for this, a chipid comparison

No, they don't. If devices offer autodetection, then they are in fact
fully compatible for the SW.

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: input: awinic,aw86927: Add Awinic AW86938

[Krzysztof Kozlowski]

On Wed, Jan 28, 2026 at 04:51:13PM +0100, Griffin Kroah-Hartman wrote:
> Add bindings for the Awinic AW86938 haptic chip which can be found in
> smartphones. These two chips require a similar devicetree configuration,
> but have a register layout that's not 100% compatible.

That would be fine if I did not look at your driver which clearly
suggest there is detection mechanism. Reliable detection by
vendor/programming interface means devices are fully compatible, so
fallback.


Best regards,
Krzysztof