On Thu, 6 May 2021 at 21:49, Marek Vasut <marex@xxxxxxx> wrote: > > On 5/6/21 7:03 PM, Dave Stevenson wrote: > > On Thu, 6 May 2021 at 13:48, Marek Vasut <marex@xxxxxxx> 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
