| Message ID | 20190604122308.98D4868B20@newverein.lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add anx6345 DP/eDP bridge for Olimex Teres-I | expand |
On Tue, Jun 4, 2019 at 5:23 AM Torsten Duwe <duwe@lst.de> wrote: > > Teres-I has an anx6345 bridge connected to the RGB666 LCD output, and > the I2C controlling signals are connected to I2C0 bus. eDP output goes > to an Innolux N116BGE panel. > > Enable it in the device tree. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > Signed-off-by: Torsten Duwe <duwe@suse.de> > --- > .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 65 ++++++++++++++++++++-- > 1 file changed, 61 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > index 0ec46b969a75..a0ad438b037f 100644 > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > @@ -65,6 +65,21 @@ > }; > }; > > + panel: panel { > + compatible ="innolux,n116bge", "simple-panel"; It's still "simple-panel". I believe I already mentioned that Rob asked it to be edp-connector. > + status = "okay"; > + power-supply = <®_dcdc1>; > + backlight = <&backlight>; > + > + ports { > + panel_in: port { > + panel_in_edp: endpoint { > + remote-endpoint = <&anx6345_out>; > + }; > + }; > + }; > + }; > + > reg_usb1_vbus: usb1-vbus { > compatible = "regulator-fixed"; > regulator-name = "usb1-vbus"; > @@ -81,20 +96,48 @@ > }; > }; > > +&de { > + status = "okay"; > +}; > + > &ehci1 { > status = "okay"; > }; > > > -/* The ANX6345 eDP-bridge is on i2c0. There is no linux (mainline) > - * driver for this chip at the moment, the bootloader initializes it. > - * However it can be accessed with the i2c-dev driver from user space. > - */ > &i2c0 { > clock-frequency = <100000>; > pinctrl-names = "default"; > pinctrl-0 = <&i2c0_pins>; > status = "okay"; > + > + anx6345: anx6345@38 { > + compatible = "analogix,anx6345"; > + reg = <0x38>; > + reset-gpios = <&pio 3 24 GPIO_ACTIVE_LOW>; /* PD24 */ > + dvdd25-supply = <®_dldo2>; > + dvdd12-supply = <®_dldo3>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + anx6345_in: endpoint { > + remote-endpoint = <&tcon0_out_anx6345>; > + }; > + }; > + port@1 { > + anx6345_out: endpoint { > + remote-endpoint = <&panel_in_edp>; > + }; > + }; > + }; > + }; > +}; > + > +&mixer0 { > + status = "okay"; > }; > > &mmc0 { > @@ -279,6 +322,20 @@ > vcc-hdmi-supply = <®_dldo1>; > }; > > +&tcon0 { > + pinctrl-names = "default"; > + pinctrl-0 = <&lcd_rgb666_pins>; > + > + status = "okay"; > +}; > + > +&tcon0_out { > + tcon0_out_anx6345: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&anx6345_in>; > + }; > +}; > + > &uart0 { > pinctrl-names = "default"; > pinctrl-0 = <&uart0_pb_pins>; > -- > 2.16.4 >
On Tue, Jun 04, 2019 at 08:08:40AM -0700, Vasily Khoruzhick wrote: > On Tue, Jun 4, 2019 at 5:23 AM Torsten Duwe <duwe@lst.de> wrote: > > > > Teres-I has an anx6345 bridge connected to the RGB666 LCD output, and > > the I2C controlling signals are connected to I2C0 bus. eDP output goes > > to an Innolux N116BGE panel. > > > > Enable it in the device tree. > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > Signed-off-by: Torsten Duwe <duwe@suse.de> > > --- > > .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 65 ++++++++++++++++++++-- > > 1 file changed, 61 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > index 0ec46b969a75..a0ad438b037f 100644 > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > @@ -65,6 +65,21 @@ > > }; > > }; > > > > + panel: panel { > > + compatible ="innolux,n116bge", "simple-panel"; > > It's still "simple-panel". I believe I already mentioned that Rob > asked it to be edp-connector. > For which there are neither bindings nor drivers. Is anybody seriously proposing to hold back support for existing (open source!) hardware in favour of an *imaginable* *possibly* better solution? Especially when this exact line is already used in some other places? (there's a space missing btw...) I'm more than glad to follow any constructive improvements towards better modularity. However there were none so far, and on top of that, it's a laptop. I see little advantage in mentioning an internal connector when the panel connected is always the same. FWIW, Rob should also have received these patches. Torsten
On Wed, Jun 05, 2019 at 12:13:17PM +0200, Torsten Duwe wrote: > On Tue, Jun 04, 2019 at 08:08:40AM -0700, Vasily Khoruzhick wrote: > > On Tue, Jun 4, 2019 at 5:23 AM Torsten Duwe <duwe@lst.de> wrote: > > > > > > Teres-I has an anx6345 bridge connected to the RGB666 LCD output, and > > > the I2C controlling signals are connected to I2C0 bus. eDP output goes > > > to an Innolux N116BGE panel. > > > > > > Enable it in the device tree. > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > Signed-off-by: Torsten Duwe <duwe@suse.de> > > > --- > > > .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 65 ++++++++++++++++++++-- > > > 1 file changed, 61 insertions(+), 4 deletions(-) > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > > index 0ec46b969a75..a0ad438b037f 100644 > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > > @@ -65,6 +65,21 @@ > > > }; > > > }; > > > > > > + panel: panel { > > > + compatible ="innolux,n116bge", "simple-panel"; > > > > It's still "simple-panel". I believe I already mentioned that Rob > > asked it to be edp-connector. > > For which there are neither bindings nor drivers. > > Is anybody seriously proposing to hold back support for existing > (open source!) hardware in favour of an *imaginable* *possibly* > better solution? Especially when this exact line is already used in > some other places? (there's a space missing btw...) It's non-existent and imaginable only because you've been ignoring everyone that said that you should do it. So it's self-inflicted, really. And the DT is considered an ABI, so yeah, we will witheld everything that doesn't fit what we would like. Your point of view is that it's more work and for no particular benefit, ours is that it's a short-term pain for a long-term gain, and the benefits will be in the maintainance cost. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Guys, this discussion is getting heated for no reason. Let's put personal frustrations aside and discuss the issue on its merits: Maxime Ripard writes: > On Wed, Jun 05, 2019 at 12:13:17PM +0200, Torsten Duwe wrote: > > On Tue, Jun 04, 2019 at 08:08:40AM -0700, Vasily Khoruzhick wrote: > > > On Tue, Jun 4, 2019 at 5:23 AM Torsten Duwe <duwe@lst.de> wrote: > > > > > > > > Teres-I has an anx6345 bridge connected to the RGB666 LCD output, and > > > > the I2C controlling signals are connected to I2C0 bus. eDP output goes > > > > to an Innolux N116BGE panel. > > > > > > > > Enable it in the device tree. > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > Signed-off-by: Torsten Duwe <duwe@suse.de> > > > > --- > > > > .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 65 ++++++++++++++++++++-- > > > > 1 file changed, 61 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > > > index 0ec46b969a75..a0ad438b037f 100644 > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > > > @@ -65,6 +65,21 @@ > > > > }; > > > > }; > > > > > > > > + panel: panel { > > > > + compatible ="innolux,n116bge", "simple-panel"; > > > > > > It's still "simple-panel". I believe I already mentioned that Rob > > > asked it to be edp-connector. Actually just dropping the "simple-panel" compatible would be a poor choice. Even if "edp-connector" is specified as binding and implemented in a driver, there are older kernel versions and other operating systems to keep in mind. If the HW works with "simple-panel" driver satisfactorily, we should definitely keep the compatible as a fall back for cases where the edp-connector driver is unavailable. If think valid compatible properties would be: compatible = "innolux,n116bge", "simple-panel"; compatible = "edp-connector", "simple-panel"; compatible = "innolux,n116bge", "edp-connector", "simple-panel"; compatible = "edp-connector", "innolux,n116bge", "simple-panel"; I can't make up my mind which one I prefere. However neither of these variants requires actually implmenting an edp-connector driver. And each of these variants is clearly preferable to shipping DTs without description of the panel at all and complies with bindings after adding a stub for "edp-connector". > And the DT is considered an ABI, so yeah, we will witheld everything > that doesn't fit what we would like. I fail to see how the patch in discussion adds new ABI. While I understand the need to pester contributors for more work, outright blocking DTs, that properly describe the HW and comply with existing bindings, seems a bit unreasonable. (Assuming there are no other issues of course.) Also note that the innolux,n116bge binding suggestes using simple-panel as fallback. HTH, Harald
On Thu, Jun 06, 2019 at 03:59:27PM +0200, Harald Geyer wrote: > Guys, this discussion is getting heated for no reason. Let's put > personal frustrations aside and discuss the issue on its merits: > > Maxime Ripard writes: > > On Wed, Jun 05, 2019 at 12:13:17PM +0200, Torsten Duwe wrote: > > > On Tue, Jun 04, 2019 at 08:08:40AM -0700, Vasily Khoruzhick wrote: > > > > On Tue, Jun 4, 2019 at 5:23 AM Torsten Duwe <duwe@lst.de> wrote: > > > > > > > > > > Teres-I has an anx6345 bridge connected to the RGB666 LCD output, and > > > > > the I2C controlling signals are connected to I2C0 bus. eDP output goes > > > > > to an Innolux N116BGE panel. > > > > > > > > > > Enable it in the device tree. > > > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > > Signed-off-by: Torsten Duwe <duwe@suse.de> > > > > > --- > > > > > .../boot/dts/allwinner/sun50i-a64-teres-i.dts | 65 ++++++++++++++++++++-- > > > > > 1 file changed, 61 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > > > > index 0ec46b969a75..a0ad438b037f 100644 > > > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts > > > > > @@ -65,6 +65,21 @@ > > > > > }; > > > > > }; > > > > > > > > > > + panel: panel { > > > > > + compatible ="innolux,n116bge", "simple-panel"; > > > > > > > > It's still "simple-panel". I believe I already mentioned that Rob > > > > asked it to be edp-connector. > > Actually just dropping the "simple-panel" compatible would be a poor > choice. Even if "edp-connector" is specified as binding and implemented in a > driver, there are older kernel versions and other operating systems to > keep in mind. Which older kernels? This is a new binding, adding a new driver, so if an older kernel uses a separate driver with its own binding, good for them, but we don't have to support it. > If the HW works with "simple-panel" driver satisfactorily, > we should definitely keep the compatible as a fall back for cases where > the edp-connector driver is unavailable. > > If think valid compatible properties would be: > compatible = "innolux,n116bge", "simple-panel"; > compatible = "edp-connector", "simple-panel"; A connector isn't a panel. > compatible = "innolux,n116bge", "edp-connector", "simple-panel"; And the innolux,n116bge is certainly not a connector either. > compatible = "edp-connector", "innolux,n116bge", "simple-panel"; > > I can't make up my mind which one I prefere. However neither of these > variants requires actually implmenting an edp-connector driver. No-one asked to do an edp-connector driver. You should use it in your DT, but if you want to have some code in your driver that parses the DT directly, I'm totally fine with that. > And each of these variants is clearly preferable to shipping DTs > without description of the panel at all and complies with bindings > after adding a stub for "edp-connector". I guess you should describe why do you think it's "clear", because I'm not sure this is obvious for everyone here. eDP allows to discover which device is on the other side and its supported timings, just like HDMI for example (or regular DP, for that matter). Would you think it's clearly preferable to ship a DT with the DP/HDMI monitor connected on the other side exposed as a panel as well? > > And the DT is considered an ABI, so yeah, we will witheld everything > > that doesn't fit what we would like. > > I fail to see how the patch in discussion adds new ABI. The binding itself is the ABI, and we will have to support that binding for pretty much forever. > While I understand the need to pester contributors for more work, > outright blocking DTs, that properly describe the HW Properly is arguable. > and comply with existing bindings And that's bindings meant for another use-case. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, Jun 07, 2019 at 08:28:02AM +0200, Maxime Ripard wrote: > On Thu, Jun 06, 2019 at 03:59:27PM +0200, Harald Geyer wrote: > > > > If think valid compatible properties would be: > > compatible = "innolux,n116bge", "simple-panel"; > > compatible = "edp-connector", "simple-panel"; > > A connector isn't a panel. > > > compatible = "innolux,n116bge", "edp-connector", "simple-panel"; > > And the innolux,n116bge is certainly not a connector either. > > > compatible = "edp-connector", "innolux,n116bge", "simple-panel"; > > > > I can't make up my mind which one I prefere. However neither of these > > variants requires actually implmenting an edp-connector driver. > > No-one asked to do an edp-connector driver. You should use it in your > DT, but if you want to have some code in your driver that parses the > DT directly, I'm totally fine with that. I must admit I fail to understand what that extra node would be good for. Logically, the eDP far side is connected to the well-known n116bge. Inside the laptop case it might as well be a flat ribbon cable or soldered directly. In good intention, that's all I wanted to express in the DT. I don't know whether the relevant mechanical dimensions of the panel and the connector are standardised, so whether one could in theory assemble it with a different panel than the one it came with. OTOH, as I checked during the discussion with anarsoul, the panel's supply voltage is permanently connected to the main 3.3V rail. We already agreed that the eDP output port must not neccessarily be specified, this setup is a good example why: because the panel is always powered, the anx6345 can always pull valid EDID data from it so at this stage there's no need for any OS driver to reach beyond the bridge. IIRC even the backlight got switched off for the blank screen without. All I wanted to say is that "there's usually an n116bge behind it"; but this is mostly redundant. So, shall we just drop the output port specification (along with the panel node) in order to get one step further? > I guess you should describe why do you think it's "clear", because I'm > not sure this is obvious for everyone here. eDP allows to discover > which device is on the other side and its supported timings, just like > HDMI for example (or regular DP, for that matter). Would you think > it's clearly preferable to ship a DT with the DP/HDMI monitor > connected on the other side exposed as a panel as well? Well, as I wrote: "in good intention". That's the panel that comes with the kit but it is very well detected automatically, just like you describe. So, just leave it out? Torsten
On 07.06.2019 11:40, Torsten Duwe wrote: > On Fri, Jun 07, 2019 at 08:28:02AM +0200, Maxime Ripard wrote: >> On Thu, Jun 06, 2019 at 03:59:27PM +0200, Harald Geyer wrote: >>> If think valid compatible properties would be: >>> compatible = "innolux,n116bge", "simple-panel"; >>> compatible = "edp-connector", "simple-panel"; >> A connector isn't a panel. >> >>> compatible = "innolux,n116bge", "edp-connector", "simple-panel"; >> And the innolux,n116bge is certainly not a connector either. >> >>> compatible = "edp-connector", "innolux,n116bge", "simple-panel"; >>> >>> I can't make up my mind which one I prefere. However neither of these >>> variants requires actually implmenting an edp-connector driver. >> No-one asked to do an edp-connector driver. You should use it in your >> DT, but if you want to have some code in your driver that parses the >> DT directly, I'm totally fine with that. > I must admit I fail to understand what that extra node would be good for. > Logically, the eDP far side is connected to the well-known n116bge. > Inside the laptop case it might as well be a flat ribbon cable or > soldered directly. > In good intention, that's all I wanted to express in the DT. I don't > know whether the relevant mechanical dimensions of the panel and the > connector are standardised, so whether one could in theory assemble it > with a different panel than the one it came with. > > OTOH, as I checked during the discussion with anarsoul, the panel's > supply voltage is permanently connected to the main 3.3V rail. > We already agreed that the eDP output port must not neccessarily be > specified, this setup is a good example why: because the panel is > always powered, the anx6345 can always pull valid EDID data from it > so at this stage there's no need for any OS driver to reach beyond > the bridge. IIRC even the backlight got switched off for the blank > screen without. > > All I wanted to say is that "there's usually an n116bge behind it"; > but this is mostly redundant. > > So, shall we just drop the output port specification (along with the > panel node) in order to get one step further? I am not sure if I understand whole discussion here, but I also do not understand whole edp-connector thing. According to VESA[1] eDP is "Internal display interface" - there is no external connector for eDP, the way it is connected is integrator's decision, but it is fixed - ie end user do not plug/unplug it. If I remember correctly in some boards eDP is connected to some DP connector (odroid xu3 if I remember correctly), but this is non-standard hack, and for this case in bindings there should be rather dp-connector not edp-connector. [1]: https://www.vesa.org/wp-content/uploads/2010/12/DisplayPort-DevCon-Presentation-eDP-Dec-2010-v3.pdf Regards Andrzej > >> I guess you should describe why do you think it's "clear", because I'm >> not sure this is obvious for everyone here. eDP allows to discover >> which device is on the other side and its supported timings, just like >> HDMI for example (or regular DP, for that matter). Would you think >> it's clearly preferable to ship a DT with the DP/HDMI monitor >> connected on the other side exposed as a panel as well? > Well, as I wrote: "in good intention". That's the panel that comes with > the kit but it is very well detected automatically, just like you describe. > > So, just leave it out? > > Torsten > > >
Hi, On Wed, Jun 12, 2019 at 12:00:21PM +0200, Andrzej Hajda wrote: > On 07.06.2019 11:40, Torsten Duwe wrote: > > On Fri, Jun 07, 2019 at 08:28:02AM +0200, Maxime Ripard wrote: > >> On Thu, Jun 06, 2019 at 03:59:27PM +0200, Harald Geyer wrote: > >>> If think valid compatible properties would be: > >>> compatible = "innolux,n116bge", "simple-panel"; > >>> compatible = "edp-connector", "simple-panel"; > >> A connector isn't a panel. > >> > >>> compatible = "innolux,n116bge", "edp-connector", "simple-panel"; > >> And the innolux,n116bge is certainly not a connector either. > >> > >>> compatible = "edp-connector", "innolux,n116bge", "simple-panel"; > >>> > >>> I can't make up my mind which one I prefere. However neither of these > >>> variants requires actually implmenting an edp-connector driver. > >> No-one asked to do an edp-connector driver. You should use it in your > >> DT, but if you want to have some code in your driver that parses the > >> DT directly, I'm totally fine with that. > > I must admit I fail to understand what that extra node would be good for. > > Logically, the eDP far side is connected to the well-known n116bge. > > Inside the laptop case it might as well be a flat ribbon cable or > > soldered directly. > > In good intention, that's all I wanted to express in the DT. I don't > > know whether the relevant mechanical dimensions of the panel and the > > connector are standardised, so whether one could in theory assemble it > > with a different panel than the one it came with. > > > > OTOH, as I checked during the discussion with anarsoul, the panel's > > supply voltage is permanently connected to the main 3.3V rail. > > We already agreed that the eDP output port must not neccessarily be > > specified, this setup is a good example why: because the panel is > > always powered, the anx6345 can always pull valid EDID data from it > > so at this stage there's no need for any OS driver to reach beyond > > the bridge. IIRC even the backlight got switched off for the blank > > screen without. > > > > All I wanted to say is that "there's usually an n116bge behind it"; > > but this is mostly redundant. > > > > So, shall we just drop the output port specification (along with the > > panel node) in order to get one step further? > > I am not sure if I understand whole discussion here, but I also do not > understand whole edp-connector thing. The context is this one: https://patchwork.freedesktop.org/patch/257352/?series=51182&rev=1 https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1 https://patchwork.freedesktop.org/patch/286468/?series=56776&rev=2 TL;DR: This bridge is being used on ARM laptops that can come with different eDP panels. Some of these panels require a regulator to be enabled for the panel to work, and this is obviously something that should be in the DT. However, we can't really describe the panel itself, since the vendor uses several of them and just relies on the eDP bus to do its job at retrieving the EDIDs. A generic panel isn't really working either since that would mean having a generic behaviour for all the panels connected to that bus, which isn't there either. The connector allows to expose this nicely. > According to VESA[1] eDP is "Internal display interface" - there is no > external connector for eDP, the way it is connected is integrator's > decision, but it is fixed - ie end user do not plug/unplug it. I'm not sure if you mean DRM or DT connector here though. In DRM, we're doing this all the time for panels. I'm literaly typing this from a laptop that has a screen with an eDP connector. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, Jun 07, 2019 at 11:40:30AM +0200, Torsten Duwe wrote: > On Fri, Jun 07, 2019 at 08:28:02AM +0200, Maxime Ripard wrote: > > On Thu, Jun 06, 2019 at 03:59:27PM +0200, Harald Geyer wrote: > > > > > > If think valid compatible properties would be: > > > compatible = "innolux,n116bge", "simple-panel"; > > > compatible = "edp-connector", "simple-panel"; > > > > A connector isn't a panel. > > > > > compatible = "innolux,n116bge", "edp-connector", "simple-panel"; > > > > And the innolux,n116bge is certainly not a connector either. > > > > > compatible = "edp-connector", "innolux,n116bge", "simple-panel"; > > > > > > I can't make up my mind which one I prefere. However neither of these > > > variants requires actually implmenting an edp-connector driver. > > > > No-one asked to do an edp-connector driver. You should use it in your > > DT, but if you want to have some code in your driver that parses the > > DT directly, I'm totally fine with that. > > I must admit I fail to understand what that extra node would be good for. > Logically, the eDP far side is connected to the well-known n116bge. > Inside the laptop case it might as well be a flat ribbon cable or > soldered directly. > In good intention, that's all I wanted to express in the DT. I don't > know whether the relevant mechanical dimensions of the panel and the > connector are standardised, so whether one could in theory assemble it > with a different panel than the one it came with. Because the panel that comes with the Teres-I is always the same. However, that's not true for all the devices out there using the bridge, starting with the pinebook. > OTOH, as I checked during the discussion with anarsoul, the panel's > supply voltage is permanently connected to the main 3.3V rail. Again, that may be the case on the Teres-I, but not necessarily on other boards. > We already agreed that the eDP output port must not neccessarily be > specified, this setup is a good example why: because the panel is > always powered, the anx6345 can always pull valid EDID data from it > so at this stage there's no need for any OS driver to reach beyond > the bridge. IIRC even the backlight got switched off for the blank > screen without. That's not really the outcome of the discussion we had here though: https://patchwork.freedesktop.org/patch/305035/ > All I wanted to say is that "there's usually an n116bge behind it"; > but this is mostly redundant. > > So, shall we just drop the output port specification (along with the > panel node) in order to get one step further? Depending on the outcome of the discussion above, yes or no :) Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Maxime, It seems I have missed your response. On 12.06.2019 17:20, Maxime Ripard wrote: >> I am not sure if I understand whole discussion here, but I also do not >> understand whole edp-connector thing. > The context is this one: > https://patchwork.freedesktop.org/patch/257352/?series=51182&rev=1 > https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1 > https://patchwork.freedesktop.org/patch/286468/?series=56776&rev=2 > > TL;DR: This bridge is being used on ARM laptops that can come with > different eDP panels. Some of these panels require a regulator to be > enabled for the panel to work, and this is obviously something that > should be in the DT. > > However, we can't really describe the panel itself, since the vendor > uses several of them and just relies on the eDP bus to do its job at > retrieving the EDIDs. A generic panel isn't really working either > since that would mean having a generic behaviour for all the panels > connected to that bus, which isn't there either. > > The connector allows to expose this nicely. As VESA presentation says[1] eDP is based on DP but is much more flexible, it is up to integrator (!!!) how the connection, power up/down, initialization sequence should be performed. Trying to cover every such case in edp-connector seems to me similar to panel-simple attempt failure. Moreover there is no such thing as physical standard eDP connector. Till now I though DT connector should describe physical connector on the device, now I am lost, are there some DT bindings guidelines about definition of a connector? Maybe instead of edp-connector one would introduce integrator's specific connector, for example with compatible "olimex,teres-edp-connector" which should follow edp abstract connector rules? This will be at least consistent with below presentation[1] - eDP requirements depends on integrator. Then if olimex has standard way of dealing with panels present in olimex/teres platforms the driver would then create drm_panel/drm_connector/drm_bridge(?) according to these rules, I guess. Anyway it still looks fishy for me :), maybe because I am not familiarized with details of these platforms. [1]: https://www.vesa.org/wp-content/uploads/2010/12/DisplayPort-DevCon-Presentation-eDP-Dec-2010-v3.pdf > >> According to VESA[1] eDP is "Internal display interface" - there is no >> external connector for eDP, the way it is connected is integrator's >> decision, but it is fixed - ie end user do not plug/unplug it. > I'm not sure if you mean DRM or DT connector here though. In DRM, > we're doing this all the time for panels. I'm literaly typing this > from a laptop that has a screen with an eDP connector. VESA describes only hardware, but since DT also describes hardware I guess it should be similar. Regards Andrzej > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hi! On Fri, Jun 28, 2019 at 12:39:32PM +0200, Andrzej Hajda wrote: > On 12.06.2019 17:20, Maxime Ripard wrote: > >> I am not sure if I understand whole discussion here, but I also do not > >> understand whole edp-connector thing. > > The context is this one: > > https://patchwork.freedesktop.org/patch/257352/?series=51182&rev=1 > > https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1 > > https://patchwork.freedesktop.org/patch/286468/?series=56776&rev=2 > > > > TL;DR: This bridge is being used on ARM laptops that can come with > > different eDP panels. Some of these panels require a regulator to be > > enabled for the panel to work, and this is obviously something that > > should be in the DT. > > > > However, we can't really describe the panel itself, since the vendor > > uses several of them and just relies on the eDP bus to do its job at > > retrieving the EDIDs. A generic panel isn't really working either > > since that would mean having a generic behaviour for all the panels > > connected to that bus, which isn't there either. > > > > The connector allows to expose this nicely. > > As VESA presentation says[1] eDP is based on DP but is much more > flexible, it is up to integrator (!!!) how the connection, power > up/down, initialization sequence should be performed. Trying to cover > every such case in edp-connector seems to me similar to panel-simple > attempt failure. Moreover there is no such thing as physical standard > eDP connector. Till now I though DT connector should describe physical > connector on the device, now I am lost, are there some DT bindings > guidelines about definition of a connector? This might be semantics but I guess we're in some kind of grey area? Like, for eDP, if it's soldered I guess we could say that there's no connector. But what happens if for some other board, that signal is routed through a ribbon? You could argue that there's no physical connector in both cases, or that there's one in both, or one for the ribbon and no connector for the one soldered in. > Maybe instead of edp-connector one would introduce integrator's specific > connector, for example with compatible "olimex,teres-edp-connector" > which should follow edp abstract connector rules? This will be at least > consistent with below presentation[1] - eDP requirements depends on > integrator. Then if olimex has standard way of dealing with panels > present in olimex/teres platforms the driver would then create > drm_panel/drm_connector/drm_bridge(?) according to these rules, I guess. > Anyway it still looks fishy for me :), maybe because I am not > familiarized with details of these platforms. That makes sense yes Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 01.07.2019 11:58, Maxime Ripard wrote: > Hi! > > On Fri, Jun 28, 2019 at 12:39:32PM +0200, Andrzej Hajda wrote: >> On 12.06.2019 17:20, Maxime Ripard wrote: >>>> I am not sure if I understand whole discussion here, but I also do not >>>> understand whole edp-connector thing. >>> The context is this one: >>> https://patchwork.freedesktop.org/patch/257352/?series=51182&rev=1 >>> https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1 >>> https://patchwork.freedesktop.org/patch/286468/?series=56776&rev=2 >>> >>> TL;DR: This bridge is being used on ARM laptops that can come with >>> different eDP panels. Some of these panels require a regulator to be >>> enabled for the panel to work, and this is obviously something that >>> should be in the DT. >>> >>> However, we can't really describe the panel itself, since the vendor >>> uses several of them and just relies on the eDP bus to do its job at >>> retrieving the EDIDs. A generic panel isn't really working either >>> since that would mean having a generic behaviour for all the panels >>> connected to that bus, which isn't there either. >>> >>> The connector allows to expose this nicely. >> As VESA presentation says[1] eDP is based on DP but is much more >> flexible, it is up to integrator (!!!) how the connection, power >> up/down, initialization sequence should be performed. Trying to cover >> every such case in edp-connector seems to me similar to panel-simple >> attempt failure. Moreover there is no such thing as physical standard >> eDP connector. Till now I though DT connector should describe physical >> connector on the device, now I am lost, are there some DT bindings >> guidelines about definition of a connector? > This might be semantics but I guess we're in some kind of grey area? > > Like, for eDP, if it's soldered I guess we could say that there's no > connector. But what happens if for some other board, that signal is > routed through a ribbon? > > You could argue that there's no physical connector in both cases, or > that there's one in both, or one for the ribbon and no connector for > the one soldered in. This is not about ribbon vs soldering. It is about usage: this connection is static across the whole life of the device (except exceptional things: repair, non-standard usage, etc). And "the real connector" is (at least for me) something where end-user can connect/disconnect different things: USB, HDMI, ethernet, etc. And obviously to be functional it should be somehow standardized. So even if there could be some grey area, I do not see it here. > >> Maybe instead of edp-connector one would introduce integrator's specific >> connector, for example with compatible "olimex,teres-edp-connector" >> which should follow edp abstract connector rules? This will be at least >> consistent with below presentation[1] - eDP requirements depends on >> integrator. Then if olimex has standard way of dealing with panels >> present in olimex/teres platforms the driver would then create >> drm_panel/drm_connector/drm_bridge(?) according to these rules, I guess. >> Anyway it still looks fishy for me :), maybe because I am not >> familiarized with details of these platforms. > That makes sense yes And what if some panel can be used with this pseudo-connecter and in some different hw directly? Code duplication? DT overlays? Regards Andrzej > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Mon, Jul 01, 2019 at 02:27:51PM +0200, Andrzej Hajda wrote: > On 01.07.2019 11:58, Maxime Ripard wrote: > > On Fri, Jun 28, 2019 at 12:39:32PM +0200, Andrzej Hajda wrote: > >> On 12.06.2019 17:20, Maxime Ripard wrote: > >>>> I am not sure if I understand whole discussion here, but I also do not > >>>> understand whole edp-connector thing. > >>> The context is this one: > >>> https://patchwork.freedesktop.org/patch/257352/?series=51182&rev=1 > >>> https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1 > >>> https://patchwork.freedesktop.org/patch/286468/?series=56776&rev=2 > >>> > >>> TL;DR: This bridge is being used on ARM laptops that can come with > >>> different eDP panels. Some of these panels require a regulator to be > >>> enabled for the panel to work, and this is obviously something that > >>> should be in the DT. > >>> > >>> However, we can't really describe the panel itself, since the vendor > >>> uses several of them and just relies on the eDP bus to do its job at > >>> retrieving the EDIDs. A generic panel isn't really working either > >>> since that would mean having a generic behaviour for all the panels > >>> connected to that bus, which isn't there either. > >>> > >>> The connector allows to expose this nicely. > >> As VESA presentation says[1] eDP is based on DP but is much more > >> flexible, it is up to integrator (!!!) how the connection, power > >> up/down, initialization sequence should be performed. Trying to cover > >> every such case in edp-connector seems to me similar to panel-simple > >> attempt failure. Moreover there is no such thing as physical standard > >> eDP connector. Till now I though DT connector should describe physical > >> connector on the device, now I am lost, are there some DT bindings > >> guidelines about definition of a connector? > > This might be semantics but I guess we're in some kind of grey area? > > > > Like, for eDP, if it's soldered I guess we could say that there's no > > connector. But what happens if for some other board, that signal is > > routed through a ribbon? > > > > You could argue that there's no physical connector in both cases, or > > that there's one in both, or one for the ribbon and no connector for > > the one soldered in. > > This is not about ribbon vs soldering. It is about usage: this > connection is static across the whole life of the device (except > exceptional things: repair, non-standard usage, etc). It doesn't have to be. > And "the real connector" is (at least for me) something where > end-user can connect/disconnect different things: USB, HDMI, > ethernet, etc. And obviously to be functional it should be somehow > standardized. So even if there could be some grey area, I do not see > it here. Well, if there's a ribbon connector, then you have a physical connector, with the end user being able to connect / disconnect various displays. It might not be the case with actual products, but it's pretty common with SBCs to have that signal routed through a connector, and the user has several options to connect a display to it. The line really is blurred. > >> Maybe instead of edp-connector one would introduce integrator's specific > >> connector, for example with compatible "olimex,teres-edp-connector" > >> which should follow edp abstract connector rules? This will be at least > >> consistent with below presentation[1] - eDP requirements depends on > >> integrator. Then if olimex has standard way of dealing with panels > >> present in olimex/teres platforms the driver would then create > >> drm_panel/drm_connector/drm_bridge(?) according to these rules, I guess. > >> Anyway it still looks fishy for me :), maybe because I am not > >> familiarized with details of these platforms. > > > That makes sense yes > > And what if some panel can be used with this pseudo-connecter and in > some different hw directly? Code duplication? DT overlays? Overlays are a solution, but I would advocate to always have the connector. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Mon, Jul 1, 2019 at 2:58 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > Hi! > > On Fri, Jun 28, 2019 at 12:39:32PM +0200, Andrzej Hajda wrote: > > On 12.06.2019 17:20, Maxime Ripard wrote: > > >> I am not sure if I understand whole discussion here, but I also do not > > >> understand whole edp-connector thing. > > > The context is this one: > > > https://patchwork.freedesktop.org/patch/257352/?series=51182&rev=1 > > > https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1 > > > https://patchwork.freedesktop.org/patch/286468/?series=56776&rev=2 > > > > > > TL;DR: This bridge is being used on ARM laptops that can come with > > > different eDP panels. Some of these panels require a regulator to be > > > enabled for the panel to work, and this is obviously something that > > > should be in the DT. > > > > > > However, we can't really describe the panel itself, since the vendor > > > uses several of them and just relies on the eDP bus to do its job at > > > retrieving the EDIDs. A generic panel isn't really working either > > > since that would mean having a generic behaviour for all the panels > > > connected to that bus, which isn't there either. > > > > > > The connector allows to expose this nicely. > > > > As VESA presentation says[1] eDP is based on DP but is much more > > flexible, it is up to integrator (!!!) how the connection, power > > up/down, initialization sequence should be performed. Trying to cover > > every such case in edp-connector seems to me similar to panel-simple > > attempt failure. Moreover there is no such thing as physical standard > > eDP connector. Till now I though DT connector should describe physical > > connector on the device, now I am lost, are there some DT bindings > > guidelines about definition of a connector? > > This might be semantics but I guess we're in some kind of grey area? > > Like, for eDP, if it's soldered I guess we could say that there's no > connector. But what happens if for some other board, that signal is > routed through a ribbon? > > You could argue that there's no physical connector in both cases, or > that there's one in both, or one for the ribbon and no connector for > the one soldered in. > > > Maybe instead of edp-connector one would introduce integrator's specific > > connector, for example with compatible "olimex,teres-edp-connector" > > which should follow edp abstract connector rules? This will be at least > > consistent with below presentation[1] - eDP requirements depends on > > integrator. Then if olimex has standard way of dealing with panels > > present in olimex/teres platforms the driver would then create > > drm_panel/drm_connector/drm_bridge(?) according to these rules, I guess. > > Anyway it still looks fishy for me :), maybe because I am not > > familiarized with details of these platforms. > > That makes sense yes Actually, it makes no sense at all. Current implementation for anx6345 driver works fine as is with any panel specified assuming panel delays are long enough for connected panel. It just doesn't use panel timings from the driver. Creating a platform driver for connector itself looks redundant since it can't be reused, it doesn't describe actual hardware and it's just defeats purpose of DT by introducing board-specific code. There's another issue: if we introduce edp-connector we'll have to specify power up delays somewhere (in dts? or in platform driver?), so edp-connector doesn't really solve the issue of multiple panels with same motherboard. I'd say DT overlays should be preferred solution here, not another connector binding. > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Mon, Jul 08, 2019 at 05:49:21PM -0700, Vasily Khoruzhick wrote: > > > Maybe instead of edp-connector one would introduce integrator's specific > > > connector, for example with compatible "olimex,teres-edp-connector" > > > which should follow edp abstract connector rules? This will be at least > > > consistent with below presentation[1] - eDP requirements depends on > > > integrator. Then if olimex has standard way of dealing with panels > > > present in olimex/teres platforms the driver would then create > > > drm_panel/drm_connector/drm_bridge(?) according to these rules, I guess. > > > Anyway it still looks fishy for me :), maybe because I am not > > > familiarized with details of these platforms. > > > > That makes sense yes > > Actually, it makes no sense at all. Current implementation for anx6345 > driver works fine as is with any panel specified assuming panel delays > are long enough for connected panel. It just doesn't use panel timings > from the driver. Creating a platform driver for connector itself looks > redundant since it can't be reused, it doesn't describe actual > hardware and it's just defeats purpose of DT by introducing > board-specific code. I'm not sure where you got the idea that the purpose of DT is to not have any board-specific code. It's perfectly fine to have some, that's even why there's a compatible assigned to each and every board. What the DT is about is allowing us to have a generic behaviour that we can detect: we can have a given behaviour for a given board, and a separate one for another one, and this will be evaluated at runtime. This is *exactly* what this is about: we can have a compatible that sets a given, more specific, behaviour (olimex,teres-edp-connector) while saying that this is compatible with the generic behaviour (edp-connector). That way, any OS will know what quirk to apply if needed, and if not that it can use the generic behaviour. And we could create a generic driver, for the generic behaviour if needed. > There's another issue: if we introduce edp-connector we'll have to > specify power up delays somewhere (in dts? or in platform driver?), so > edp-connector doesn't really solve the issue of multiple panels with > same motherboard. And that's what that compatible is about :) > I'd say DT overlays should be preferred solution here, not another > connector binding. Overlays are a way to apply a device tree dynamically. It's orthogonal to the binding. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
于 2019年7月9日 GMT+08:00 下午4:55:32, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >On Mon, Jul 08, 2019 at 05:49:21PM -0700, Vasily Khoruzhick wrote: >> > > Maybe instead of edp-connector one would introduce integrator's >specific >> > > connector, for example with compatible >"olimex,teres-edp-connector" >> > > which should follow edp abstract connector rules? This will be at >least >> > > consistent with below presentation[1] - eDP requirements depends >on >> > > integrator. Then if olimex has standard way of dealing with >panels >> > > present in olimex/teres platforms the driver would then create >> > > drm_panel/drm_connector/drm_bridge(?) according to these rules, I >guess. >> > > Anyway it still looks fishy for me :), maybe because I am not >> > > familiarized with details of these platforms. >> > >> > That makes sense yes >> >> Actually, it makes no sense at all. Current implementation for >anx6345 >> driver works fine as is with any panel specified assuming panel >delays >> are long enough for connected panel. It just doesn't use panel >timings >> from the driver. Creating a platform driver for connector itself >looks >> redundant since it can't be reused, it doesn't describe actual >> hardware and it's just defeats purpose of DT by introducing >> board-specific code. > >I'm not sure where you got the idea that the purpose of DT is to not >have any board-specific code. > >It's perfectly fine to have some, that's even why there's a compatible >assigned to each and every board. > >What the DT is about is allowing us to have a generic behaviour that >we can detect: we can have a given behaviour for a given board, and a >separate one for another one, and this will be evaluated at runtime. > >This is *exactly* what this is about: we can have a compatible that >sets a given, more specific, behaviour (olimex,teres-edp-connector) >while saying that this is compatible with the generic behaviour >(edp-connector). That way, any OS will know what quirk to apply if >needed, and if not that it can use the generic behaviour. > >And we could create a generic driver, for the generic behaviour if >needed. > >> There's another issue: if we introduce edp-connector we'll have to >> specify power up delays somewhere (in dts? or in platform driver?), >so >> edp-connector doesn't really solve the issue of multiple panels with >> same motherboard. > >And that's what that compatible is about :) Maybe we can introduce a connector w/o any driver just like hdmi-connector? > >> I'd say DT overlays should be preferred solution here, not another >> connector binding. > >Overlays are a way to apply a device tree dynamically. It's orthogonal >to the binding. > >Maxime > >-- >Maxime Ripard, Bootlin >Embedded Linux and Kernel engineering >https://bootlin.com
On Tue, Jul 09, 2019 at 04:58:35PM +0800, Icenowy Zheng wrote: > > > 于 2019年7月9日 GMT+08:00 下午4:55:32, Maxime Ripard <maxime.ripard@bootlin.com> 写到: > >On Mon, Jul 08, 2019 at 05:49:21PM -0700, Vasily Khoruzhick wrote: > >> > > Maybe instead of edp-connector one would introduce integrator's > >specific > >> > > connector, for example with compatible > >"olimex,teres-edp-connector" > >> > > which should follow edp abstract connector rules? This will be at > >least > >> > > consistent with below presentation[1] - eDP requirements depends > >on > >> > > integrator. Then if olimex has standard way of dealing with > >panels > >> > > present in olimex/teres platforms the driver would then create > >> > > drm_panel/drm_connector/drm_bridge(?) according to these rules, I > >guess. > >> > > Anyway it still looks fishy for me :), maybe because I am not > >> > > familiarized with details of these platforms. > >> > > >> > That makes sense yes > >> > >> Actually, it makes no sense at all. Current implementation for > >anx6345 > >> driver works fine as is with any panel specified assuming panel > >delays > >> are long enough for connected panel. It just doesn't use panel > >timings > >> from the driver. Creating a platform driver for connector itself > >looks > >> redundant since it can't be reused, it doesn't describe actual > >> hardware and it's just defeats purpose of DT by introducing > >> board-specific code. > > > >I'm not sure where you got the idea that the purpose of DT is to not > >have any board-specific code. > > > >It's perfectly fine to have some, that's even why there's a compatible > >assigned to each and every board. > > > >What the DT is about is allowing us to have a generic behaviour that > >we can detect: we can have a given behaviour for a given board, and a > >separate one for another one, and this will be evaluated at runtime. > > > >This is *exactly* what this is about: we can have a compatible that > >sets a given, more specific, behaviour (olimex,teres-edp-connector) > >while saying that this is compatible with the generic behaviour > >(edp-connector). That way, any OS will know what quirk to apply if > >needed, and if not that it can use the generic behaviour. > > > >And we could create a generic driver, for the generic behaviour if > >needed. > > > >> There's another issue: if we introduce edp-connector we'll have to > >> specify power up delays somewhere (in dts? or in platform driver?), > >so > >> edp-connector doesn't really solve the issue of multiple panels with > >> same motherboard. > > > >And that's what that compatible is about :) > > Maybe we can introduce a connector w/o any driver just like hdmi-connector? Ironically, a driver for it has been sent yesterday :) But yeah, we can definitely do that too. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, Jul 9, 2019 at 1:55 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Mon, Jul 08, 2019 at 05:49:21PM -0700, Vasily Khoruzhick wrote: > > > > Maybe instead of edp-connector one would introduce integrator's specific > > > > connector, for example with compatible "olimex,teres-edp-connector" > > > > which should follow edp abstract connector rules? This will be at least > > > > consistent with below presentation[1] - eDP requirements depends on > > > > integrator. Then if olimex has standard way of dealing with panels > > > > present in olimex/teres platforms the driver would then create > > > > drm_panel/drm_connector/drm_bridge(?) according to these rules, I guess. > > > > Anyway it still looks fishy for me :), maybe because I am not > > > > familiarized with details of these platforms. > > > > > > That makes sense yes > > > > Actually, it makes no sense at all. Current implementation for anx6345 > > driver works fine as is with any panel specified assuming panel delays > > are long enough for connected panel. It just doesn't use panel timings > > from the driver. Creating a platform driver for connector itself looks > > redundant since it can't be reused, it doesn't describe actual > > hardware and it's just defeats purpose of DT by introducing > > board-specific code. > > I'm not sure where you got the idea that the purpose of DT is to not > have any board-specific code. I believe DT was an attempt to move to declarative approach for describing hardware. Yes, we have different compatibles for different devices but they're specific to particular device rather than particular board. Device interconnection is described in DT along with some properties rather than in board-specific C-file. Introducing board-specific compatible for a connector isn't looking right to me. > It's perfectly fine to have some, that's even why there's a compatible > assigned to each and every board. > > What the DT is about is allowing us to have a generic behaviour that > we can detect: we can have a given behaviour for a given board, and a > separate one for another one, and this will be evaluated at runtime. > > This is *exactly* what this is about: we can have a compatible that > sets a given, more specific, behaviour (olimex,teres-edp-connector) > while saying that this is compatible with the generic behaviour > (edp-connector). That way, any OS will know what quirk to apply if > needed, and if not that it can use the generic behaviour. > > And we could create a generic driver, for the generic behaviour if > needed. > > > There's another issue: if we introduce edp-connector we'll have to > > specify power up delays somewhere (in dts? or in platform driver?), so > > edp-connector doesn't really solve the issue of multiple panels with > > same motherboard. > > And that's what that compatible is about :) Sorry, I fail to see how it would be different from using existing panels infrastructure and different panels compatibles. I think Rob's idea was to introduce generic edp-connector. If we can't make it generic then let's use panel infrastructure. > > I'd say DT overlays should be preferred solution here, not another > > connector binding. > > Overlays are a way to apply a device tree dynamically. It's orthogonal > to the binding. It isn't orthogonal to original problem though. > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Tue, Jul 09, 2019 at 01:30:18PM -0700, Vasily Khoruzhick wrote: > On Tue, Jul 9, 2019 at 1:55 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Mon, Jul 08, 2019 at 05:49:21PM -0700, Vasily Khoruzhick wrote: > > > > > Maybe instead of edp-connector one would introduce integrator's specific > > > > > connector, for example with compatible "olimex,teres-edp-connector" > > > > > which should follow edp abstract connector rules? This will be at least > > > > > consistent with below presentation[1] - eDP requirements depends on > > > > > integrator. Then if olimex has standard way of dealing with panels > > > > > present in olimex/teres platforms the driver would then create > > > > > drm_panel/drm_connector/drm_bridge(?) according to these rules, I guess. > > > > > Anyway it still looks fishy for me :), maybe because I am not > > > > > familiarized with details of these platforms. > > > > > > > > That makes sense yes > > > > > > Actually, it makes no sense at all. Current implementation for anx6345 > > > driver works fine as is with any panel specified assuming panel delays > > > are long enough for connected panel. It just doesn't use panel timings > > > from the driver. Creating a platform driver for connector itself looks > > > redundant since it can't be reused, it doesn't describe actual > > > hardware and it's just defeats purpose of DT by introducing > > > board-specific code. > > > > I'm not sure where you got the idea that the purpose of DT is to not > > have any board-specific code. > > I believe DT was an attempt to move to declarative approach for > describing hardware. Yes, we have different compatibles for different > devices but they're specific to particular device rather than > particular board. Device interconnection is described in DT along with > some properties rather than in board-specific C-file. You're right, but it's not incompatible with having some code to deal with some board quirk. > Introducing board-specific compatible for a connector isn't looking > right to me. If that board has a board-specific behaviour for it's connector, then what's the issue? You can't describe all the quirks in the all boards using purely properties. > > It's perfectly fine to have some, that's even why there's a compatible > > assigned to each and every board. > > > > What the DT is about is allowing us to have a generic behaviour that > > we can detect: we can have a given behaviour for a given board, and a > > separate one for another one, and this will be evaluated at runtime. > > > > This is *exactly* what this is about: we can have a compatible that > > sets a given, more specific, behaviour (olimex,teres-edp-connector) > > while saying that this is compatible with the generic behaviour > > (edp-connector). That way, any OS will know what quirk to apply if > > needed, and if not that it can use the generic behaviour. > > > > And we could create a generic driver, for the generic behaviour if > > needed. > > > > > There's another issue: if we introduce edp-connector we'll have to > > > specify power up delays somewhere (in dts? or in platform driver?), so > > > edp-connector doesn't really solve the issue of multiple panels with > > > same motherboard. > > > > And that's what that compatible is about :) > > Sorry, I fail to see how it would be different from using existing > panels infrastructure and different panels compatibles. I think Rob's > idea was to introduce generic edp-connector. Again, there's no such thing as a generic edp-connector. The spec doesn't define anything related to the power sequence for example. > If we can't make it generic then let's use panel infrastructure. Which uses a device specific compatible. Really, I'm not sure what your objection and / or argument is here. In addition, when that was brought up in the discussion, you rejected it because it was inconvenient: https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1#comment_535206 And I agree with you on that one. > > > I'd say DT overlays should be preferred solution here, not another > > > connector binding. > > > > Overlays are a way to apply a device tree dynamically. It's orthogonal > > to the binding. > > It isn't orthogonal to original problem though. It is. The original problem is that you want to power up whatever is on the other side of a eDP link using an arbitrary regulator. This is a "how do I describe that in my DT" problem, and it really has nothing to do with how the DT is being passed to the kernel. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Wed, Jul 10, 2019 at 4:40 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Tue, Jul 09, 2019 at 01:30:18PM -0700, Vasily Khoruzhick wrote: > > On Tue, Jul 9, 2019 at 1:55 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > On Mon, Jul 08, 2019 at 05:49:21PM -0700, Vasily Khoruzhick wrote: > > > > > > Maybe instead of edp-connector one would introduce integrator's specific > > > > > > connector, for example with compatible "olimex,teres-edp-connector" > > > > > > which should follow edp abstract connector rules? This will be at least > > > > > > consistent with below presentation[1] - eDP requirements depends on > > > > > > integrator. Then if olimex has standard way of dealing with panels > > > > > > present in olimex/teres platforms the driver would then create > > > > > > drm_panel/drm_connector/drm_bridge(?) according to these rules, I guess. > > > > > > Anyway it still looks fishy for me :), maybe because I am not > > > > > > familiarized with details of these platforms. > > > > > > > > > > That makes sense yes > > > > > > > > Actually, it makes no sense at all. Current implementation for anx6345 > > > > driver works fine as is with any panel specified assuming panel delays > > > > are long enough for connected panel. It just doesn't use panel timings > > > > from the driver. Creating a platform driver for connector itself looks > > > > redundant since it can't be reused, it doesn't describe actual > > > > hardware and it's just defeats purpose of DT by introducing > > > > board-specific code. > > > > > > I'm not sure where you got the idea that the purpose of DT is to not > > > have any board-specific code. > > > > I believe DT was an attempt to move to declarative approach for > > describing hardware. Yes, we have different compatibles for different > > devices but they're specific to particular device rather than > > particular board. Device interconnection is described in DT along with > > some properties rather than in board-specific C-file. > > You're right, but it's not incompatible with having some code to deal > with some board quirk. > > > Introducing board-specific compatible for a connector isn't looking > > right to me. > > If that board has a board-specific behaviour for it's connector, then > what's the issue? > > You can't describe all the quirks in the all boards using purely > properties. > > > > It's perfectly fine to have some, that's even why there's a compatible > > > assigned to each and every board. > > > > > > What the DT is about is allowing us to have a generic behaviour that > > > we can detect: we can have a given behaviour for a given board, and a > > > separate one for another one, and this will be evaluated at runtime. > > > > > > This is *exactly* what this is about: we can have a compatible that > > > sets a given, more specific, behaviour (olimex,teres-edp-connector) > > > while saying that this is compatible with the generic behaviour > > > (edp-connector). That way, any OS will know what quirk to apply if > > > needed, and if not that it can use the generic behaviour. > > > > > > And we could create a generic driver, for the generic behaviour if > > > needed. > > > > > > > There's another issue: if we introduce edp-connector we'll have to > > > > specify power up delays somewhere (in dts? or in platform driver?), so > > > > edp-connector doesn't really solve the issue of multiple panels with > > > > same motherboard. > > > > > > And that's what that compatible is about :) > > > > Sorry, I fail to see how it would be different from using existing > > panels infrastructure and different panels compatibles. I think Rob's > > idea was to introduce generic edp-connector. > > Again, there's no such thing as a generic edp-connector. The spec > doesn't define anything related to the power sequence for example. > > > If we can't make it generic then let's use panel infrastructure. > > Which uses a device specific compatible. Really, I'm not sure what > your objection and / or argument is here. > > In addition, when that was brought up in the discussion, you rejected > it because it was inconvenient: > https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1#comment_535206 It is inconvenient, but I don't understand how having board-specific connectors fixes it. > And I agree with you on that one. > > > > > I'd say DT overlays should be preferred solution here, not another > > > > connector binding. > > > > > > Overlays are a way to apply a device tree dynamically. It's orthogonal > > > to the binding. > > > > It isn't orthogonal to original problem though. > > It is. The original problem is that you want to power up whatever is > on the other side of a eDP link using an arbitrary regulator. > > This is a "how do I describe that in my DT" problem, and it really has > nothing to do with how the DT is being passed to the kernel. > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Wed, Jul 10, 2019 at 03:11:04PM -0700, Vasily Khoruzhick wrote: > On Wed, Jul 10, 2019 at 4:40 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > There's another issue: if we introduce edp-connector we'll have to > > > > > specify power up delays somewhere (in dts? or in platform driver?), so > > > > > edp-connector doesn't really solve the issue of multiple panels with > > > > > same motherboard. > > > > > > > > And that's what that compatible is about :) > > > > > > Sorry, I fail to see how it would be different from using existing > > > panels infrastructure and different panels compatibles. I think Rob's > > > idea was to introduce generic edp-connector. > > > > Again, there's no such thing as a generic edp-connector. The spec > > doesn't define anything related to the power sequence for example. > > > > > If we can't make it generic then let's use panel infrastructure. > > > > Which uses a device specific compatible. Really, I'm not sure what > > your objection and / or argument is here. > > > > In addition, when that was brought up in the discussion, you rejected > > it because it was inconvenient: > > https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1#comment_535206 > > It is inconvenient, but I don't understand how having board-specific > connectors fixes it. How it would not fix it? You'll have one connector, without the need to describe each and every panel in the device tree and rely on the EDID instead, and you'll have the option to power up the regulator you need. I really don't understand what's the issue here, so let's take a step back. What are is the issue , what are your requirements, and how would you like that to be described ? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, Jul 12, 2019 at 1:15 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Wed, Jul 10, 2019 at 03:11:04PM -0700, Vasily Khoruzhick wrote: > > On Wed, Jul 10, 2019 at 4:40 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > There's another issue: if we introduce edp-connector we'll have to > > > > > > specify power up delays somewhere (in dts? or in platform driver?), so > > > > > > edp-connector doesn't really solve the issue of multiple panels with > > > > > > same motherboard. > > > > > > > > > > And that's what that compatible is about :) > > > > > > > > Sorry, I fail to see how it would be different from using existing > > > > panels infrastructure and different panels compatibles. I think Rob's > > > > idea was to introduce generic edp-connector. > > > > > > Again, there's no such thing as a generic edp-connector. The spec > > > doesn't define anything related to the power sequence for example. > > > > > > > If we can't make it generic then let's use panel infrastructure. > > > > > > Which uses a device specific compatible. Really, I'm not sure what > > > your objection and / or argument is here. > > > > > > In addition, when that was brought up in the discussion, you rejected > > > it because it was inconvenient: > > > https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1#comment_535206 > > > > It is inconvenient, but I don't understand how having board-specific > > connectors fixes it. > > How it would not fix it? I think I got your idea, but yet I think it's not the best solution. Do I understand correctly that you're proposing to introduce board-specific edp-connector driver that will be aware of worst case power up delays and will control backlight and power? Then why not to add another board-specific panel (e.g. "pine64,pinebook-panel") to simple-panel.c that does the same? > You'll have one connector, without the need to describe each and every > panel in the device tree and rely on the EDID instead, and you'll have > the option to power up the regulator you need. > > I really don't understand what's the issue here, so let's take a step > back. What are is the issue , what are your requirements, and how > would you like that to be described ? We have a device (Pinebook) that uses the same board with multiple edp panels. So far there're pinebooks with 3 different panels: 11" with 768p panel, 11" with 1080p panel, 14" with 768p panel. Currently there's no way to describe all pinebooks with a single dts. There's a simple workaround though -- we can just specify a panel with worst power up delays in dts and it'll work since anx6345 driver ignores panel modes anyway and uses EDID. Originally I proposed to extend simple-panel driver to support generic edp-panel but it was rejected. I still believe that it's the best solution assuming we can specify delays in dts, since panels list is specific to particular device and it probably can't be reused, i.e. there's no good reason to move it into C code. Rob Herring proposed to introduce edp-connector. While I still believe that it's not accurate description of hardware since it'll have to have backlight node (backlight is actually panel property) I was OK with this approach assuming we can store delays in dts. Later it evolved into board-specific edp-connector. So far I don't understand why everyone is trying to avoid introducing edp-panel driver that can read delays from dts. Basically, I don't understand what's the magic behind simple-panel.c and why new panels should be added there rather than described in dts. [1] Doesn't explain that. [1] http://sietch-tagr.blogspot.com/2016/04/display-panels-are-not-special.html Regards, Vasily > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Mon, Jul 15, 2019 at 05:28:53PM -0700, Vasily Khoruzhick wrote: > On Fri, Jul 12, 2019 at 1:15 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Wed, Jul 10, 2019 at 03:11:04PM -0700, Vasily Khoruzhick wrote: > > > On Wed, Jul 10, 2019 at 4:40 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > > There's another issue: if we introduce edp-connector we'll have to > > > > > > > specify power up delays somewhere (in dts? or in platform driver?), so > > > > > > > edp-connector doesn't really solve the issue of multiple panels with > > > > > > > same motherboard. > > > > > > > > > > > > And that's what that compatible is about :) > > > > > > > > > > Sorry, I fail to see how it would be different from using existing > > > > > panels infrastructure and different panels compatibles. I think Rob's > > > > > idea was to introduce generic edp-connector. > > > > > > > > Again, there's no such thing as a generic edp-connector. The spec > > > > doesn't define anything related to the power sequence for example. > > > > > > > > > If we can't make it generic then let's use panel infrastructure. > > > > > > > > Which uses a device specific compatible. Really, I'm not sure what > > > > your objection and / or argument is here. > > > > > > > > In addition, when that was brought up in the discussion, you rejected > > > > it because it was inconvenient: > > > > https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1#comment_535206 > > > > > > It is inconvenient, but I don't understand how having board-specific > > > connectors fixes it. > > > > How it would not fix it? > > I think I got your idea, but yet I think it's not the best solution. > > Do I understand correctly that you're proposing to introduce > board-specific edp-connector driver that will be aware of worst case > power up delays and will control backlight and power? > > Then why not to add another board-specific panel (e.g. > "pine64,pinebook-panel") to simple-panel.c that does the same? That would be fine for me too. Thierry was against it though IIRC, and I don't recall why exactly. > > You'll have one connector, without the need to describe each and every > > panel in the device tree and rely on the EDID instead, and you'll have > > the option to power up the regulator you need. > > > > I really don't understand what's the issue here, so let's take a step > > back. What are is the issue , what are your requirements, and how > > would you like that to be described ? > > We have a device (Pinebook) that uses the same board with multiple edp > panels. So far there're pinebooks with 3 different panels: 11" with > 768p panel, 11" with 1080p panel, 14" with 768p panel. > > Currently there's no way to describe all pinebooks with a single dts. > There's a simple workaround though -- we can just specify a panel with > worst power up delays in dts and it'll work since anx6345 driver > ignores panel modes anyway and uses EDID. > > Originally I proposed to extend simple-panel driver to support generic > edp-panel but it was rejected. I still believe that it's the best > solution assuming we can specify delays in dts, since panels list is > specific to particular device and it probably can't be reused, i.e. > there's no good reason to move it into C code. > > Rob Herring proposed to introduce edp-connector. While I still believe > that it's not accurate description of hardware since it'll have to > have backlight node (backlight is actually panel property) I was OK > with this approach assuming we can store delays in dts. > > Later it evolved into board-specific edp-connector. I think you got that wrong. As far as I'm concerned, the plan was to have two compatibles: the board-specific one, and the generic one. Something like compatible = "pine64,pinebook-edp-connector", "edp-connector"; or whatever. > So far I don't understand why everyone is trying to avoid introducing > edp-panel driver that can read delays from dts. Basically, I don't > understand what's the magic behind simple-panel.c and why new panels > should be added there rather than described in dts. [1] Doesn't > explain that. So others might have different viewpoints here as well, but the major downside I see in putting those kind of values in the device tree is that at some point, someone will get it wrong, and chances are that even for the same panel, everyone will use a slightly different set of timings. And once it's wrong, then it's a mess to fix. You have to track down every DT using it, make sure it's corrected, and then every user will have to change their DT in their system. Whereas if you have just a compatible and those timings in the kernel, then the only thing required is a kernel update, which should be a pretty standard operation. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts index 0ec46b969a75..a0ad438b037f 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-teres-i.dts @@ -65,6 +65,21 @@ }; }; + panel: panel { + compatible ="innolux,n116bge", "simple-panel"; + status = "okay"; + power-supply = <®_dcdc1>; + backlight = <&backlight>; + + ports { + panel_in: port { + panel_in_edp: endpoint { + remote-endpoint = <&anx6345_out>; + }; + }; + }; + }; + reg_usb1_vbus: usb1-vbus { compatible = "regulator-fixed"; regulator-name = "usb1-vbus"; @@ -81,20 +96,48 @@ }; }; +&de { + status = "okay"; +}; + &ehci1 { status = "okay"; }; -/* The ANX6345 eDP-bridge is on i2c0. There is no linux (mainline) - * driver for this chip at the moment, the bootloader initializes it. - * However it can be accessed with the i2c-dev driver from user space. - */ &i2c0 { clock-frequency = <100000>; pinctrl-names = "default"; pinctrl-0 = <&i2c0_pins>; status = "okay"; + + anx6345: anx6345@38 { + compatible = "analogix,anx6345"; + reg = <0x38>; + reset-gpios = <&pio 3 24 GPIO_ACTIVE_LOW>; /* PD24 */ + dvdd25-supply = <®_dldo2>; + dvdd12-supply = <®_dldo3>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + anx6345_in: endpoint { + remote-endpoint = <&tcon0_out_anx6345>; + }; + }; + port@1 { + anx6345_out: endpoint { + remote-endpoint = <&panel_in_edp>; + }; + }; + }; + }; +}; + +&mixer0 { + status = "okay"; }; &mmc0 { @@ -279,6 +322,20 @@ vcc-hdmi-supply = <®_dldo1>; }; +&tcon0 { + pinctrl-names = "default"; + pinctrl-0 = <&lcd_rgb666_pins>; + + status = "okay"; +}; + +&tcon0_out { + tcon0_out_anx6345: endpoint@0 { + reg = <0>; + remote-endpoint = <&anx6345_in>; + }; +}; + &uart0 { pinctrl-names = "default"; pinctrl-0 = <&uart0_pb_pins>;