Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CAD=FV=Uq6K95FBkKYn=M6+7cfyam11n_f-9AMxosmDBZQYfmsQ@mail.gmail.com>
References: <20200504213624.1.Ibc8eeddcee94984a608d6900b46f9ffde4045da4@changeid> <158865745768.11125.12003632060774071567@swboyd.mtv.corp.google.com> <CAD=FV=Uq6K95FBkKYn=M6+7cfyam11n_f-9AMxosmDBZQYfmsQ@mail.gmail.com>
Subject: Re: [PATCH] drm/bridge: ti-sn65dsi86: Implement lane reordering + polarity
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     Andrzej Hajda <a.hajda@samsung.com>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Rob Clark <robdclark@chromium.org>,
        Sean Paul <seanpaul@chromium.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@linux.ie>,
        Jernej Skrabec <jernej.skrabec@siol.net>,
        Jonas Karlman <jonas@kwiboo.se>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        LKML <linux-kernel@vger.kernel.org>
To:     Doug Anderson <dianders@chromium.org>
Date:   Tue, 05 May 2020 12:52:58 -0700
Message-ID: <158870837878.26370.13953763070282841423@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Doug Anderson (2020-05-05 11:45:05)
> On Mon, May 4, 2020 at 10:44 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Douglas Anderson (2020-05-04 21:36:31)
> > >         regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
> > >                            CHA_DSI_LANES_MASK, val);
> > >
> > > +       regmap_write(pdata->regmap, SN_LN_ASSIGN_REG, pdata->ln_assig=
n);
> > > +       regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, LN_POLRS_=
MASK,
> > > +                          pdata->ln_polrs << LN_POLRS_OFFSET);
> > > +
> > >         /* set dsi clk frequency value */
> > >         ti_sn_bridge_set_dsi_rate(pdata);
> > >
> > > @@ -1063,6 +1066,50 @@ static int ti_sn_setup_gpio_controller(struct =
ti_sn_bridge *pdata)
> > >         return ret;
> > >  }
> > >
> > > +static void ti_sn_bridge_parse_lanes(struct ti_sn_bridge *pdata,
> > > +                                    struct device_node *np)
> > > +{
> > > +       u32 lane_assignments[SN_MAX_DP_LANES] =3D { 0, 1, 2, 3 };
> > > +       u32 lane_polarities[SN_MAX_DP_LANES] =3D { };
> > > +       struct device_node *endpoint;
> > > +       u8 ln_assign =3D 0;
> > > +       u8 ln_polrs =3D 0;
> >
> > Do we need to assign to 0 to start? Seems like no?
>=20
> Yes.  See usage:
>=20
>   ln_assign =3D ln_assign << LN_ASSIGN_WIDTH | lane_assignments[i];
>   ln_polrs =3D ln_polrs << 1 | lane_polarities[i];
>=20
> Notably each time we shift a new bit in we base on the old value.  If
> you think it'll make it clearer, I can put this initialization at the
> beginning of the loop.  It's 2 extra lines of code but if it adds
> clarity I'll do it.

No it doesn't really make it any clearer.