[PATCH V3 2/2] drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver

[Dave Stevenson]

On Thu, 6 May 2021 at 13:48, Marek Vasut <marex at denx.de> wrote:
>
> On 5/6/21 11:45 AM, Dave Stevenson wrote:
> > Hi Marek
>
> Hi,
>
> > I'm taking an interest as there are a number of Raspberry Pi users
> > trying to get this chip up and running (not there quite yet).
> > A couple of fairly minor comments
>
> Is there any readily available display unit / expansion board with this
> chip ?

Not that I'm aware of. It's a forum thread[1] where two different
users are trying to bring up the chip, each with their own boards. One
has just reported they have got it working with Jagan's patch set but
with a load of hacks to both bridge and DSI drivers.
If Laurent has a board then that may be a useful further test route.

I'm not 100% convinced that the Pi is doing exactly the right thing
with regard LP-11 state during initialisation, but hope to get into
the lab to check either tomorrow or early next week.

[1] https://www.raspberrypi.org/forums/viewtopic.php?f=44&t=305690

> [...]
>
> >> +#define REG_DSI_LANE                           0x10
> >> +#define  REG_DSI_LANE_LVDS_LINK_CFG_DUAL       BIT(5) /* dual or 2x single */
> >
> > The bit name here seems a little odd.
> > Bits 5&6 are the DSI channel mode on SN65DSI85, not the LVDS link
> > config (which appears to be reg 0x18 bit 4)
> > DSI_CHANNEL_MODE
> > 00 – Dual-channel DSI receiver
> > 01 – Single channel DSI receiver (default)
> > 10 – Two single channel DSI receivers
> > 11 – Reserved
> > SN65DSI83 and 84 require it to be set to 01. You have that end result,
> > but using an odd register name that only documents one of the 2 bits.
> >
> > Is it worth documenting bit 7 as being the '85 Dual DSI link
> > LEFT_RIGHT_PIXELS flag at the same time? The chip isn't supported in
> > dual DSI mode at present, but defining all the registers up front
> > seems reasonable.
>
> Yes, let's fix those.
>
> [...]
>
> >> +       /*
> >> +        * Reset the chip, pull EN line low for t_reset=10ms,
> >> +        * then high for t_en=1ms.
> >> +        */
> >> +       regcache_mark_dirty(ctx->regmap);
> >> +       gpiod_set_value(ctx->enable_gpio, 0);
> >> +       usleep_range(10000, 11000);
> >> +       gpiod_set_value(ctx->enable_gpio, 1);
> >> +       usleep_range(1000, 1100);
> >> +}
> >
> > Whilst section 6.6 of the SN65DSI84 datasheet does list t_en as 1ms,
> > the initialization sequence listed in table 7.2 states
> > Init seq 3 - Set EN pin to Low
> > Wait 10 ms
> > Init seq 4 - Tie EN pin to High
> > Wait 10 ms
> >
> > with the note that these are "Minimum recommended delay. It is fine to
> > exceed these."
> >
> > Have you had alternate guidance from TI over that delay?
>
> No, I trusted the timing diagrams in section 6.6 of the datasheet. I
> would like to believe those are correct, while the init sequence listing
> might be a copy-paste error.

It's a tough one to call as to which is going to be correct. I just
thought I'd flag it.

> [...]
>
> >> +static void sn65dsi83_enable(struct drm_bridge *bridge)
> >> +{
> >> +       struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> >> +       unsigned int pval;
> >> +       u16 val;
> >> +       int ret;
> >> +
> >> +       /* Clear reset, disable PLL */
> >> +       regmap_write(ctx->regmap, REG_RC_RESET, 0x00);
> >> +       regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> >> +
> >> +       /* Reference clock derived from DSI link clock. */
> >> +       regmap_write(ctx->regmap, REG_RC_LVDS_PLL,
> >> +               REG_RC_LVDS_PLL_LVDS_CLK_RANGE(sn65dsi83_get_lvds_range(ctx)) |
> >> +               REG_RC_LVDS_PLL_HS_CLK_SRC_DPHY);
> >
> > (Checkpatch whinge for "Alignment should match open parenthesis" on
> > several lines through this function)
>
> Do you have any extra checkpatch settings somewhere ?
> I get this on current next:
>
> linux-2.6$ ./scripts/checkpatch.pl -f drivers/gpu/drm/bridge/ti-sn65dsi83.c
> total: 0 errors, 0 warnings, 625 lines checked

Sorry, yes "./scripts/checkpatch.pl --strict --codespell". Too much
working in the linux-media realms where --strict is required :-/

  Dave

> [...]