[PATCH v3 0/5] input/touchscreen: imagis: add support for IST3032C

[PATCH v3 0/5] input/touchscreen: imagis: add support for IST3032C

[Karel Balej]

From: Karel Balej <balejk@matfyz.cz>

This patch series generalizes the Imagis touchscreen driver to support
other Imagis chips, namely IST3038B, which use a slightly different
protocol.

It also adds necessary information to the driver so that the IST3032C
touchscreen can be used with it. The motivation for this is the
samsung,coreprimevelte smartphone with which this series has been
tested. However, the support for this device is not yet in-tree, the
effort is happening at [1]. In particular, the driver for the regulator
needed by the touchscreen on this device has not been rewritten for
mainline yet.

Note that this is a prerequisite for this patch [2] which implements
support for touch keys for Imagis touchscreens that have it.

[1] https://lore.kernel.org/all/20231102-pxa1908-lkml-v7-0-cabb1a0cb52b@skole.hr/
[2] https://lore.kernel.org/all/20231112194124.24916-1-duje.mihanovic@skole.hr/
---
v3:
- Rebase to v6.7-rc3.
- v2: https://lore.kernel.org/all/20231003133440.4696-1-karelb@gimli.ms.mff.cuni.cz/
v2:
- Do not rename the driver.
- Do not hardcode voltage required by the IST3032C.
- Use Markuss' series which generalizes the driver. Link to the original
  series: https://lore.kernel.org/all/20220504152406.8730-1-markuss.broks@gmail.com/
- Separate bindings into separate patch.
- v1: https://lore.kernel.org/all/20230926173531.18715-1-balejk@matfyz.cz/
---

Karel Balej (2):
  dt-bindings: input/touchscreen: imagis: add compatible for IST3032C
  input/touchscreen: imagis: add support for IST3032C

Markuss Broks (3):
  input/touchscreen: imagis: Correct the maximum touch area value
  dt-bindings: input/touchscreen: Add compatible for IST3038B
  input/touchscreen: imagis: Add support for Imagis IST3038B

 .../input/touchscreen/imagis,ist3038c.yaml    |  2 +
 drivers/input/touchscreen/imagis.c            | 70 +++++++++++++++----
 2 files changed, 60 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH v3 1/5] input/touchscreen: imagis: Correct the maximum touch area value

[Karel Balej]

From: Markuss Broks <markuss.broks@gmail.com>

As specified in downstream IST3038B driver and proved by testing,
the correct maximum reported value of touch area is 16.

Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 drivers/input/touchscreen/imagis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
index 07111ca24455..e67fd3011027 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -210,7 +210,7 @@ static int imagis_init_input_dev(struct imagis_ts *ts)
 
 	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
 	input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
