LinuxLists.cc - [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I

2019-06-04 12:25:08

Subject: [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I

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 <[email protected]>
Signed-off-by: Torsten Duwe <[email protected]>
---
.../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";
+ status = "okay";
+ power-supply = <&reg_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 = <&reg_dldo2>;
+ dvdd12-supply = <&reg_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 = <&reg_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

2019-06-04 15:10:47

by Vasily Khoruzhick

[permalink] [raw]

Subject: Re: [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I

On Tue, Jun 4, 2019 at 5:23 AM Torsten Duwe <[email protected]> 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 <[email protected]>
> Signed-off-by: Torsten Duwe <[email protected]>
> ---
> .../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 = <&reg_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 = <&reg_dldo2>;
> + dvdd12-supply = <&reg_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 = <&reg_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
>

2019-06-05 10:15:19

by Torsten Duwe

[permalink] [raw]

Subject: Re: [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I

On Tue, Jun 04, 2019 at 08:08:40AM -0700, Vasily Khoruzhick wrote:
> On Tue, Jun 4, 2019 at 5:23 AM Torsten Duwe <[email protected]> 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 <[email protected]>
> > Signed-off-by: Torsten Duwe <[email protected]>
> > ---
> > .../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

2019-06-05 12:04:42

(No filename) (4.61 kB)
signature.asc (235.00 B)
Download all attachments

2019-07-10 22:13:38

by Vasily Khoruzhick

[permalink] [raw]

Subject: Re: [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I

On Wed, Jul 10, 2019 at 4:40 AM Maxime Ripard <[email protected]> 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 <[email protected]> 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

2019-07-12 20:16:44

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I

On Wed, Jul 10, 2019 at 03:11:04PM -0700, Vasily Khoruzhick wrote:
> On Wed, Jul 10, 2019 at 4:40 AM Maxime Ripard <[email protected]> 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

2019-07-16 00:31:24

by Vasily Khoruzhick

[permalink] [raw]

Subject: Re: [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I

On Fri, Jul 12, 2019 at 1:15 PM Maxime Ripard <[email protected]> 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 <[email protected]> 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

2019-07-24 14:11:20

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v2 7/7] arm64: dts: allwinner: a64: enable ANX6345 bridge on Teres-I

On Mon, Jul 15, 2019 at 05:28:53PM -0700, Vasily Khoruzhick wrote:
> On Fri, Jul 12, 2019 at 1:15 PM Maxime Ripard <[email protected]> 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 <[email protected]> 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

Attachments:

(No filename) (4.86 kB)
signature.asc (235.00 B)
Download all attachments