[PATCH] drm: bridge: adv7511: fix reading edid segments

[Emil Abildgaard Svendsen]

On Fri, Oct 27, 2023 at 12:51:07PM +0300, Jani Nikula wrote:
> On Thu, 26 Oct 2023, Emil Abildgaard Svendsen <EMAS at bang-olufsen.dk> wrote:
> > Currently reading EDID only works because usually only two EDID blocks
> > of 128 bytes is used. Where an EDID segment holds 256 bytes or two EDID
> > blocks. And the first EDID segment read works fine but E-EDID specifies
> > up to 128 segments.
> >
> > The logic is broken so change EDID segment index to multiple of 256
> > bytes and not 128 (block size).
> >
> > Also change check of DDC status. Instead of silently not reading EDID
> > when in "IDLE" state [1]. Check if the DDC controller is in reset
> > instead (no HPD).
> 
> "Also" in a commit message is often a good indication that the patch
> should be split to do the "also" in a separate patch. One "thing" per
> patch. Here, it'll be useful for debugging [1], too, to figure out which
> part broken things. (I suspect it's the status handling.)

Thank you for the tip! I have now split it into two [1]. Yes, I believe
you're correct, it's the status handling.

[1] https://lore.kernel.org/all/20231027122214.599067-1-emas@bang-olufsen.dk/
> 
> 
> BR,
> Jani.
> 
> 
> [1] https://lore.kernel.org/r/5aa375f1-07cd-4e28-8cd5-7e10b4b05c6a@kontron.de
> 
> 
> >
> > [1]
> > ADV7511 Programming Guide: Table 11: DDCController Status:
> >
> >   0xC8 [3:0]  DDC Controller State
> >
> >   0000        In Reset (No Hot Plug Detected)
> >   0001        Reading EDID
> >   0010        IDLE (Waiting for HDCP Requested)
> >   0011        Initializing HDCP
> >   0100        HDCP Enabled
> >   0101        Initializing HDCP Repeater
> >
> > Signed-off-by: Emil Svendsen <emas at bang-olufsen.dk>
> > ---
> >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 24 ++++++++++++--------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > index 8be235144f6d..c641c2ccd412 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > @@ -537,6 +537,8 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
> >                                 size_t len)
> >  {
> >       struct adv7511 *adv7511 = data;
> > +     struct device* dev = &adv7511->i2c_edid->dev;
> > +     int edid_segment = block / 2;
> >       struct i2c_msg xfer[2];
> >       uint8_t offset;
> >       unsigned int i;
> > @@ -545,7 +547,7 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
> >       if (len > 128)
> >               return -EINVAL;
> >
> > -     if (adv7511->current_edid_segment != block / 2) {
> > +     if (adv7511->current_edid_segment != edid_segment) {
> >               unsigned int status;
> >
> >               ret = regmap_read(adv7511->regmap, ADV7511_REG_DDC_STATUS,
> > @@ -553,15 +555,19 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
> >               if (ret < 0)
> >                       return ret;
> >
> > -             if (status != 2) {
> > -                     adv7511->edid_read = false;
> > -                     regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT,
> > -                                  block);
> > -                     ret = adv7511_wait_for_edid(adv7511, 200);
> > -                     if (ret < 0)
> > -                             return ret;
> > +             if (!(status & 0x0F)) {
> > +                     dev_dbg(dev, "DDC in reset no hot plug detected %x\n",
> > +                              status);
> > +                     return -EIO;
> >               }
> >
> > +             adv7511->edid_read = false;
> > +             regmap_write(adv7511->regmap, ADV7511_REG_EDID_SEGMENT,
> > +                          edid_segment);
> > +             ret = adv7511_wait_for_edid(adv7511, 200);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> >               /* Break this apart, hopefully more I2C controllers will
> >                * support 64 byte transfers than 256 byte transfers
> >                */
> > @@ -589,7 +595,7 @@ static int adv7511_get_edid_block(void *data, u8 *buf, unsigned int block,
> >                       offset += 64;
> >               }
> >
> > -             adv7511->current_edid_segment = block / 2;
> > +             adv7511->current_edid_segment = edid_segment;
> >       }
> >
> >       if (block % 2 == 0)
> 
> --
> Jani Nikula, Intel