Re: [PATCH 4/8] ARM: dts: sun8i: Add touchscreen node for sun8i-a23-gt90h

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 08/23/2016 05:17 PM, Maxime Ripard wrote:
> On Tue, Aug 23, 2016 at 11:39:06AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 23-08-16 11:26, Maxime Ripard wrote:
>>> On Mon, Aug 22, 2016 at 09:03:57PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 22-08-16 20:30, Maxime Ripard wrote:
>>>>> On Mon, Aug 08, 2016 at 09:43:14PM +0200, Hans de Goede wrote:
>>>>>> The gt90h tablet has a gsl3675 touchscreen, add a dt node describing it.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>> arch/arm/boot/dts/sun8i-a23-gt90h-v4.dts | 8 ++++++++
>>>>>> 1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/sun8i-a23-gt90h-v4.dts b/arch/arm/boot/dts/sun8i-a23-gt90h-v4.dts
>>>>>> index f27ebbb..da55b5a 100644
>>>>>> --- a/arch/arm/boot/dts/sun8i-a23-gt90h-v4.dts
>>>>>> +++ b/arch/arm/boot/dts/sun8i-a23-gt90h-v4.dts
>>>>>> @@ -53,6 +53,14 @@
>>>>>> 	status = "okay";
>>>>>> };
>>>>>>
>>>>>> +&gsl1680 {
>>>>>> +	compatible = "silead,gsl3675";
>>>>>> +	touchscreen-fw-name = "silead/gsl3675-gt90h.fw";
>>>>>
>>>>> That's not documented anywhere, and looks really suspicious.
>>>>
>>>> Ugh, that should have been in:
>>>>
>>>> Documentation/devicetree/bindings/input/touchscreen/silead_gsl1680.txt
>>>>
>>>> But somehow it is not (I believe it was there in earlier revisions of
>>>> the patch), I'll send a patch to fix this.
>>>>
>>>> About it being suspicious, this is not really firmware it is a bunch
>>>> of configuration data / lookup tables for the controller which tell
>>>> it in which order the touchscreen horizontal / vertical sensor
>>>> lines are connected to its sense pins, and what values to send
>>>> for finger x% between line z and line z+1, which differs per
>>>> tablet model, since not all tablets use the same digitizer.
>>>
>>> It's not really the firmware itself that I find suspicious, but more
>>> the encoding of a path to a file in the DT,
>>
>> It is not a path it is a filename. We could drop the "silead/" prefix
>> and put that in the driver instead to really make it a filename.
>>
>>> especially when you can
>>> apparently derive it from other informations already found in the DT
>>> (<vendor>/<compatible>-<board>.fw)
>>
>> That will not work, sometimes different boards use the same digitizer
>> and thus the same firmware. Also in case of the q8 tablets, we need
>> different firmware for different variants (this is to be dealt with
>> by the q8-hardware-manager I'm working on), since although they
>> all use the same digitizer they do not wire it up to the controllers
>> pins the same in all models, so we need different firmware files
>> corresponding to different wirings.
>>
>> TL;DR: There is no 1:1 mapping between board and the firmware filename.
>
> The point still holds. It's exactly the same case than the broadcom
> nvram filename discussion, and it raised the same issues.

No it is not, in this case we also want to be able to specify
different fw names on devices using the same base dts, here
is a tablet for all the q8 tablets with gsl1680 I've:

a13-94v-0:		a082 a23-tj-a23-q88-v4.0_20140815/GSL1680E_700_FW.fw	1024x600 inverted-x
a13-tzx-713b-v2.1:	a082 a23-tj-a23-q88-v4.0_20140815/GSL1680E_700_FW.fw	1024x600
a23-q8h-v3:		a082 a23-tj-a23-q88-v4.0_20140815/GSL1688_A70_FW.fw	 800x480 swapped-x-y
a23-tj-a23-q88-v4.0:	a082 a23-tj-a23-q88-v4.0_20140815/GSL1680E_700_FW.fw	1024x600
a23-tw-ao721-v41:	a082 a23-tj-a23-q88-v4.0_20140815/GSL1680E_700_FW.fw	1024x600 inverted-x
a23-ippo-q8h-v1.2:	a082 a23-tj-a23-q88-v4.0_20140815/GSL1688_A70_FW.fw	 800x480 swapped-x-y
a33-ippo-q8h-v1.2:	a082 a23-tj-a23-q88-v4.0_20140815/GSL1688_A70_FW.fw	 800x480 swapped-x-y
a33-tzx-723q4:		b482 a33-Q8_V2.4_1118/GSL1680_FW_D702.fw		 960x640 inverted-x
a33-q8-v2.4-1118:	b482 a33-Q8_V2.4_1118/GSL1680_FW_D702.fw		 960x640
a33-q8h-v1.5:		b482 a33-q8h-v1.5/GSL1688_A70_FW.fw			 960x640

I'm working on a q8hardware-mgr (inspired by the beagle bone cape mgr)
to automatically detect the touchscreen controller as well as the controller id
(the 2nd column above), and it will need to set a filename for the firmware
file based on this, deriving this from the machine compatible will
simply not work here.

Regards,

Hans