From mboxrd@z Thu Jan 1 00:00:00 1970 From: Benoit Parrot Subject: Re: [PATCH v4 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver Date: Wed, 11 Oct 2017 08:22:59 -0500 Message-ID: <20171011132258.GB25400@ti.com> References: <20170922100823.18184-1-maxime.ripard@free-electrons.com> <20170922100823.18184-3-maxime.ripard@free-electrons.com> <20170929172709.GA3163@ti.com> <20171011092409.ndtr3fdo2oj3zueb@flea.lan> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: Content-Disposition: inline In-Reply-To: <20171011092409.ndtr3fdo2oj3zueb@flea.lan> Sender: linux-media-owner@vger.kernel.org To: Maxime Ripard Cc: Mauro Carvalho Chehab , Mark Rutland , Rob Herring , Laurent Pinchart , linux-media@vger.kernel.org, devicetree@vger.kernel.org, Cyprian Wronka , Richard Sproul , Alan Douglas , Steve Creaney , Thomas Petazzoni , Boris Brezillon , Niklas =?iso-8859-1?Q?S=F6derlund?= , Hans Verkuil , Sakari Ailus , nm@ti.com List-Id: devicetree@vger.kernel.org Hi Maxime, Maxime Ripard wrote on Wed [2017-Oct-11 11:24:09 +0200]: > Hi Benoit, > > On Fri, Sep 29, 2017 at 05:27:09PM +0000, Benoit Parrot wrote: > > > +static int csi2rx_get_resources(struct csi2rx_priv *csi2rx, > > > + struct platform_device *pdev) > > > +{ > > > + struct resource *res; > > > + unsigned char i; > > > + u32 reg; > > > + > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + csi2rx->base = devm_ioremap_resource(&pdev->dev, res); > > > + if (IS_ERR(csi2rx->base)) > > > + return PTR_ERR(csi2rx->base); > > > + > > > + csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk"); > > > + if (IS_ERR(csi2rx->sys_clk)) { > > > + dev_err(&pdev->dev, "Couldn't get sys clock\n"); > > > + return PTR_ERR(csi2rx->sys_clk); > > > + } > > > + > > > + csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk"); > > > + if (IS_ERR(csi2rx->p_clk)) { > > > + dev_err(&pdev->dev, "Couldn't get P clock\n"); > > > + return PTR_ERR(csi2rx->p_clk); > > > + } > > > + > > > + csi2rx->dphy = devm_phy_optional_get(&pdev->dev, "dphy"); > > > + if (IS_ERR(csi2rx->dphy)) { > > > + dev_err(&pdev->dev, "Couldn't get external D-PHY\n"); > > > + return PTR_ERR(csi2rx->dphy); > > > + } > > > + > > > + /* > > > + * FIXME: Once we'll have external D-PHY support, the check > > > + * will need to be removed. > > > + */ > > > + if (csi2rx->dphy) { > > > + dev_err(&pdev->dev, "External D-PHY not supported yet\n"); > > > + return -EINVAL; > > > + } > > > > I understand that in your current environment you do not have a > > DPHY. But I am wondering in a real setup where you will have either > > an internal or an external DPHY, how are they going to interact with > > this driver or vice-versa? > > It's difficult to give an answer with so little details. How would you > choose between those two PHYs? Is there a mux, or should we just power > one of the two? If that's the case, is there any use case were we > might want to power both? If not, which one should we favor, in which > situations? Oops, I guess I should clarify, in this case I did not mean we would have both an internal and an external DPHY. I just meant one or the other. Basically just want to see how you would actually handle a DPHY here whether it's internal or external? For instance, using direct register access from within this driver or make use of an separate phy driver... > > I guess all those questions actually depend on the way the integration > has been done, and we're not quite there yet. I guess we could do > either a platform specific structure or a glue, depending on the > complexity. The platform specific compatible will allow us to do that > as we see fit anyway. > Regards, Benoit