Re: [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I
On Tue, Jul 9, 2019 at 1:55 AM Maxime Ripard 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
Re: [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I
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 signature.asc Description: PGP signature
Re: [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I
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=1 https://patchwork.freedesktop.org/patch/283012/?series=56163=1 https://patchwork.freedesktop.org/patch/286468/?series=56776=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 signature.asc Description: PGP signature
