Hi Alvin, Thank you for the patch.
CC'ing Hans Verkuil, to review the CEC side. On Sat, Oct 14, 2023 at 09:43:01PM +0200, Alvin Šipraga wrote: > From: Alvin Šipraga <a...@bang-olufsen.dk> > > The adv7511 driver is solely responsible for setting the physical > address of its CEC adapter. To do this, it must read the EDID. However, > EDID is only read when either the drm_bridge_funcs :: get_edid or > drm_connector_helper_funcs :: get_modes ops are called. Without loss of > generality, it cannot be assumed that these ops are called when a sink > gets attached. Therefore there exist scenarios in which the CEC physical > address will be invalid (f.f.f.f), rendering the CEC adapter inoperable. > > Address this problem by always fetching the EDID in the HPD work when we > detect a connection. The CEC physical address is set in the process. > > Signed-off-by: Alvin Šipraga <a...@bang-olufsen.dk> > --- > Pardon the insertion of the ugly adv7511_get_edid() prototype, but I did > not want to clobber git history by rearranging a bunch of functions. If > this is the preferred approach I will happily re-spin the patch. There's nothing wrong in rearranging functions, it is actually preferred to adding forward declarations. You can submit a set of two patches, one to reorder the functions, and then a second one to fix the problem. This makes review easier by isolating the refactoring with no functional change from the functional changes. > --- > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 2611afd2c1c1..3d32c109963c 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > @@ -424,6 +424,9 @@ static bool adv7511_hpd(struct adv7511 *adv7511) > return false; > } > > +static struct edid *adv7511_get_edid(struct adv7511 *adv7511, > + struct drm_connector *connector); > + > static void adv7511_hpd_work(struct work_struct *work) > { > struct adv7511 *adv7511 = container_of(work, struct adv7511, hpd_work); > @@ -457,6 +460,9 @@ static void adv7511_hpd_work(struct work_struct *work) > if (adv7511->connector.dev) { > if (status == connector_status_disconnected) > cec_phys_addr_invalidate(adv7511->cec_adap); > + else > + adv7511_get_edid(adv7511, &adv7511->connector); > + > drm_kms_helper_hotplug_event(adv7511->connector.dev); > } else { > drm_bridge_hpd_notify(&adv7511->bridge, status); > -- Regards, Laurent Pinchart