On Tue, 31 Mar 2020, Ezequiel Garcia [off-list ref] 
wrote:

On Tue, 2020-03-31 at 00:31 +0300, Adrian Ratiu wrote: 

quoted

On Mon, 30 Mar 2020, Ezequiel Garcia [off-list ref] 
wrote: 

quoted

Hello Fabio, Adrian:   On Mon, 2020-03-30 at 08:49 -0300, 
Fabio Estevam wrote:  

quoted

Hi Adrian,  On Mon, Mar 30, 2020 at 8:34 AM Adrian Ratiu 
[off-list ref] wrote:  

quoted

This adds support for the Synopsis DesignWare MIPI DSI 
v1.01  host controller which is embedded in i.MX 6 SoCs. 
Based on  following patches, but updated/extended to work 
with existing  support found in the kernel:  - drm: imx: 
Support Synopsys  DesignWare MIPI DSI host controller  
  Signed-off-by: Liu Ying [off-list ref]  
 - ARM: dtsi: imx6qdl: Add support for MIPI DSI host  
controller  
  Signed-off-by: Liu Ying [off-list ref]  

 This one looks like a devicetree patch, but this patch 
 does  
not touch devicetree.   

quoted

+       ret = clk_prepare_enable(dsi->pllref_clk); + 
if  (ret) { +               dev_err(dev, "%s: Failed to 
enable  pllref_clk\n", __func__); +               return 
ret; +  } + +       dsi->mux_sel = 
syscon_regmap_lookup_by_phandle(dev->of_node, "fsl,gpr"); 
+  if (IS_ERR(dsi->mux_sel)) { +               ret = 
PTR_ERR(dsi->mux_sel); +               dev_err(dev, "%s: 
Failed to get GPR regmap: %d\n", +  __func__, ret); + 
return ret;  

 You should disable the dsi->pllref_clk clock prior to  
returning the error.   

 Another approach could be moving the clock on and off to to 
component_ops.{bind,unbind} (as rockhip driver does).    What 
exactly is the PLL clock needed for? Would it make sense to 
move it some of the PHY power on/off? (Maybe not, but it's 
worthing checking).    Also, it seems the other IP blocks 
have this PLL clock, so maybe  it could be moved to the 
dw_mipi_dsi core? This could be  something for a follow-up, 
to avoid creeping this series. 

 Hi Ezequiel,  pll is the video reference clock which drives 
the data lanes and  yes all drivers have it as it's a basic 
requirement, so moving it  to the common bridge is indeed a 
good idea, however this kind of  driver refactoring is out of 
scope for this specific patch series,  because, for now, I'd 
like to get the regmap and the imx6 driver  in, once that is 
done we can think how to further abstract away  common logic 
and slim down the existing drivers further.   Basically I just 
want to avoid feature creep as I expect v6 to be  ~ 8 patches 
big and the series is already over 1200 lines.  

Oh, absolutely: if there's one thing I try to avoid is feature 
creep -- together with bikeshedding! 

Do note however, that you could move the PLL clock handling to 
component_ops.{bind,unbind} and maybe simplify the error 
handling. 

(BTW, great work!)

Thanks! I'll do the bind/unbind move for the new imx6 driver which 
I'm
adding in this series to make it resemble the existing rockchip 
driver a bit more, then I'll stop short of further driver 
refactorings.

Cheers,
Ezequiel