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

[Marek Vasut]

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 ?

[...]

>> +#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

[...]