Re: [PATCH] arm64: dts: renesas: eagle: Add capture overlay for expansion board

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux