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

[Marek Vasut]

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.