Hi Hans. From: Hans Verkuil <hverkuil@xxxxxxxxx> Date: Thu, May 9, 2019 at 11:31 AM To: Dariusz Marcinkiewicz Cc: <linux-media@xxxxxxxxxxxxxxx>, <hans.verkuil@xxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx> > On 5/9/19 9:52 AM, Dariusz Marcinkiewicz wrote: > > Hi Hans. > > > > On Wed, Apr 24, 2019 at 2:09 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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.
