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

From: Markuss Broks <markuss.broks@gmail.com>
To: Karel Balej <karelb@gimli.ms.mff.cuni.cz>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Henrik Rydberg <rydberg@bitmath.org>,
	linux-input@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Cc: "Duje Mihanović" <duje.mihanovic@skole.hr>,
	~postmarketos/upstreaming@lists.sr.ht,
	phone-devel@vger.kernel.org, "Karel Balej" <balejk@matfyz.cz>
Subject: Re: [PATCH v3 5/5] input/touchscreen: imagis: add support for IST3032C
Date: Sat, 9 Dec 2023 22:50:52 +0200	[thread overview]
Message-ID: <da6d2828-41db-4c29-a1aa-024b1fbcc43a@gmail.com> (raw)
In-Reply-To: <CXJAG826ZTNA.2F8WOGVNYADKP@gimli.ms.mff.cuni.cz>

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