Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190416083852.126786-1-darekm@google.com> <dcb189af-f67f-ede3-b4fb-de1da7b53ccb@xs4all.nl>
 <CALFZZQEoiikyXmHJyEgEJTRNryTN8ScNK4P3bDCp-9v9GtvU8g@mail.gmail.com> <b78864b4-e470-99b2-5505-890d178b72f8@xs4all.nl>
In-Reply-To: <b78864b4-e470-99b2-5505-890d178b72f8@xs4all.nl>
From:   Dariusz Marcinkiewicz <darekm@google.com>
Date:   Thu, 16 May 2019 16:10:00 +0200
Message-ID: <CALFZZQF9qJbDsG1nzdjTxaG=89Pu0k=pxg3TChMQcf+hDxaeuw@mail.gmail.com>
Subject: Re: [RFC PATCH v2] media: cec: expose HDMI connector to CEC dev mapping
To:     Hans Verkuil <hverkuil@xs4all.nl>
Cc:     linux-media@vger.kernel.org, hans.verkuil@cisco.com,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Hans.

From: Hans Verkuil <hverkuil@xs4all.nl>
Date: Thu, May 9, 2019 at 11:31 AM
To: Dariusz Marcinkiewicz
Cc: <linux-media@vger.kernel.org>, <hans.verkuil@cisco.com>,
<linux-kernel@vger.kernel.org>

> On 5/9/19 9:52 AM, Dariusz Marcinkiewicz wrote:
> > Hi Hans.
> >
> > On Wed, Apr 24, 2019 at 2:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> Hi Dariusz,
> >>
> >> This is getting close, so I think for the next version you can drop
> >> the RFC tag.
> >>
> >> Some comments:
> >>
> > ...
> >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c
> >>> @@ -261,7 +261,7 @@ static int dw_hdmi_cec_probe(struct platform_device *pdev)
> >>>       cec->adap = cec_allocate_adapter(&dw_hdmi_cec_ops, cec, "dw_hdmi",
> >>>                                        CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT |
> >>>                                        CEC_CAP_RC | CEC_CAP_PASSTHROUGH,
> >>> -                                      CEC_MAX_LOG_ADDRS);
> >>> +                                      CEC_MAX_LOG_ADDRS, NULL);
> >>
> >> Hmm, the connector information is actually available through cec->hdmi.
> >>
> >> I think it would make sense to create a helper function that fills in
> >> struct cec_connector_info based on a struct drm_connector pointer.
> >> And add a function to drivers/gpu/drm/bridge/synopsys/dw-hdmi.c that
> >> dw-hdmi-cec.c can call that does the same.
> >
> > Looking at the code here, is the connector info guaranteed to be
> > available at the time cec_allocate_adapter is called here?
> > drm_connector won't be initialized until dw_hdmi_bridge_attach is
> > called, which happens after the cec platform device is created.
>
> Good point. The creation of the cec platform device should probably
> be moved to dw_hdmi_bridge_attach.
>
> > ...
> >>>       priv->adap = cec_allocate_adapter(&tda9950_cec_ops, priv, "tda9950",
> >>>                                         CEC_CAP_DEFAULTS,
> >>> -                                       CEC_MAX_LOG_ADDRS);
> >>> +                                       CEC_MAX_LOG_ADDRS,
> >>> +                                       NULL);
> >>
> >> Here too the drm_connector can be found via struct tda9950_glue.
> >> So it is easy to provide proper connector information.
> >
> > The same concern as with the comment before.
>
> Same solution: this has to be moved.
>
> I have hardware to test patches for both drivers. It might take 2-3 weeks
> before I can test as I don't always has access to the hardware, but at
> least I can verify that moving this code won't break anything.
>
> It's best to first move the code in separate patches before applying the
> "expose HDMI connector to CEC dev mapping" patch on top of them.
>

I've submitted another revision of the changes, with those 2 patches
added on top. Hope that is ok.

Please take a look. It would be great if you could give those 2
patches a go on an actual hardware.

Thank you and best regards.