| Message ID | 1470685398-14568-4-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 9, 2016 at 3:43 AM, Hans de Goede <hdegoede@redhat.com> wrote: > The gt90h tablet has a gsl3675 touchscreen, add a dt node describing it. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Chen-Yu Tsai <wens@csie.org> > --- > 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"; The silead binding should be updated with all the other supported chips though. It currently only lists gsl1680. > + touchscreen-fw-name = "silead/gsl3675-gt90h.fw"; > + touchscreen-size-x = <1792>; > + touchscreen-size-y = <1024>; > + status = "okay"; > +}; > + > &lradc { > button@600 { > label = "Back"; > -- > 2.7.4 >
Hi, On 16-08-16 04:47, Chen-Yu Tsai wrote: > On Tue, Aug 9, 2016 at 3:43 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> The gt90h tablet has a gsl3675 touchscreen, add a dt node describing it. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Acked-by: Chen-Yu Tsai <wens@csie.org> > >> --- >> 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"; > > The silead binding should be updated with all the other > supported chips though. It currently only lists gsl1680. Good point. I've just send out a patch adding them. Regards, Hans > >> + touchscreen-fw-name = "silead/gsl3675-gt90h.fw"; >> + touchscreen-size-x = <1792>; >> + touchscreen-size-y = <1024>; >> + status = "okay"; >> +}; >> + >> &lradc { >> button@600 { >> label = "Back"; >> -- >> 2.7.4 >>
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. Rob, is that even allowed? Thanks, Maxime
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. Regards, Hans
23.08.2016, 03:08, "Hans de Goede" <hdegoede@redhat.com>: > 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. I do think it's a mixture of executable MCU codes and data. The GSL1680 datasheet indicates that there's a 32-bit MCU in it. > > Regards, > > Hans > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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, especially when you can apparently derive it from other informations already found in the DT (<vendor>/<compatible>-<board>.fw) Thanks, Maxime
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. Also note that this is not the first driver to introduce a firmware filename in a dts binding, a quick grep shows at least: Documentation/devicetree/bindings/remoteproc/wkup_m3_rproc.txt Also does this, as well as several dt-bindings where board specific firmware gets embedded in the device tree itself. Regards, Hans
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. > Also note that this is not the first driver to introduce a firmware > filename in a dts binding, a quick grep shows at least: > > Documentation/devicetree/bindings/remoteproc/wkup_m3_rproc.txt > > Also does this, as well as several dt-bindings where board specific > firmware gets embedded in the device tree itself. Which has never been acked by any of the DT maintainers. Maxime
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
On Tue, Aug 23, 2016 at 05:59:50PM +0200, Hans de Goede wrote: > 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: At least two other dev told you exactly that in that thread though: http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/439978.html http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/440024.html > > 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. I'm not sure why we need to stick to some insane naming scheme. Or why can't we use symlinks. Maxime
Hi, On 26-08-16 22:46, Maxime Ripard wrote: > On Tue, Aug 23, 2016 at 05:59:50PM +0200, Hans de Goede wrote: >> 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: > > At least two other dev told you exactly that in that thread though: > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/439978.html > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/440024.html That is a different scenario / different mail thread, I agree that the solution proposed there should work for the brcmfmac driver. But that is _not_ what we're discussing here. >> 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. > > I'm not sure why we need to stick to some insane naming scheme. Or why > can't we use symlinks. All the above tablets use the same dts file (sun5i-a13-q8-tablet.dts, sun8i-a23-q8-tablet.dts, sun8i-a33-q8-tablet.dts) while not having the same touchscreen, as discussed in the past I'm working on a kernel module to detect which touchscreen is in use and then automatically update the dt using dynamic-devicetree since creating a separate dt file for each variant is madness (there is a new revision every few weeks). I already have this sort of working and I can detect if a tablet has a gsl1680 touchscreen (*) and if it is the a082 or b482 variant. Now we could encode the chip-id (the a082/b482) in the firmware name at the driver level, but that is still not good enough, e.g. these 2 tablets: 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 Both use a a082 revision gsl1680 but need different firmware files, since they both use sun8i-a23-q8-tablet.dts, so simply embedding the machine compatible string in the filename passed to request_firmware is not going to help. I hope to get permission to get these firmware files added to linux-firmware and then the different files need unique names, and we need a way to specify in devicetree which file to load. We can do some weird indirect dance but that is not really helpful, esp. since I also expect users to need to override which firmware file gets used in certain cases and in that scenario simply specifying the filename certainly is the most userfriendly. TL;DR: we need a way to specify which firmware needs to be loaded and this CANNOT be derived from the machine compatible as devices with the same machine compatible need different firmwares. Regards, Hans *) Not all a13/a23/a33 tablets have a gsl1680 touchscreen, I'm also working on detecting other controllers
On Sat, Aug 27, 2016 at 10:32:16AM +0200, Hans de Goede wrote: > >>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. > > > >I'm not sure why we need to stick to some insane naming scheme. Or why > >can't we use symlinks. > > All the above tablets use the same dts file (sun5i-a13-q8-tablet.dts, > sun8i-a23-q8-tablet.dts, sun8i-a33-q8-tablet.dts) while not having > the same touchscreen, as discussed in the past I'm working on a kernel > module to detect which touchscreen is in use and then automatically > update the dt using dynamic-devicetree since creating a separate dt > file for each variant is madness (there is a new revision every few > weeks). > > I already have this sort of working and I can detect if a tablet has > a gsl1680 touchscreen (*) and if it is the a082 or b482 variant. Now > we could encode the chip-id (the a082/b482) in the firmware name > at the driver level, but that is still not good enough, e.g. these > 2 tablets: > > 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 > > Both use a a082 revision gsl1680 but need different firmware files, > since they both use sun8i-a23-q8-tablet.dts, so simply embedding > the machine compatible string in the filename passed to > request_firmware is not going to help. > > I hope to get permission to get these firmware files added to > linux-firmware and then the different files need unique names, > and we need a way to specify in devicetree which file to load. > > We can do some weird indirect dance but that is not really > helpful, esp. since I also expect users to need to override > which firmware file gets used in certain cases and in that > scenario simply specifying the filename certainly is the most > userfriendly. > > TL;DR: we need a way to specify which firmware needs to > be loaded and this CANNOT be derived from the machine > compatible as devices with the same machine compatible > need different firmwares. You convinced me. Let's see Rob's answer, on your new serie, but since it's what he suggested, that should be ok. I'll merge it as soon as he's ok with it. Maxime
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"; + touchscreen-size-x = <1792>; + touchscreen-size-y = <1024>; + status = "okay"; +}; + &lradc { button@600 { label = "Back";
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(+)