On Tue, 31 Mar 2020, Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
wrote:
On Tue, 2020-03-31 at 00:31 +0300, Adrian Ratiu wrote:
On Mon, 30 Mar 2020, Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
wrote:
> Hello Fabio, Adrian: On Mon, 2020-03-30 at 08:49 -0300,
> Fabio Estevam wrote:
> > Hi Adrian, On Mon, Mar 30, 2020 at 8:34 AM Adrian Ratiu
> > <adrian.ratiu@xxxxxxxxxxxxx> wrote:
> > > 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 <Ying.Liu@xxxxxxxxxxxxx>
> > > - ARM: dtsi: imx6qdl: Add support for MIPI DSI host
> > > controller
> > > Signed-off-by: Liu Ying <Ying.Liu@xxxxxxxxxxxxx>
> > This one looks like a devicetree patch, but this patch
> > does
> > not touch devicetree.
> > > + 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
