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

[Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

On Tue, 19 Sep 2017 17:25:29 +0300
Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:

> 
> 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-l0cyMroinI0@public.gmane.org> 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-l0cyMroinI0@public.gmane.org> 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-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>      
> >>>>
> >>>> <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".

I'll switch to dsi_p_clk and dsi_sys_clk.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html