-	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+	input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 16, 0, 0);
 
 	touchscreen_parse_properties(input_dev, true, &ts->prop);
 	if (!ts->prop.max_x || !ts->prop.max_y) {
-- 
2.43.0

[PATCH v3 2/5] dt-bindings: input/touchscreen: Add compatible for IST3038B

[Karel Balej]

From: Markuss Broks <markuss.broks@gmail.com>

Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC,
add the compatible for it to the IST3038C bindings.

Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml   | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
index 0d6b033fd5fb..b5372c4eae56 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
@@ -18,6 +18,7 @@ properties:
 
   compatible:
     enum:
+      - imagis,ist3038b
       - imagis,ist3038c
 
   reg:
-- 
2.43.0

[PATCH v3 3/5] input/touchscreen: imagis: Add support for Imagis IST3038B

[Karel Balej]

From: Markuss Broks <markuss.broks@gmail.com>

Imagis IST3038B is another variant of Imagis IST3038 IC, which has
a different register interface from IST3038C (possibly firmware defined).
This should also work for IST3044B (though untested), however other
variants using this interface/protocol(IST3026, IST3032, IST3026B,
IST3032B) have a different format for coordinates, and they'd need
additional effort to be supported by this driver.

Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 drivers/input/touchscreen/imagis.c | 58 ++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 11 deletions(-)

diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
index e67fd3011027..84a02672ac47 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -13,7 +13,7 @@
 
 #define IST3038C_HIB_ACCESS		(0x800B << 16)
 #define IST3038C_DIRECT_ACCESS		BIT(31)
-#define IST3038C_REG_CHIPID		0x40001000
+#define IST3038C_REG_CHIPID		(0x40001000 | IST3038C_DIRECT_ACCESS)
 #define IST3038C_REG_HIB_BASE		0x30000100
 #define IST3038C_REG_TOUCH_STATUS	(IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS)
 #define IST3038C_REG_TOUCH_COORD	(IST3038C_REG_HIB_BASE | IST3038C_HIB_ACCESS | 0x8)
@@ -31,8 +31,21 @@
 #define IST3038C_FINGER_COUNT_SHIFT	12
 #define IST3038C_FINGER_STATUS_MASK	GENMASK(9, 0)
 
+#define IST3038B_REG_STATUS		0x20
+#define IST3038B_REG_CHIPID		0x30
+#define IST3038B_WHOAMI			0x30380b
+
+struct imagis_properties {
+	unsigned int interrupt_msg_cmd;
+	unsigned int touch_coord_cmd;
+	unsigned int whoami_cmd;
+	unsigned int whoami_val;
+	bool protocol_b;
+};
+
 struct imagis_ts {
 	struct i2c_client *client;
+	const struct imagis_properties *tdata;
 	struct input_dev *input_dev;
 	struct touchscreen_properties prop;
 	struct regulator_bulk_data supplies[2];
@@ -84,8 +97,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
 	int i;
 	int error;
 
-	error = imagis_i2c_read_reg(ts, IST3038C_REG_INTR_MESSAGE,
-				    &intr_message);
+	error = imagis_i2c_read_reg(ts, ts->tdata->interrupt_msg_cmd, &intr_message);
 	if (error) {
 		dev_err(&ts->client->dev,
 			"failed to read the interrupt message: %d\n", error);
@@ -104,9 +116,13 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
 	finger_pressed = intr_message & IST3038C_FINGER_STATUS_MASK;
 
 	for (i = 0; i < finger_count; i++) {
-		error = imagis_i2c_read_reg(ts,
-					    IST3038C_REG_TOUCH_COORD + (i * 4),
-					    &finger_status);
+		if (ts->tdata->protocol_b)
+			error = imagis_i2c_read_reg(ts,
+						    ts->tdata->touch_coord_cmd, &finger_status);
+		else
+			error = imagis_i2c_read_reg(ts,
+						    ts->tdata->touch_coord_cmd + (i * 4),
+						    &finger_status);
 		if (error) {
 			dev_err(&ts->client->dev,
 				"failed to read coordinates for finger %d: %d\n",
@@ -261,6 +277,12 @@ static int imagis_probe(struct i2c_client *i2c)
 
 	ts->client = i2c;
 
+	ts->tdata = device_get_match_data(dev);
+	if (!ts->tdata) {
+		dev_err(dev, "missing chip data\n");
+		return -EINVAL;
+	}
+
 	error = imagis_init_regulators(ts);
 	if (error) {
 		dev_err(dev, "regulator init error: %d\n", error);
@@ -279,15 +301,13 @@ static int imagis_probe(struct i2c_client *i2c)
 		return error;
 	}
 
-	error = imagis_i2c_read_reg(ts,
-			IST3038C_REG_CHIPID | IST3038C_DIRECT_ACCESS,
-			&chip_id);
+	error = imagis_i2c_read_reg(ts, ts->tdata->whoami_cmd, &chip_id);
 	if (error) {
 		dev_err(dev, "chip ID read failure: %d\n", error);
 		return error;
 	}
 
-	if (chip_id != IST3038C_WHOAMI) {
+	if (chip_id != ts->tdata->whoami_val) {
 		dev_err(dev, "unknown chip ID: 0x%x\n", chip_id);
 		return -EINVAL;
 	}
@@ -343,9 +363,25 @@ static int imagis_resume(struct device *dev)
 
 static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume);
 
+static const struct imagis_properties imagis_3038b_data = {
+	.interrupt_msg_cmd = IST3038B_REG_STATUS,
+	.touch_coord_cmd = IST3038B_REG_STATUS,
+	.whoami_cmd = IST3038B_REG_CHIPID,
+	.whoami_val = IST3038B_WHOAMI,
+	.protocol_b = true,
+};
+
+static const struct imagis_properties imagis_3038c_data = {
+	.interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
+	.touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
+	.whoami_cmd = IST3038C_REG_CHIPID,
+	.whoami_val = IST3038C_WHOAMI,
+};
+
 #ifdef CONFIG_OF
 static const struct of_device_id imagis_of_match[] = {
-	{ .compatible = "imagis,ist3038c", },
+	{ .compatible = "imagis,ist3038b", .data = &imagis_3038b_data },
+	{ .compatible = "imagis,ist3038c", .data = &imagis_3038c_data },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, imagis_of_match);
-- 
2.43.0

[PATCH v3 4/5] dt-bindings: input/touchscreen: imagis: add compatible for IST3032C

[Karel Balej]

From: Karel Balej <balejk@matfyz.cz>

Document possible usage of the Imagis driver with the IST3032C
touchscreen.

Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml   | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
index b5372c4eae56..2af71cbcc97d 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
@@ -18,6 +18,7 @@ properties:
 
   compatible:
     enum:
+      - imagis,ist3032c
       - imagis,ist3038b
       - imagis,ist3038c
 
-- 
2.43.0

[PATCH v3 5/5] input/touchscreen: imagis: add support for IST3032C

[Karel Balej]

From: Karel Balej <balejk@matfyz.cz>

IST3032C is a touchscreen chip used for instance in the
samsung,coreprimevelte smartphone, with which this was tested. Add the
chip specific information to the driver.

Signed-off-by: Karel Balej <balejk@matfyz.cz>
---
 drivers/input/touchscreen/imagis.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
index 84a02672ac47..41f28e6e9cb1 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -35,6 +35,8 @@
 #define IST3038B_REG_CHIPID		0x30
 #define IST3038B_WHOAMI			0x30380b
 
+#define IST3032C_WHOAMI			0x32c
+
 struct imagis_properties {
 	unsigned int interrupt_msg_cmd;
 	unsigned int touch_coord_cmd;
@@ -363,6 +365,13 @@ static int imagis_resume(struct device *dev)
 
 static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume);
 
+static const struct imagis_properties imagis_3032c_data = {
+	.interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
+	.touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
+	.whoami_cmd = IST3038C_REG_CHIPID,
+	.whoami_val = IST3032C_WHOAMI,
+};
+
 static const struct imagis_properties imagis_3038b_data = {
 	.interrupt_msg_cmd = IST3038B_REG_STATUS,
 	.touch_coord_cmd = IST3038B_REG_STATUS,
@@ -380,6 +389,7 @@ static const struct imagis_properties imagis_3038c_data = {
 
 #ifdef CONFIG_OF
 static const struct of_device_id imagis_of_match[] = {
+	{ .compatible = "imagis,ist3032c", .data = &imagis_3032c_data },
 	{ .compatible = "imagis,ist3038b", .data = &imagis_3038b_data },
 	{ .compatible = "imagis,ist3038c", .data = &imagis_3038c_data },
 	{ },
-- 
2.43.0

Re: [PATCH v3 4/5] dt-bindings: input/touchscreen: imagis: add compatible for IST3032C

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On Sat, Dec 02, 2023 at 01:48:35PM +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
> 
> Document possible usage of the Imagis driver with the IST3032C
> touchscreen.

Please leave mention of the driver out of the binding patch (we deal
only with the hardware here) and instead describe what is incompatibly
different between these two devices.

Thanks,
Conor.

> 
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>  .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml   | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> index b5372c4eae56..2af71cbcc97d 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> @@ -18,6 +18,7 @@ properties:
>  
>    compatible:
>      enum:
> +      - imagis,ist3032c
>        - imagis,ist3038b
>        - imagis,ist3038c
>  
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/5] dt-bindings: input/touchscreen: Add compatible for IST3038B

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

On Sat, Dec 02, 2023 at 01:48:33PM +0100, Karel Balej wrote:
> From: Markuss Broks <markuss.broks@gmail.com>
> 
> Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC,
> add the compatible for it to the IST3038C bindings.

This one is better, but would be well served by mentioning what
specifically is different (register addresses or firmware commands?)

Cheers,
Conor.

> 
> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>  .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml   | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> index 0d6b033fd5fb..b5372c4eae56 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> @@ -18,6 +18,7 @@ properties:
>  
>    compatible:
>      enum:
> +      - imagis,ist3038b
>        - imagis,ist3038c
>  
>    reg:
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/5] dt-bindings: input/touchscreen: Add compatible for IST3038B

[Markuss Broks]

Hi Conor,

On 12/3/23 13:20, Conor Dooley wrote:
> On Sat, Dec 02, 2023 at 01:48:33PM +0100, Karel Balej wrote:
>> From: Markuss Broks <markuss.broks@gmail.com>
>>
>> Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC,
>> add the compatible for it to the IST3038C bindings.
> This one is better, but would be well served by mentioning what
> specifically is different (register addresses or firmware commands?)

I don't think anyone knows this other than Imagis itself. I would guess 
it's different hardware, since register addresses are indeed different, 
but on the other hand, there is a possibility that firmware on the MCU 
could be responding to those commands. I suppose "... IST3038B is a 
hardware variant of ... IST3038" would be more correct.

The reason why I think it could be firmware-defined is because we have a 
lot of variants (30xxA, 30xxB, 30xxC, plain 30xx), and the numbers 
usually mean feature level/completeness, e.g. some don't support the 
touch pressure or touchkeys, and we don't know what A/B/C/none means.

>
> Cheers,
> Conor.
>
>> Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
>> Signed-off-by: Karel Balej <balejk@matfyz.cz>
>> ---
>>   .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml   | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
>> index 0d6b033fd5fb..b5372c4eae56 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
>> @@ -18,6 +18,7 @@ properties:
>>   
>>     compatible:
>>       enum:
>> +      - imagis,ist3038b
>>         - imagis,ist3038c
>>   
>>     reg:
>> -- 
>> 2.43.0
>>
- Markuss

Re: [PATCH v3 5/5] input/touchscreen: imagis: add support for IST3032C

[Markuss Broks]

Hi Karel,

On 12/2/23 14:48, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
>
> IST3032C is a touchscreen chip used for instance in the
> samsung,coreprimevelte smartphone, with which this was tested. Add the
> chip specific information to the driver.
>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>   drivers/input/touchscreen/imagis.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
> index 84a02672ac47..41f28e6e9cb1 100644
> --- a/drivers/input/touchscreen/imagis.c
> +++ b/drivers/input/touchscreen/imagis.c
> @@ -35,6 +35,8 @@
>   #define IST3038B_REG_CHIPID		0x30
>   #define IST3038B_WHOAMI			0x30380b
>   
> +#define IST3032C_WHOAMI			0x32c
> +
Perhaps it should be ordered in alphabetic/alphanumeric order, 
alternatively, the chip ID values could be grouped.
>   struct imagis_properties {
>   	unsigned int interrupt_msg_cmd;
>   	unsigned int touch_coord_cmd;
> @@ -363,6 +365,13 @@ static int imagis_resume(struct device *dev)
>   
>   static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume);
>   
> +static const struct imagis_properties imagis_3032c_data = {
> +	.interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
> +	.touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
> +	.whoami_cmd = IST3038C_REG_CHIPID,
> +	.whoami_val = IST3032C_WHOAMI,
> +};
> +
>   static const struct imagis_properties imagis_3038b_data = {
>   	.interrupt_msg_cmd = IST3038B_REG_STATUS,
>   	.touch_coord_cmd = IST3038B_REG_STATUS,
> @@ -380,6 +389,7 @@ static const struct imagis_properties imagis_3038c_data = {
>   
>   #ifdef CONFIG_OF
>   static const struct of_device_id imagis_of_match[] = {
> +	{ .compatible = "imagis,ist3032c", .data = &imagis_3032c_data },
>   	{ .compatible = "imagis,ist3038b", .data = &imagis_3038b_data },
>   	{ .compatible = "imagis,ist3038c", .data = &imagis_3038c_data },
>   	{ },

Other than that,

Reviewed-by: Markuss Broks <markuss.broks@gmail.com>

- Markuss

Re: [PATCH v3 2/5] dt-bindings: input/touchscreen: Add compatible for IST3038B

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2770 bytes --]

On Mon, Dec 04, 2023 at 02:40:44PM +0200, Markuss Broks wrote:
> On 12/3/23 13:20, Conor Dooley wrote:
> > On Sat, Dec 02, 2023 at 01:48:33PM +0100, Karel Balej wrote:
> > > From: Markuss Broks <markuss.broks@gmail.com>
> > > 
> > > Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC,
> > > add the compatible for it to the IST3038C bindings.
> > This one is better, but would be well served by mentioning what
> > specifically is different (register addresses or firmware commands?)
> 
> I don't think anyone knows this other than Imagis itself. I would guess it's
> different hardware, since register addresses are indeed different, but on
> the other hand, there is a possibility that firmware on the MCU could be
> responding to those commands. I suppose "... IST3038B is a hardware variant
> of ... IST3038" would be more correct.

Only Imagis might know the specifics, but you (plural) have made driver
changes so you know what is different in terms of the programming model.
I'm just asking for you to mention how the programming model varies in
the commit message. Otherwise I can't know whether you should have added
a fallback compatible, without going and reading your driver change. The
commit message for the bindings should stand on its own merit in that
regard.
"Variant" alone does not suffice, as many variants of devices have a
compatible programming model, be that for a subset of features or
complete compatibility.

> The reason why I think it could be firmware-defined is because we have a lot
> of variants (30xxA, 30xxB, 30xxC, plain 30xx), and the numbers usually mean
> feature level/completeness, e.g. some don't support the touch pressure or
> touchkeys, and we don't know what A/B/C/none means.

Ultimately whether it is due to firmware or the hardware isn't
particular important, just mention what is incompatibly different.

Cheers,
Conor.


> > > Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> > > Signed-off-by: Karel Balej <balejk@matfyz.cz>
> > > ---
> > >   .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml   | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> > > index 0d6b033fd5fb..b5372c4eae56 100644
> > > --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> > > +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> > > @@ -18,6 +18,7 @@ properties:
> > >     compatible:
> > >       enum:
> > > +      - imagis,ist3038b
> > >         - imagis,ist3038c
> > >     reg:
> > > -- 
> > > 2.43.0
> > > 
> - Markuss

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 3/5] input/touchscreen: imagis: Add support for Imagis IST3038B

[kernel test robot]

Hi Karel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on dtor-input/next]
[also build test WARNING on dtor-input/for-linus robh/for-next linus/master v6.7-rc4 next-20231205]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Karel-Balej/dt-bindings-input-touchscreen-Add-compatible-for-IST3038B/20231202-215030
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link:    https://lore.kernel.org/r/20231202125948.10345-4-karelb%40gimli.ms.mff.cuni.cz
patch subject: [PATCH v3 3/5] input/touchscreen: imagis: Add support for Imagis IST3038B
config: hexagon-randconfig-r111-20231204 (https://download.01.org/0day-ci/archive/20231205/202312051823.19Cxz3sZ-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20231205/202312051823.19Cxz3sZ-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/202312051823.19Cxz3sZ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/input/touchscreen/imagis.c:5:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/input/touchscreen/imagis.c:5:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/input/touchscreen/imagis.c:5:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/input/touchscreen/imagis.c:366:39: warning: unused variable 'imagis_3038b_data' [-Wunused-const-variable]
     366 | static const struct imagis_properties imagis_3038b_data = {
         |                                       ^
>> drivers/input/touchscreen/imagis.c:374:39: warning: unused variable 'imagis_3038c_data' [-Wunused-const-variable]
     374 | static const struct imagis_properties imagis_3038c_data = {
         |                                       ^
   8 warnings generated.


vim +/imagis_3038b_data +366 drivers/input/touchscreen/imagis.c

   365	
 > 366	static const struct imagis_properties imagis_3038b_data = {
   367		.interrupt_msg_cmd = IST3038B_REG_STATUS,
   368		.touch_coord_cmd = IST3038B_REG_STATUS,
   369		.whoami_cmd = IST3038B_REG_CHIPID,
   370		.whoami_val = IST3038B_WHOAMI,
   371		.protocol_b = true,
   372	};
   373	
 > 374	static const struct imagis_properties imagis_3038c_data = {
   375		.interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
   376		.touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
   377		.whoami_cmd = IST3038C_REG_CHIPID,
   378		.whoami_val = IST3038C_WHOAMI,
   379	};
   380	

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

Re: [PATCH v3 5/5] input/touchscreen: imagis: add support for IST3032C

[kernel test robot]

Hi Karel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on dtor-input/next]
[also build test WARNING on dtor-input/for-linus robh/for-next linus/master v6.7-rc4 next-20231205]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Karel-Balej/dt-bindings-input-touchscreen-Add-compatible-for-IST3038B/20231202-215030
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link:    https://lore.kernel.org/r/20231202125948.10345-6-karelb%40gimli.ms.mff.cuni.cz
patch subject: [PATCH v3 5/5] input/touchscreen: imagis: add support for IST3032C
config: hexagon-randconfig-r111-20231204 (https://download.01.org/0day-ci/archive/20231205/202312052257.8Qd4OcID-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20231205/202312052257.8Qd4OcID-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/202312052257.8Qd4OcID-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/input/touchscreen/imagis.c:5:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     547 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     560 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   In file included from drivers/input/touchscreen/imagis.c:5:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     573 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   In file included from drivers/input/touchscreen/imagis.c:5:
   In file included from include/linux/i2c.h:19:
   In file included from include/linux/regulator/consumer.h:35:
   In file included from include/linux/suspend.h:5:
   In file included from include/linux/swap.h:9:
   In file included from include/linux/memcontrol.h:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:9:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     584 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     594 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     604 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
>> drivers/input/touchscreen/imagis.c:368:39: warning: unused variable 'imagis_3032c_data' [-Wunused-const-variable]
     368 | static const struct imagis_properties imagis_3032c_data = {
         |                                       ^
   drivers/input/touchscreen/imagis.c:375:39: warning: unused variable 'imagis_3038b_data' [-Wunused-const-variable]
     375 | static const struct imagis_properties imagis_3038b_data = {
         |                                       ^
   drivers/input/touchscreen/imagis.c:383:39: warning: unused variable 'imagis_3038c_data' [-Wunused-const-variable]
     383 | static const struct imagis_properties imagis_3038c_data = {
         |                                       ^
   9 warnings generated.


vim +/imagis_3032c_data +368 drivers/input/touchscreen/imagis.c

   367	
 > 368	static const struct imagis_properties imagis_3032c_data = {
   369		.interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
   370		.touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
   371		.whoami_cmd = IST3038C_REG_CHIPID,
   372		.whoami_val = IST3032C_WHOAMI,
   373	};
   374	

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

Re: [PATCH v3 5/5] input/touchscreen: imagis: add support for IST3032C

[Karel Balej]

Markuss,

thank you for the review.

> > diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
> > index 84a02672ac47..41f28e6e9cb1 100644
> > --- a/drivers/input/touchscreen/imagis.c
> > +++ b/drivers/input/touchscreen/imagis.c
> > @@ -35,6 +35,8 @@
> >   #define IST3038B_REG_CHIPID		0x30
> >   #define IST3038B_WHOAMI			0x30380b
> >   
> > +#define IST3032C_WHOAMI			0x32c
> > +

> Perhaps it should be ordered in alphabetic/alphanumeric order, 
> alternatively, the chip ID values could be grouped.

Here I followed suit and just started a new section for the new chip,
except there is only one entry. I do agree that it would be better to
sort the chips alphanumerically and I am actually surprised that I
didn't do that - but now I see that the chips that you added are not
sorted either, so it might be because of that.

I propose to definitely swap the order of the sections, putting 32C
first, then 38B and 38C at the end (from top to bottom). The chip ID
values could then still be grouped in a new section, but I think I would
actually prefer to keep them as parts of the respective sections as it
is now, although it is in no way a strong preference.

Please let me know whether you agree with this or have a different
preference. And if the former, please confirm that I can add your
Reviewed-by trailer to the patch modified in such a way.

Best regards,
K. B.

Re: [PATCH v3 2/5] dt-bindings: input/touchscreen: Add compatible for IST3038B

[Karel Balej]

On Mon Dec 4, 2023 at 1:52 PM CET, Conor Dooley wrote:
> On Mon, Dec 04, 2023 at 02:40:44PM +0200, Markuss Broks wrote:
> > On 12/3/23 13:20, Conor Dooley wrote:
> > > On Sat, Dec 02, 2023 at 01:48:33PM +0100, Karel Balej wrote:
> > > > From: Markuss Broks <markuss.broks@gmail.com>
> > > > 
> > > > Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC,
> > > > add the compatible for it to the IST3038C bindings.
> > > This one is better, but would be well served by mentioning what
> > > specifically is different (register addresses or firmware commands?)
> > 
> > I don't think anyone knows this other than Imagis itself. I would guess it's
> > different hardware, since register addresses are indeed different, but on
> > the other hand, there is a possibility that firmware on the MCU could be
> > responding to those commands. I suppose "... IST3038B is a hardware variant
> > of ... IST3038" would be more correct.
>
> Only Imagis might know the specifics, but you (plural) have made driver
> changes so you know what is different in terms of the programming model.
> I'm just asking for you to mention how the programming model varies in
> the commit message. Otherwise I can't know whether you should have added
> a fallback compatible, without going and reading your driver change. The
> commit message for the bindings should stand on its own merit in that
> regard.
> "Variant" alone does not suffice, as many variants of devices have a
> compatible programming model, be that for a subset of features or
> complete compatibility.
>
> > The reason why I think it could be firmware-defined is because we have a lot
> > of variants (30xxA, 30xxB, 30xxC, plain 30xx), and the numbers usually mean
> > feature level/completeness, e.g. some don't support the touch pressure or
> > touchkeys, and we don't know what A/B/C/none means.
>
> Ultimately whether it is due to firmware or the hardware isn't
> particular important, just mention what is incompatibly different.

I propose to update the commit description as such:

	Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC
	differing from IST3038C in its register interface. Add the
	compatible for it to the IST3038C bindings.

>
> Cheers,
> Conor.
>
>
> > > > Signed-off-by: Markuss Broks <markuss.broks@gmail.com>
> > > > Signed-off-by: Karel Balej <balejk@matfyz.cz>
> > > > ---
> > > >   .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml   | 1 +
> > > >   1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> > > > index 0d6b033fd5fb..b5372c4eae56 100644
> > > > --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml
> > > > @@ -18,6 +18,7 @@ properties:
> > > >     compatible:
> > > >       enum:
> > > > +      - imagis,ist3038b
> > > >         - imagis,ist3038c
> > > >     reg:
> > > > -- 
> > > > 2.43.0
> > > > 
> > - Markuss

Kind regards,
K. B.

Re: [PATCH v3 2/5] dt-bindings: input/touchscreen: Add compatible for IST3038B

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2522 bytes --]

On Sat, Dec 09, 2023 at 10:05:27AM +0100, Karel Balej wrote:
> On Mon Dec 4, 2023 at 1:52 PM CET, Conor Dooley wrote:
> > On Mon, Dec 04, 2023 at 02:40:44PM +0200, Markuss Broks wrote:
> > > On 12/3/23 13:20, Conor Dooley wrote:
> > > > On Sat, Dec 02, 2023 at 01:48:33PM +0100, Karel Balej wrote:
> > > > > From: Markuss Broks <markuss.broks@gmail.com>
> > > > > 
> > > > > Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC,
> > > > > add the compatible for it to the IST3038C bindings.
> > > > This one is better, but would be well served by mentioning what
> > > > specifically is different (register addresses or firmware commands?)
> > > 
> > > I don't think anyone knows this other than Imagis itself. I would guess it's
> > > different hardware, since register addresses are indeed different, but on
> > > the other hand, there is a possibility that firmware on the MCU could be
> > > responding to those commands. I suppose "... IST3038B is a hardware variant
> > > of ... IST3038" would be more correct.
> >
> > Only Imagis might know the specifics, but you (plural) have made driver
> > changes so you know what is different in terms of the programming model.
> > I'm just asking for you to mention how the programming model varies in
> > the commit message. Otherwise I can't know whether you should have added
> > a fallback compatible, without going and reading your driver change. The
> > commit message for the bindings should stand on its own merit in that
> > regard.
> > "Variant" alone does not suffice, as many variants of devices have a
> > compatible programming model, be that for a subset of features or
> > complete compatibility.
> >
> > > The reason why I think it could be firmware-defined is because we have a lot
> > > of variants (30xxA, 30xxB, 30xxC, plain 30xx), and the numbers usually mean
> > > feature level/completeness, e.g. some don't support the touch pressure or
> > > touchkeys, and we don't know what A/B/C/none means.
> >
> > Ultimately whether it is due to firmware or the hardware isn't
> > particular important, just mention what is incompatibly different.
> 
> I propose to update the commit description as such:
> 
> 	Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC
> 	differing from IST3038C in its register interface. Add the
> 	compatible for it to the IST3038C bindings.


SGTM. You can add
Acked-by: Conor Dooley <conor.dooley@microchip.com>
with that commit message update.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 5/5] input/touchscreen: imagis: add support for IST3032C

[Markuss Broks]

Hi Karel,

On 12/8/23 23:59, Karel Balej wrote:
> Markuss,
>
> thank you for the review.
>
>>> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
>>> index 84a02672ac47..41f28e6e9cb1 100644
>>> --- a/drivers/input/touchscreen/imagis.c
>>> +++ b/drivers/input/touchscreen/imagis.c
>>> @@ -35,6 +35,8 @@
>>>    #define IST3038B_REG_CHIPID		0x30
>>>    #define IST3038B_WHOAMI			0x30380b
>>>    
>>> +#define IST3032C_WHOAMI			0x32c
>>> +
>> Perhaps it should be ordered in alphabetic/alphanumeric order,
>> alternatively, the chip ID values could be grouped.
> Here I followed suit and just started a new section for the new chip,
> except there is only one entry. I do agree that it would be better to
> sort the chips alphanumerically and I am actually surprised that I
> didn't do that - but now I see that the chips that you added are not
> sorted either, so it might be because of that.
>
> I propose to definitely swap the order of the sections, putting 32C
> first, then 38B and 38C at the end (from top to bottom). The chip ID
> values could then still be grouped in a new section, but I think I would
> actually prefer to keep them as parts of the respective sections as it
> is now, although it is in no way a strong preference.
We could do that, yeah. It is not a problem right now since there's only 
3 models supported, but it would maker sense and set some order for when 
we'd have more supported devices.
>
> Please let me know whether you agree with this or have a different
> preference. And if the former, please confirm that I can add your
> Reviewed-by trailer to the patch modified in such a way.
Yeah, it's fine.
>
> Best regards,
> K. B.

- Markuss

Re: [PATCH v3 2/5] dt-bindings: input/touchscreen: Add compatible for IST3038B

[Karel Balej]

Markuss,

On Sat Dec 9, 2023 at 11:58 AM CET, Conor Dooley wrote:
> On Sat, Dec 09, 2023 at 10:05:27AM +0100, Karel Balej wrote:
> > On Mon Dec 4, 2023 at 1:52 PM CET, Conor Dooley wrote:
> > > On Mon, Dec 04, 2023 at 02:40:44PM +0200, Markuss Broks wrote:
> > > > On 12/3/23 13:20, Conor Dooley wrote:
> > > > > On Sat, Dec 02, 2023 at 01:48:33PM +0100, Karel Balej wrote:
> > > > > > From: Markuss Broks <markuss.broks@gmail.com>
> > > > > > 
> > > > > > Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC,
> > > > > > add the compatible for it to the IST3038C bindings.
> > > > > This one is better, but would be well served by mentioning what
> > > > > specifically is different (register addresses or firmware commands?)
> > > > 
> > > > I don't think anyone knows this other than Imagis itself. I would guess it's
> > > > different hardware, since register addresses are indeed different, but on
> > > > the other hand, there is a possibility that firmware on the MCU could be
> > > > responding to those commands. I suppose "... IST3038B is a hardware variant
> > > > of ... IST3038" would be more correct.
> > >
> > > Only Imagis might know the specifics, but you (plural) have made driver
> > > changes so you know what is different in terms of the programming model.
> > > I'm just asking for you to mention how the programming model varies in
> > > the commit message. Otherwise I can't know whether you should have added
> > > a fallback compatible, without going and reading your driver change. The
> > > commit message for the bindings should stand on its own merit in that
> > > regard.
> > > "Variant" alone does not suffice, as many variants of devices have a
> > > compatible programming model, be that for a subset of features or
> > > complete compatibility.
> > >
> > > > The reason why I think it could be firmware-defined is because we have a lot
> > > > of variants (30xxA, 30xxB, 30xxC, plain 30xx), and the numbers usually mean
> > > > feature level/completeness, e.g. some don't support the touch pressure or
> > > > touchkeys, and we don't know what A/B/C/none means.
> > >
> > > Ultimately whether it is due to firmware or the hardware isn't
> > > particular important, just mention what is incompatibly different.
> > 
> > I propose to update the commit description as such:
> > 
> > 	Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC
> > 	differing from IST3038C in its register interface. Add the
> > 	compatible for it to the IST3038C bindings.

is this change OK with you?

>
>
> SGTM. You can add
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> with that commit message update.
>
> Thanks,
> Conor.

Kind regards,
K. B.