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. > ... >>> +/** >>> + * struct cec_event_connector - tells if and which connector is associated >>> + * with the CEC adapter. >>> + * @card_no: drm card number, -1 if no connector >> >> If there is no connector, then type is NO_CONNECTOR. So this >> doesn't make sense. Wouldn't it be better to just use '__u32 card_no'? >> > Yes, removed (leftover from previous revision where there was no > connector type field). > This and remaining comments are (hopefully) resolved in the new > version of the patch, I've just sent. > > Will add more docs in subsequent revs. > > Thank you! > Regards, Hans
