Hi Geert,
Tanks for your feedback.
I have taken all your comments for v2 except the i2c address of adv7126,
see reason below.
On 2024-02-15 19:32:32 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Tue, Jan 23, 2024 at 3:54 PM Niklas Söderlund
> <niklas.soderlund+renesas@xxxxxxxxxxxx> wrote:
> > The Eagle board supports an optional expansion board. The expansion
>
> The title page of the schematics document calls this the "Function
> expansion board".
>
> > board adds support for HDMI OUT, HDMI capture from two different sources
> > and eMMC.
> >
> > This change only adds support for the two HDMI capture sources.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>
> Thanks for your patch!
>
> > --- a/arch/arm64/boot/dts/renesas/Makefile
> > +++ b/arch/arm64/boot/dts/renesas/Makefile
> > @@ -62,6 +62,8 @@ dtb-$(CONFIG_ARCH_R8A77965) += r8a77965-ulcb.dtb
> > dtb-$(CONFIG_ARCH_R8A77965) += r8a77965-ulcb-kf.dtb
> >
> > dtb-$(CONFIG_ARCH_R8A77970) += r8a77970-eagle.dtb
>
> Please add
>
> dtb-$(CONFIG_ARCH_R8A77970) += r8a77970-eagle-expansion.dtbo
>
> so that the .dtbo is considered for installation, too.
>
> > +r8a77970-eagle-expansion-dtbs := r8a77970-eagle.dtb r8a77970-eagle-expansion.dtbo
> > +dtb-$(CONFIG_ARCH_R8A77970) += r8a77970-eagle-expansion.dtb
> > dtb-$(CONFIG_ARCH_R8A77970) += r8a77970-v3msk.dtb
> >
> > dtb-$(CONFIG_ARCH_R8A77980) += r8a77980-condor.dtb
>
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle-expansion.dtso
>
> This is a rather generic name.
> What about r8a77970-eagle-function-expansion.dtso?
>
> > @@ -0,0 +1,214 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device Tree Source for the Eagle V3M expansion board.
>
> ... Function expansion board?
>
> > + *
> > + * Copyright (C) 2024 Niklas Söderlund <niklas.soderlund@xxxxxxxxxxxx>
> > + */
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/interrupt-controller/irq.h>
>
> Please move the includes below the /.../; markers.
>
> > +
> > +/dts-v1/;
> > +/plugin/;
> > +
> > +/ {
> > + /* CN4 */
> > + /* Eagle: SW18 set to OFF */
> > + cvbs-in-cn4 {
> > + compatible = "composite-video-connector";
> > + label = "CVBS IN CN4";
> > +
> > + port {
> > + cvbs_con: endpoint {
> > + remote-endpoint = <&adv7482_ain7>;
> > + };
> > + };
> > + };
> > +
> > + /* CN3 */
> > + /* Eagle: SW18 set to OFF */
> > + hdmi-in-cn3 {
>
> Please use alphabetical sort order for nodes without unit addresses.
>
> > + compatible = "hdmi-connector";
> > + label = "HDMI IN CN3";
> > + type = "a";
> > +
> > + port {
> > + hdmi_in_con: endpoint {
> > + remote-endpoint = <&adv7482_hdmi>;
> > + };
> > + };
> > + };
> > +
> > + /* CN2 */
> > + /* Eagle: SW35 set 5, 6 and 8 to OFF */
> > + hdmi-in-cn2 {
> > + compatible = "hdmi-connector";
> > + label = "HDMI IN CN2";
> > + type = "a";
> > +
> > + port {
> > + hdmi_in_con2: endpoint {
> > + remote-endpoint = <&adv7612_in>;
> > + };
> > + };
> > + };
> > +};
> > +
> > +/* Disconnect MAX9286 GMSL i2c. */
>
> I2C
>
> > +&i2c3 {
> > + status = "disabled";
> > +};
> > +
> > +/* Connect expansion board i2c. */
>
> I2C
>
> > +&i2c0 {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + io_expander_27: gpio@27 {
> > + compatible = "onnn,pca9654";
> > + reg = <0x27>;
> > + gpio-controller;
> > + #gpio-cells = <2>;
> > +
> > + vin0_adv7612_en {
> > + gpio-hog;
> > + gpios = <0x3 0x0>;
>
> Please use symbolic values for GPIO flags, i.e. GPIO_ACTIVE_HIGH.
>
> > + output-low;
> > + line-name = "VIN0_ADV7612_ENn";
>
> Given this signal is active-low, you probably want:
>
> gpios = <3 GPIO_ACTIVE_LOW>;
> output-high;
>
> > + };
> > + };
> > +
> > + dmi-decoder@4c {
>
> hdmi-decoder
>
> According to the schematics, the primary address is 0x4d?
Indeed it is, but it do not answer at 0x4d but at 0x4c, I do not know
why, maybe the datasheet is wrong? I have had some version or not of
this change in my tree for years so I can't remember how I figured this
out, but I just retested and nothing answers at 0x4d but capture works
find with 0x4c.
>
> > + compatible = "adi,adv7612";
> > + reg = <0x4c>, <0x50>, <0x52>, <0x54>, <0x56>, <0x58>;
> > + reg-names = "main", "afe", "rep", "edid", "hdmi", "cp";
> > + interrupt-parent = <&gpio3>;
> > + interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> > + default-input = <0>;
>
> > + };
> > +
> > + adv7482_70: video-receiver@70 {
>
> Do you see a future user for this label?
> Just wondering, as some nodes have labels, others haven't.
>
> > + compatible = "adi,adv7482";
> > + reg = <0x70 0x71 0x72 0x73 0x74 0x75
> > + 0x60 0x61 0x62 0x63 0x64 0x65>;
> > + reg-names = "main", "dpll", "cp", "hdmi", "edid", "repeater",
> > + "infoframe", "cbus", "cec", "sdp", "txa", "txb" ;
> > + interrupt-parent = <&gpio3>;
> > + interrupts = <03 IRQ_TYPE_LEVEL_LOW>, <04 IRQ_TYPE_LEVEL_LOW>;
> > + interrupt-names = "intrq1", "intrq2";
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
--
Kind Regards,
Niklas Söderlund
