Hi Marek,
On Thu, May 06, 2021 at 02:48:11PM +0200, Marek Vasut 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 ?
For what it's worth, I have a board with a Raspberry Pi CM4 and a
SN65DSI83. It's a customer design, not an off-the-shelf part I'm afraid,
but I plan to eventually test your driver on it.
> [...]
>
> >> +#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.
>
> [...]
>
> >> +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
>
> [...]
--
Regards,
Laurent Pinchart
