MIME-Version: 1.0
References: <20230311231007.46174-1-aford173@gmail.com> <6ae67215-28b6-04bf-b61d-1ba99ffdf6fb@linaro.org>
In-Reply-To: <6ae67215-28b6-04bf-b61d-1ba99ffdf6fb@linaro.org>
From:   Robert Foss <rfoss@kernel.org>
Date:   Mon, 13 Mar 2023 14:56:22 +0100
Message-ID: <CAN6tsi5jW8QOGe3yMQzFMsU7f9=HKOx8a_2azV3rTRNaz-v=0g@mail.gmail.com>
Subject: Re: [PATCH] drm/bridge: adv7533: Fix adv7533_mode_valid for adv7533
 and adv7535
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc:     Adam Ford <aford173@gmail.com>, dri-devel@lists.freedesktop.org,
        aford@beaconembedded.com, quic_abhinavk@quicinc.com,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Mar 12, 2023 at 2:10=E2=80=AFAM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 12/03/2023 01:10, Adam Ford wrote:
> > When dynamically switching lanes was removed, the intent of the code
> > was to check to make sure that higher speed items used 4 lanes, but
> > it had the unintended consequence of removing the slower speeds for
> > 4-lane users.
> >
> > This attempts to remedy this by doing a check to see that the
> > max frequency doesn't exceed the chip limit, and a second
> > check to make sure that the max bit-rate doesn't exceed the
> > number of lanes * max bit rate / lane.
> >
> > Fixes: 9a0cdcd6649b ("drm/bridge: adv7533: remove dynamic lane switchin=
g from adv7533 bridge")
> >
>
> Please remove the empty line here. There should be no empty lines
> between the tags
>
>
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm=
/bridge/adv7511/adv7533.c
> > index fdfeadcefe80..10a112d36945 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> > @@ -103,13 +103,9 @@ void adv7533_dsi_power_off(struct adv7511 *adv)
> >   enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv,
> >                                       const struct drm_display_mode *mo=
de)
> >   {
> > -     int lanes;
> > +     unsigned long max_lane_freq;
> >       struct mipi_dsi_device *dsi =3D adv->dsi;
> > -
> > -     if (mode->clock > 80000)
> > -             lanes =3D 4;
> > -     else
> > -             lanes =3D 3;
> > +     u8 bpp =3D mipi_dsi_pixel_format_to_bpp(dsi->format);
> >
> >       /*
> >        * TODO: add support for dynamic switching of lanes
>
> Drop the comment please. It makes little sense with your new implementati=
on.
>
> > @@ -117,8 +113,16 @@ enum drm_mode_status adv7533_mode_valid(struct adv=
7511 *adv,
> >        * out the modes which shall need different number of lanes
> >        * than what was configured in the device tree.
> >        */
> > -     if (lanes !=3D dsi->lanes)
> > -             return MODE_BAD;
> > +
> > +     /* Check max clock for either 7533 or 7535 */
> > +     if (mode->clock > (adv->type =3D=3D ADV7533 ? 80000 : 148500))
> > +             return MODE_CLOCK_HIGH;
> > +
> > +     /* Check max clock for each lane */
> > +     max_lane_freq =3D (adv->type =3D=3D ADV7533 ? 800000 : 891000);
> > +
> > +     if (mode->clock * bpp > max_lane_freq * adv->num_dsi_lanes)
> > +             return MODE_CLOCK_HIGH;
> >
> >       return MODE_OK;
> >   }
>
> --
> With best wishes
> Dmitry
>

With Dmitrys feedback, feel free to add my Reviewed-by.