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?
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.
> > +
> > + clk_prepare_enable(csi2rx->p_clk);
> > + reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
> > + clk_disable_unprepare(csi2rx->p_clk);
> > +
> > + csi2rx->max_lanes = (reg & 7);
> > + if (csi2rx->max_lanes > CSI2RX_LANES_MAX) {
> > + dev_err(&pdev->dev, "Invalid number of lanes: %u\n",
> > + csi2rx->max_lanes);
> > + return -EINVAL;
> > + }
> > +
> > + csi2rx->max_streams = ((reg >> 4) & 7);
> > + if (csi2rx->max_streams > CSI2RX_STREAMS_MAX) {
> > + dev_err(&pdev->dev, "Invalid number of streams: %u\n",
> > + csi2rx->max_streams);
> > + return -EINVAL;
> > + }
> > +
> > + csi2rx->has_internal_dphy = (reg & BIT(3)) ? true : false;
> > +
> > + /*
> > + * FIXME: Once we'll have internal D-PHY support, the check
> > + * will need to be removed.
> > + */
> > + if (csi2rx->has_internal_dphy) {
> > + dev_err(&pdev->dev, "Internal D-PHY not supported yet\n");
> > + return -EINVAL;
> > + }
> > +
> > + for (i = 0; i < csi2rx->max_streams; i++) {
> > + char clk_name[16];
> > +
> > + snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
> > + csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
> > + if (IS_ERR(csi2rx->pixel_clk[i])) {
> > + dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
> > + return PTR_ERR(csi2rx->pixel_clk[i]);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> > +{
> > + struct v4l2_fwnode_endpoint v4l2_ep;
> > + struct device_node *ep, *remote;
>
> *remote is now unused.
It's fixed, thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Attachment:
signature.asc
Description: PGP signature
