Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date:   Thu, 28 Sep 2023 19:56:45 +0200
Message-Id: <CVUQTCJBG265.1MTSYOLY0FR7Q@gimli.ms.mff.cuni.cz>
Cc:     "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>,
        "Markuss Broks" <markuss.broks@gmail.com>,
        <linux-input@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        =?utf-8?q?Duje_Mihanovi=C4=87?= <duje.mihanovic@skole.hr>,
        <~postmarketos/upstreaming@lists.sr.ht>
Subject: Re: [PATCH 2/2] input: Imagis: add support for the IST3032C
 touchscreen
To:     "Jeff LaBundy" <jeff@labundy.com>, "Karel Balej" <balejk@matfyz.cz>
From:   "Karel Balej" <karelb@gimli.ms.mff.cuni.cz>
References: <20230926173531.18715-1-balejk@matfyz.cz>
 <20230926173531.18715-3-balejk@matfyz.cz> <ZROIKSVCiTI3VB2B@nixie71>
In-Reply-To: <ZROIKSVCiTI3VB2B@nixie71>
Precedence: bulk

Hello, Jeff,

thank you very much for your feedback.

> > +	if (chip_id =3D=3D IST3032C_WHOAMI) {
> > +		/*
> > +		 * if the regulator voltage is not set like this, the touchscreen
> > +		 * generates random touches without user interaction
> > +		 */
> > +		error =3D regulator_set_voltage(ts->supplies[0].consumer, 3100000, 3=
100000);
> > +		if (error)
> > +			dev_warn(dev, "failed to set regulator voltage\n");
> > +	}
> > +
>
> Opinions may vary, but mine is that this kind of hard-coded board-level p=
olicy
> does not belong in the driver. Surely the supply need not be equal to exa=
ctly
> 3.1 V and this is merely an upper or lower limit? Assuming so, what if th=
e board
> designer opts to share this supply with another consumer that requires a =
specific
> voltage not equal to 3.1 V, but still within the safe range of IST3032C?
>
> To me, this restriction belongs in dts, specifically within the regulator=
 child
> node referenced by the client which bears the new 'ist3032c' compatible s=
tring.
> Maybe a better solution is to simply note this presumed silicon erratum i=
n the
> description of the vdd supply in the binding which, as Conor states, must=
 not be
> clubbed with driver patches.

I agree that the voltage should not be hardcoded. I do not know what the
safe range for the touchscreen is though, because the downstream driver
does exactly this. I will try to test it with several values within the
range allowed by the regulator and see if I can determine some limits on
when the "ghost" touches do not appear.

However I am not sure whether this setting should be moved to the
regulator DT - it is my understanding that the DT for the regulator
should list the min/max range *supported* by the regulator, not conform
to requirements of its consumers, which should instead ask for the
regulator to be set to a range they require themselves, via their driver
- is it not so?

The regulator driver is not mainlined yet (although I managed to get the
downstream code working with mainline), however the downstream DT
contains much wider range of supported voltage (compared to those 3.1 V
used by the touchscreen) - an information which would get lost if I set
the DT for the regulator by the requirements of the touchscreen, which I
believe would have similiar implications as what you said regarding
using this regulator with other consumers.

What would seem a reasonable solution to me would be to move the voltage
range values to the touchscreen DT (which incidentally is what the
downstream driver does also, except it uses one value for both min and
max), so that they would be set by the driver but not hardcoded in the
code - what do you think about this?

Best regards,
K. B.