On 5/16/19 4:10 PM, Dariusz Marcinkiewicz wrote: > 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. I should be able to test the dw-hdmi patch next week, but testing the tda9950 patch has to wait until the week after that. Regards, Hans > > Thank you and best regards. >
