Re: [RFC PATCH v2] media: cec: expose HDMI connector to CEC dev mapping

[Dariusz Marcinkiewicz <darekm@google.com>]

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.
...
> >       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.
...
> > +/**
> > + * 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!