[PATCH v3 1/2] drm/bridge: Add Cadence DSI driver

[Tomi Valkeinen]


Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

On 19/09/17 16:48, Boris Brezillon wrote:
> On Tue, 19 Sep 2017 16:38:31 +0300
> Tomi Valkeinen <tomi.valkeinen at ti.com> wrote:
> 
>> 
>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>
>> On 19/09/17 16:25, Boris Brezillon wrote:
>>> On Tue, 19 Sep 2017 15:59:20 +0300
>>> Tomi Valkeinen <tomi.valkeinen at ti.com> wrote:
>>>   
>>>> Hi Boris,
>>>>
>>>>
>>>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>>>>
>>>> On 31/08/17 18:55, Boris Brezillon wrote:  
>>>>> Add a driver for Cadence DPI -> DSI bridge.
>>>>>
>>>>> This driver only support a subset of Cadence DSI bridge capabilities.
>>>>>
>>>>> Here is a non-exhaustive list of missing features:
>>>>>  * burst mode
>>>>>  * dynamic configuration of the DPHY based on the
>>>>>  * support for additional input interfaces (SDI input)
>>>>>
>>>>> Signed-off-by: Boris Brezillon <boris.brezillon at free-electrons.com>    
>>>>
>>>> <snip>
>>>>  
>>>>> +	dsi->pclk = devm_clk_get(&pdev->dev, "pclk");
>>>>> +	if (IS_ERR(dsi->pclk))
>>>>> +		return PTR_ERR(dsi->pclk);    
>>>>
>>>> What's the purpose of pclk? Isn't that normally dealt with the normal
>>>> modesetting, enabled with the video stream? How could it even be enabled
>>>> here, without anyone setting the rate?  
>>>
>>> It's the peripheral clock, not the pixel clock, and AFAIU it has to be
>>> enabled before accessing DSI registers.  
>>
>> Is that the dsi_p_clk? I can't find "peripheral clock" in the specs.
> 
> Yep, it is dsi_p_clk (the APB clock).
> 
>>
>> I think calling it "pclk" in a display driver is very confusing, as
>> pclk, at least for me, always means pixel clock =).
> 
> I can rename it if you prefer. What name would you like to see?
> abp_clk? periph_clk? Something else?

Is there something wrong with dsi_p_clk? If possible, it's nice if the
terms in SW match to the HW docs. In the minimum, the DT doc should give
the mapping from SW to HW terms, at the moment it just says "pclk".

 Tomi