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

[Dave Stevenson]

On Thu, 6 May 2021 at 21:49, Marek Vasut <marex at denx.de> wrote:
>
> On 5/6/21 7:03 PM, Dave Stevenson wrote:
> > 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.
>
> Good
>
> > [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 :-/
>
> So I can add a variable , assign it the value of
> sn65dsi83_get_lvds_range(ctx) and then do
> REG_RC_LVDS_PLL_LVDS_CLK_RANGE(val), but that doesn't really improve the
> readability at all, it just adds extra indirection.

Unless drm is sticking hard to the older limit, "bdc48fa11e46
checkpatch/coding-style: deprecate 80-column warning" allows you up to
100 chars.
Just going with the natural indentation is therefore fine and makes
checkpatch happy even in strict mode.

  Dave