Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1590936 > unrolled thread

Re: [PATCH] omap3isp: add support for CSI1 bus

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] omap3isp: add support for CSI1 bus Pavel Machek <pavel@ucw.cz> - 2017-03-02 10:10 +0100
    [PATCHv2] omap3isp: add support for CSI1 bus Pavel Machek <pavel@ucw.cz> - 2017-03-02 11:20 +0100
      [PATCH] omap3isp: wait for regulators to come up Pavel Machek <pavel@ucw.cz> - 2017-03-02 13:50 +0100
        Re: [PATCH] omap3isp: wait for regulators to come up Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-03-02 16:20 +0100
          Re: [PATCH] omap3isp: wait for regulators to come up Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-04 16:40 +0100
      Re: [PATCHv2] omap3isp: add support for CSI1 bus Pavel Machek <pavel@ucw.cz> - 2017-03-02 13:50 +0100
        Re: [PATCHv2] omap3isp: add support for CSI1 bus Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-03 23:20 +0100
        Re: [PATCHv2] omap3isp: add support for CSI1 bus Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-04 14:10 +0100
          Re: [PATCHv2] omap3isp: add support for CSI1 bus Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-04 16:50 +0100
            Re: [PATCHv2] omap3isp: add support for CSI1 bus Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-03-04 19:50 +0100
          Re: [PATCHv2] omap3isp: add support for CSI1 bus Pavel Machek <pavel@ucw.cz> - 2017-03-05 00:00 +0100
          Re: [PATCHv2] omap3isp: add support for CSI1 bus Pavel Machek <pavel@ucw.cz> - 2017-03-05 15:30 +0100
          [PATCH] v4l2-fwnode: Fix clock lane parsing Pavel Machek <pavel@ucw.cz> - 2017-03-06 08:40 +0100
            Re: [PATCH] v4l2-fwnode: Fix clock lane parsing Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-11 00:00 +0100
          [RFC] omap3isp: add support for CSI1 bus Pavel Machek <pavel@ucw.cz> - 2017-03-06 09:00 +0100
            Re: [RFC] omap3isp: add support for CSI1 bus Pavel Machek <pavel@ucw.cz> - 2017-03-10 14:50 +0100
      Re: [PATCHv2] omap3isp: add support for CSI1 bus Sakari Ailus <sakari.ailus@iki.fi> - 2017-03-02 14:10 +0100

#1590936 — Re: [PATCH] omap3isp: add support for CSI1 bus

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > >> +
> > >> +static int isp_of_parse_node_endpoint(struct device *dev,
> > >> +				      struct device_node *node,
> > >> +				      struct isp_async_subdev *isd)
> > >> +{
> > >> +	struct isp_bus_cfg *buscfg;
> > >> +	struct v4l2_of_endpoint vep;
> > >> +	int ret;
> > >> +
> > >> +	isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
> > > 
> > > Why do you now need to allocate this manually ?
> > 
> > bus is now a pointer.
> 
> I've seen that, but why have you changed it ?

subdev support. Needs to go into separate patch. Will be done shortly.

> > >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > >> @@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
> > >> *ccp2, u8 enable) return ret;
> > >> 
> > >>  	}
> > >> 
> > >> +	if (isp->revision == ISP_REVISION_2_0) {
> > > 
> > > The isp_csiphy.c code checks phy->isp->phy_type for the same purpose,
> > > shouldn't you use that too ?
> > 
> > Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check
> > here? Can do...
> 
> Yes that's what I meant.

Ok, that's something I can do.

But code is still somewhat "interesting". Code in omap3isp_csiphy_acquire()
assumes csi2, and I don't need most of it.. so I'll just not use it,
but it looks strange. I'll post new patch shortly.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1590961 — [PATCHv2] omap3isp: add support for CSI1 bus

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > > >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > > >> @@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
> > > >> *ccp2, u8 enable) return ret;
> > > >> 
> > > >>  	}
> > > >> 
> > > >> +	if (isp->revision == ISP_REVISION_2_0) {
> > > > 
> > > > The isp_csiphy.c code checks phy->isp->phy_type for the same purpose,
> > > > shouldn't you use that too ?
> > > 
> > > Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check
> > > here? Can do...
> > 
> > Yes that's what I meant.
> 
> Ok, that's something I can do.
> 
> But code is still somewhat "interesting". Code in omap3isp_csiphy_acquire()
> assumes csi2, and I don't need most of it.. so I'll just not use it,
> but it looks strange. I'll post new patch shortly.

Ok, how about this one?

									Pavel

omap3isp: add rest of CSI1 support
    
CSI1 needs one more bit to be set up. Do just that.
    
It is not as straightforward as I'd like, see the comments in the code
for explanation.
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

index ca09523..b6e055e 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -21,6 +23,7 @@
 #include <linux/mutex.h>
 #include <linux/uaccess.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
 
 #include "isp.h"
 #include "ispreg.h"
@@ -160,6 +163,22 @@ static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable)
 			return ret;
 	}
 
+	if (isp->phy_type == ISP_PHY_TYPE_3430) {
+		struct media_pad *pad;
+		struct v4l2_subdev *sensor;
+		const struct isp_ccp2_cfg *buscfg;
+
+		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
+		sensor = media_entity_to_v4l2_subdev(pad->entity);
+		/* Struct isp_bus_cfg has union inside */
+		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
+
+		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
+					ISP_INTERFACE_CCP2B_PHY1,
+					enable, !!buscfg->phy_layer,
+					buscfg->strobe_clk_pol);
+	}
+
 	/* Enable/Disable all the LCx channels */
 	for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
 		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LCx_CTRL(i),
@@ -1137,10 +1159,19 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 	if (isp->revision == ISP_REVISION_2_0) {
 		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
 		if (IS_ERR(ccp2->vdds_csib)) {
+			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}
+		/*
+		 * If we set up ccp2->phy here,
+		 * omap3isp_csiphy_acquire() will go ahead and assume
+		 * csi2, dereferencing some null pointers.
+		 *
+		 * ccp2->phy = &isp->isp_csiphy2;
+		 */
 	} else if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
 	}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 871d4fe..897097b 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -68,8 +68,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
 	regmap_write(phy->isp->syscon, phy->isp->syscon_offset, reg);
 }
 
-static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
-				    bool ccp2_strobe)
+void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
+			     bool ccp2_strobe, bool strobe_clk_pol)
 {
 	u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
 		| OMAP343X_CONTROL_CSIRXFE_RESET;
@@ -85,6 +85,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
 
 	if (ccp2_strobe)
 		csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+	
+	if (strobe_clk_pol)
+		csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
 
 	regmap_write(phy->isp->syscon, phy->isp->syscon_offset, csirxfe);
 }
@@ -108,7 +111,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
 	if (phy->isp->phy_type == ISP_PHY_TYPE_3630 && on)
 		return csiphy_routing_cfg_3630(phy, iface, ccp2_strobe);
 	if (phy->isp->phy_type == ISP_PHY_TYPE_3430)
-		return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe);
+		return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe, false);
 }
 
 /*
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.h b/drivers/media/platform/omap3isp/ispcsiphy.h
index 28b63b2..88c5c1e8 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.h
+++ b/drivers/media/platform/omap3isp/ispcsiphy.h
@@ -40,4 +40,7 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
 void omap3isp_csiphy_release(struct isp_csiphy *phy);
 int omap3isp_csiphy_init(struct isp_device *isp);
 
+void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
+				    bool ccp2_strobe, bool strobe_clk_pol);
+
 #endif	/* OMAP3_ISP_CSI_PHY_H */



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1591047 — [PATCH] omap3isp: wait for regulators to come up

[Multipart message — attachments visible in raw view] — view raw

If regulator returns -EPROBE_DEFER, we need to return it too, so that
omap3isp will be re-probed when regulator is ready.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index ca09523..b6e055e 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1137,10 +1159,12 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 	if (isp->revision == ISP_REVISION_2_0) {
 		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
 		if (IS_ERR(ccp2->vdds_csib)) {
+			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}
 	} else if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
 	}

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1591186 — Re: [PATCH] omap3isp: wait for regulators to come up

Hi Pavel,

Thank you for the patch.

On Thursday 02 Mar 2017 13:45:32 Pavel Machek wrote:
> If regulator returns -EPROBE_DEFER, we need to return it too, so that
> omap3isp will be re-probed when regulator is ready.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..b6e055e 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1137,10 +1159,12 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>  	if (isp->revision == ISP_REVISION_2_0) {
>  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
>  		if (IS_ERR(ccp2->vdds_csib)) {
> +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;

This looks good to me, but it will result in the caller printing a "CCP2 
initialization failed" error message, which I'm not sure is right. Maybe we 
should move that message to the omap3isp_ccp2_init() function ?

In any case, this change is fine, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  			dev_dbg(isp->dev,
>  				"Could not get regulator vdds_csib\n");
>  			ccp2->vdds_csib = NULL;
>  		}
>  	} else if (isp->revision == ISP_REVISION_15_0) {
>  		ccp2->phy = &isp->isp_csiphy1;
>  	}

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1592519 — Re: [PATCH] omap3isp: wait for regulators to come up

On Thu, Mar 02, 2017 at 04:46:42PM +0200, Laurent Pinchart wrote:
> Hi Pavel,
> 
> Thank you for the patch.
> 
> On Thursday 02 Mar 2017 13:45:32 Pavel Machek wrote:
> > If regulator returns -EPROBE_DEFER, we need to return it too, so that
> > omap3isp will be re-probed when regulator is ready.
> > 
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..b6e055e 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -1137,10 +1159,12 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> >  	if (isp->revision == ISP_REVISION_2_0) {
> >  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> >  		if (IS_ERR(ccp2->vdds_csib)) {
> > +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> > +				return -EPROBE_DEFER;
> 
> This looks good to me, but it will result in the caller printing a "CCP2 
> initialization failed" error message, which I'm not sure is right. Maybe we 
> should move that message to the omap3isp_ccp2_init() function ?
> 
> In any case, this change is fine, so
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Applied to ccp2 branch with the error message moved. The old message is
still printed if the error code is different.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1591050 — Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > Ok, how about this one?
> > omap3isp: add rest of CSI1 support
> >     
> > CSI1 needs one more bit to be set up. Do just that.
> >     
> > It is not as straightforward as I'd like, see the comments in the code
> > for explanation.
...
> > +	if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > +		struct media_pad *pad;
> > +		struct v4l2_subdev *sensor;
> > +		const struct isp_ccp2_cfg *buscfg;
> > +
> > +		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> > +		sensor = media_entity_to_v4l2_subdev(pad->entity);
> > +		/* Struct isp_bus_cfg has union inside */
> > +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > +
> > +		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > +					ISP_INTERFACE_CCP2B_PHY1,
> > +					enable, !!buscfg->phy_layer,
> > +					buscfg->strobe_clk_pol);
> 
> You should do this through omap3isp_csiphy_acquire(), and not call
> csiphy_routing_cfg_3430() directly from here.

Well, unfortunately omap3isp_csiphy_acquire() does have csi2
assumptions hard-coded :-(.

This will probably fail.

	        rval = omap3isp_csi2_reset(phy->csi2);
	        if (rval < 0)
		                goto done;
				
And this will oops:

static int omap3isp_csiphy_config(struct isp_csiphy *phy)
{
	struct isp_csi2_device *csi2 = phy->csi2;
        struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);
 	struct isp_bus_cfg *buscfg = pipe->external->host_priv;

> > @@ -1137,10 +1159,19 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> >  	if (isp->revision == ISP_REVISION_2_0) {
> >  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> >  		if (IS_ERR(ccp2->vdds_csib)) {
> > +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> > +				return -EPROBE_DEFER;
> 
> This should go to a separate patch.

Ok, easy enough.

> >  			dev_dbg(isp->dev,
> >  				"Could not get regulator vdds_csib\n");
> >  			ccp2->vdds_csib = NULL;
> >  		}
> > +		/*
> > +		 * If we set up ccp2->phy here,
> > +		 * omap3isp_csiphy_acquire() will go ahead and assume
> > +		 * csi2, dereferencing some null pointers.
> > +		 *
> > +		 * ccp2->phy = &isp->isp_csiphy2;
> 
> That needs to be fixed separately.

See analysis above. Yes, it would be nice to fix it. Can you provide
some hints how to do that? Maybe even patch to test? :-).

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1592314 — Re: [PATCHv2] omap3isp: add support for CSI1 bus

Hi Pavel,

On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > Ok, how about this one?
> > > omap3isp: add rest of CSI1 support
> > >     
> > > CSI1 needs one more bit to be set up. Do just that.
> > >     
> > > It is not as straightforward as I'd like, see the comments in the code
> > > for explanation.
> ...
> > > +	if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > > +		struct media_pad *pad;
> > > +		struct v4l2_subdev *sensor;
> > > +		const struct isp_ccp2_cfg *buscfg;
> > > +
> > > +		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> > > +		sensor = media_entity_to_v4l2_subdev(pad->entity);
> > > +		/* Struct isp_bus_cfg has union inside */
> > > +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > > +
> > > +		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > > +					ISP_INTERFACE_CCP2B_PHY1,
> > > +					enable, !!buscfg->phy_layer,
> > > +					buscfg->strobe_clk_pol);
> > 
> > You should do this through omap3isp_csiphy_acquire(), and not call
> > csiphy_routing_cfg_3430() directly from here.
> 
> Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> assumptions hard-coded :-(.
> 
> This will probably fail.
> 
> 	        rval = omap3isp_csi2_reset(phy->csi2);
> 	        if (rval < 0)
> 		                goto done;

Yes. It needs to be fixed. :-)

> 				
> And this will oops:
> 
> static int omap3isp_csiphy_config(struct isp_csiphy *phy)
> {
> 	struct isp_csi2_device *csi2 = phy->csi2;
>         struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);
>  	struct isp_bus_cfg *buscfg = pipe->external->host_priv;

There seems to be some more work left, yes. :-I

> 
> > > @@ -1137,10 +1159,19 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > >  	if (isp->revision == ISP_REVISION_2_0) {
> > >  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> > >  		if (IS_ERR(ccp2->vdds_csib)) {
> > > +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> > > +				return -EPROBE_DEFER;
> > 
> > This should go to a separate patch.
> 
> Ok, easy enough.
> 
> > >  			dev_dbg(isp->dev,
> > >  				"Could not get regulator vdds_csib\n");
> > >  			ccp2->vdds_csib = NULL;
> > >  		}
> > > +		/*
> > > +		 * If we set up ccp2->phy here,
> > > +		 * omap3isp_csiphy_acquire() will go ahead and assume
> > > +		 * csi2, dereferencing some null pointers.
> > > +		 *
> > > +		 * ccp2->phy = &isp->isp_csiphy2;
> > 
> > That needs to be fixed separately.
> 
> See analysis above. Yes, it would be nice to fix it. Can you provide
> some hints how to do that? Maybe even patch to test? :-).

If I only will have the time. Let's see if I can find some time this
week-end.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1592490 — Re: [PATCHv2] omap3isp: add support for CSI1 bus

Hi Pavel,

On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > Ok, how about this one?
> > > omap3isp: add rest of CSI1 support
> > >     
> > > CSI1 needs one more bit to be set up. Do just that.
> > >     
> > > It is not as straightforward as I'd like, see the comments in the code
> > > for explanation.
> ...
> > > +	if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > > +		struct media_pad *pad;
> > > +		struct v4l2_subdev *sensor;
> > > +		const struct isp_ccp2_cfg *buscfg;
> > > +
> > > +		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> > > +		sensor = media_entity_to_v4l2_subdev(pad->entity);
> > > +		/* Struct isp_bus_cfg has union inside */
> > > +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > > +
> > > +		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > > +					ISP_INTERFACE_CCP2B_PHY1,
> > > +					enable, !!buscfg->phy_layer,
> > > +					buscfg->strobe_clk_pol);
> > 
> > You should do this through omap3isp_csiphy_acquire(), and not call
> > csiphy_routing_cfg_3430() directly from here.
> 
> Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> assumptions hard-coded :-(.
> 
> This will probably fail.
> 
> 	        rval = omap3isp_csi2_reset(phy->csi2);
> 	        if (rval < 0)
> 		                goto done;

Could you try to two patches I've applied on the ccp2 branch (I'll remove
them if there are issues).

That's compile tested for now only.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1592524 — Re: [PATCHv2] omap3isp: add support for CSI1 bus

On Sat, Mar 04, 2017 at 03:03:18PM +0200, Sakari Ailus wrote:
> Hi Pavel,
> 
> On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > > Ok, how about this one?
> > > > omap3isp: add rest of CSI1 support
> > > >     
> > > > CSI1 needs one more bit to be set up. Do just that.
> > > >     
> > > > It is not as straightforward as I'd like, see the comments in the code
> > > > for explanation.
> > ...
> > > > +	if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > > > +		struct media_pad *pad;
> > > > +		struct v4l2_subdev *sensor;
> > > > +		const struct isp_ccp2_cfg *buscfg;
> > > > +
> > > > +		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> > > > +		sensor = media_entity_to_v4l2_subdev(pad->entity);
> > > > +		/* Struct isp_bus_cfg has union inside */
> > > > +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > > > +
> > > > +		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > > > +					ISP_INTERFACE_CCP2B_PHY1,
> > > > +					enable, !!buscfg->phy_layer,
> > > > +					buscfg->strobe_clk_pol);
> > > 
> > > You should do this through omap3isp_csiphy_acquire(), and not call
> > > csiphy_routing_cfg_3430() directly from here.
> > 
> > Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> > assumptions hard-coded :-(.
> > 
> > This will probably fail.
> > 
> > 	        rval = omap3isp_csi2_reset(phy->csi2);
> > 	        if (rval < 0)
> > 		                goto done;
> 
> Could you try to two patches I've applied on the ccp2 branch (I'll remove
> them if there are issues).
> 
> That's compile tested for now only.

One more thing. What's needed for configuring the PHY for CCP2?

For instance, is the CSI-2 PHY regulator still needed in
omap3isp_csiphy_acquire()? One way to do this might go to see the original
driver for N900; I don't have the TRM at hand right now.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1592603 — Re: [PATCHv2] omap3isp: add support for CSI1 bus

Hi Sakari,

On Saturday 04 Mar 2017 17:39:46 Sakari Ailus wrote:
> On Sat, Mar 04, 2017 at 03:03:18PM +0200, Sakari Ailus wrote:
> > On Thu, Mar 02, 2017 at 01:38:48PM +0100, Pavel Machek wrote:
> >> 
> >>>> Ok, how about this one?
> >>>> omap3isp: add rest of CSI1 support
> >>>> 
> >>>> CSI1 needs one more bit to be set up. Do just that.
> >>>> 
> >>>> It is not as straightforward as I'd like, see the comments in the
> >>>> code for explanation.
> >>
> >> ...
> >> 
> >>>> +	if (isp->phy_type == ISP_PHY_TYPE_3430) {
> >>>> +		struct media_pad *pad;
> >>>> +		struct v4l2_subdev *sensor;
> >>>> +		const struct isp_ccp2_cfg *buscfg;
> >>>> +
> >>>> +		pad = media_entity_remote_pad(&ccp2
> >>>> ->pads[CCP2_PAD_SINK]);
> >>>> +		sensor = media_entity_to_v4l2_subdev(pad->entity);
> >>>> +		/* Struct isp_bus_cfg has union inside */
> >>>> +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)
> >>>> ->bus.ccp2;
> >>>> +
> >>>> +		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> >>>> +					ISP_INTERFACE_CCP2B_PHY1,
> >>> > +					enable, !!buscfg->phy_layer,
> >>> > +					buscfg->strobe_clk_pol);
> >>> 
> >>> You should do this through omap3isp_csiphy_acquire(), and not call
> >>> csiphy_routing_cfg_3430() directly from here.
> >> 
> >> Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> >> assumptions hard-coded :-(.
> >> 
> >> This will probably fail.
> >> 
> >> 	        rval = omap3isp_csi2_reset(phy->csi2);
> >> 	        if (rval < 0)
> >> 		                goto done;
> > 
> > Could you try to two patches I've applied on the ccp2 branch (I'll remove
> > them if there are issues).
> > 
> > That's compile tested for now only.
> 
> One more thing. What's needed for configuring the PHY for CCP2?
> 
> For instance, is the CSI-2 PHY regulator still needed in
> omap3isp_csiphy_acquire()? One way to do this might go to see the original
> driver for N900; I don't have the TRM at hand right now.

The OMAP34xx TRM and data manual both mention separate VDDS power supplies for 
the CSIb and CSI2 I/O complexes.

vdds_csi2		CSI2 Complex I/O
vdds_csib		CSIb Complex I/O

On OMAP36xx, we instead have

vdda_csiphy1		Input power for camera PHY buffer
vdda_csiphy2		Input power for camera PHY buffer

We need to enable the vds_csib regulator to operate the CSI1/CCP2 PHY, but 
that regulator gets enabled in ispccp2.c as that module is powered by the 
vdds_csib supply on OMAP34xx. However, it won't hurt to do so, and the code 
could be simpler if we manage the regulators the same way on OMAP34xx and 
OMAP36xx.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1592632 — Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > > > +	if (isp->phy_type == ISP_PHY_TYPE_3430) {
> > > > +		struct media_pad *pad;
> > > > +		struct v4l2_subdev *sensor;
> > > > +		const struct isp_ccp2_cfg *buscfg;
> > > > +
> > > > +		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> > > > +		sensor = media_entity_to_v4l2_subdev(pad->entity);
> > > > +		/* Struct isp_bus_cfg has union inside */
> > > > +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > > > +
> > > > +		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> > > > +					ISP_INTERFACE_CCP2B_PHY1,
> > > > +					enable, !!buscfg->phy_layer,
> > > > +					buscfg->strobe_clk_pol);
> > > 
> > > You should do this through omap3isp_csiphy_acquire(), and not call
> > > csiphy_routing_cfg_3430() directly from here.
> > 
> > Well, unfortunately omap3isp_csiphy_acquire() does have csi2
> > assumptions hard-coded :-(.
> > 
> > This will probably fail.
> > 
> > 	        rval = omap3isp_csi2_reset(phy->csi2);
> > 	        if (rval < 0)
> > 		                goto done;
> 
> Could you try to two patches I've applied on the ccp2 branch (I'll remove
> them if there are issues).
> 
> That's compile tested for now only.

Thanks! They seem to be step in right direction. I still need to call
csiphy_routing_cfg_3430() directly for camera to work, but at least it
does not crash if I set up the phy pointer. I'll debug it some more.

Best regards,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1592773 — Re: [PATCHv2] omap3isp: add support for CSI1 bus

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > This will probably fail.
> > 
> > 	        rval = omap3isp_csi2_reset(phy->csi2);
> > 	        if (rval < 0)
> > 		                goto done;
> 
> Could you try to two patches I've applied on the ccp2 branch (I'll remove
> them if there are issues).
> 
> That's compile tested for now only.

They help a lot. Now I can use similar code paths...

Not yet a mergeable patch, but already better than it was.

Thanks and best regards,
									Pavel


diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 24a9fc5..79838bd 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -21,6 +23,7 @@
 #include <linux/mutex.h>
 #include <linux/uaccess.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
 
 #include "isp.h"
 #include "ispreg.h"
@@ -1149,6 +1170,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}
+		ccp2->phy = &isp->isp_csiphy2;
 	} else if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
 	}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..94461df 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -68,8 +68,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
 	regmap_write(phy->isp->syscon, phy->isp->syscon_offset, reg);
 }
 
-static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
-				    bool ccp2_strobe)
+void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
+			     bool ccp2_strobe, bool strobe_clk_pol)
 {
 	u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
 		| OMAP343X_CONTROL_CSIRXFE_RESET;
@@ -85,6 +85,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
 
 	if (ccp2_strobe)
 		csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+	
+	if (strobe_clk_pol)
+		csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
 
 	regmap_write(phy->isp->syscon, phy->isp->syscon_offset, csirxfe);
 }
@@ -108,7 +111,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
 	if (phy->isp->phy_type == ISP_PHY_TYPE_3630 && on)
 		return csiphy_routing_cfg_3630(phy, iface, ccp2_strobe);
 	if (phy->isp->phy_type == ISP_PHY_TYPE_3430)
-		return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe);
+		return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe, false);
 }
 
 /*
@@ -197,27 +200,40 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 	}
 
 	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
-	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
 		lanes = &buscfg->bus.ccp2.lanecfg;
-	else
+		phy->num_data_lanes = 1;
+	} else
 		lanes = &buscfg->bus.csi2.lanecfg;
 
+	printk("lane verification... %d\n", phy->num_data_lanes);
+
 	/* Clock and data lanes verification */
 	for (i = 0; i < phy->num_data_lanes; i++) {
-		if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3)
-			return -EINVAL;
+		if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3) {
+			printk("Bad cfg\n");
+			//return -EINVAL;
+		}
 
-		if (used_lanes & (1 << lanes->data[i].pos))
-			return -EINVAL;
+		if (used_lanes & (1 << lanes->data[i].pos)) {
+			printk("Already used\n");
+			//return -EINVAL;
+		}
 
 		used_lanes |= 1 << lanes->data[i].pos;
 	}
 
-	if (lanes->clk.pol > 1 || lanes->clk.pos > 3)
-		return -EINVAL;
+	printk("used lanes... %d\n", used_lanes);	
 
-	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
-		return -EINVAL;
+	if (lanes->clk.pol > 1 || lanes->clk.pos > 3) {
+		printk("Bad clock\n");
+		//return -EINVAL;
+	}
+
+	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos)) {
+		printk("Reused clock\n");
+		//return -EINVAL;
+	}
 
 	/*
 	 * The PHY configuration is lost in off mode, that's not an
@@ -302,13 +318,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
 	if (rval < 0)
 		goto done;
 
-	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
-	if (rval) {
-		regulator_disable(phy->vdd);
-		goto done;
+	if (phy->isp->revision == ISP_REVISION_15_0) {
+		rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
+		if (rval) {
+			regulator_disable(phy->vdd);
+			goto done;
+		}
+		
+		csiphy_power_autoswitch_enable(phy, true);		
 	}
 
-	csiphy_power_autoswitch_enable(phy, true);
 	phy->phy_in_use = 1;
 
 done:

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1592973 — [PATCH] v4l2-fwnode: Fix clock lane parsing

[Multipart message — attachments visible in raw view] — view raw

Fix clock lane parsing in v4l2-fwnode.
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index d6666d3..44036b8 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -167,7 +167,7 @@ void v4l2_fwnode_endpoint_parse_csi1_bus(struct fwnode_handle *fwn,
                bus->data_lane = v;
 
        if (!fwnode_property_read_u32(fwn, "clock-lanes", &v))
-               bus->data_lane = v;
+               bus->clock_lane = v;
 
        if (bus_type == V4L2_FWNODE_BUS_TYPE_CCP2)
 	       vfwn->bus_type = V4L2_MBUS_CCP2;

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1598156 — Re: [PATCH] v4l2-fwnode: Fix clock lane parsing

On Mon, Mar 06, 2017 at 08:23:24AM +0100, Pavel Machek wrote:
> Fix clock lane parsing in v4l2-fwnode.
>     
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index d6666d3..44036b8 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -167,7 +167,7 @@ void v4l2_fwnode_endpoint_parse_csi1_bus(struct fwnode_handle *fwn,
>                 bus->data_lane = v;
>  
>         if (!fwnode_property_read_u32(fwn, "clock-lanes", &v))
> -               bus->data_lane = v;
> +               bus->clock_lane = v;

Oh my. Did I really write it like that?

I'll merge your fix next Monday. Thanks!

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1592979 — [RFC] omap3isp: add support for CSI1 bus

[Multipart message — attachments visible in raw view] — view raw

omap3isp: add rest of CSI1 support

CSI1 needs one more bit to be set up. Do just that.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

Hmm. Looking at that... num_data_lanes probably should be modified in
local variable, not globally like this. Should I do that?

Anything else that needs fixing?

index 24a9fc5..6feba36 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -21,6 +23,7 @@
 #include <linux/mutex.h>
 #include <linux/uaccess.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
 
 #include "isp.h"
 #include "ispreg.h"
@@ -1149,6 +1152,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
 		}
+		ccp2->phy = &isp->isp_csiphy2;
 	} else if (isp->revision == ISP_REVISION_15_0) {
 		ccp2->phy = &isp->isp_csiphy1;
 	}
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 50c0f64..cd6351b 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -197,9 +200,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 	}
 
 	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
-	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
+	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
 		lanes = &buscfg->bus.ccp2.lanecfg;
-	else
+		phy->num_data_lanes = 1;
+	} else
 		lanes = &buscfg->bus.csi2.lanecfg;
 
 	/* Clock and data lanes verification */
@@ -302,13 +306,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
 	if (rval < 0)
 		goto done;
 
-	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
-	if (rval) {
-		regulator_disable(phy->vdd);
-		goto done;
+	if (phy->isp->revision == ISP_REVISION_15_0) {
+		rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
+		if (rval) {
+			regulator_disable(phy->vdd);
+			goto done;
+		}
+		
+		csiphy_power_autoswitch_enable(phy, true);		
 	}
 
-	csiphy_power_autoswitch_enable(phy, true);
 	phy->phy_in_use = 1;
 
 done:

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1597654 — Re: [RFC] omap3isp: add support for CSI1 bus

[Multipart message — attachments visible in raw view] — view raw

On Mon 2017-03-06 08:56:59, Pavel Machek wrote:
> omap3isp: add rest of CSI1 support
> 
> CSI1 needs one more bit to be set up. Do just that.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> ---
> 
> Hmm. Looking at that... num_data_lanes probably should be modified in
> local variable, not globally like this. Should I do that?
> 
> Anything else that needs fixing?

Ping? Feedback here would be nice. This is last "interesting" piece of
the hardware support...

Best regards,
								Pavel

> index 24a9fc5..6feba36 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -21,6 +23,7 @@
>  #include <linux/mutex.h>
>  #include <linux/uaccess.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
>  
>  #include "isp.h"
>  #include "ispreg.h"
> @@ -1149,6 +1152,7 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>  				"Could not get regulator vdds_csib\n");
>  			ccp2->vdds_csib = NULL;
>  		}
> +		ccp2->phy = &isp->isp_csiphy2;
>  	} else if (isp->revision == ISP_REVISION_15_0) {
>  		ccp2->phy = &isp->isp_csiphy1;
>  	}
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 50c0f64..cd6351b 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -197,9 +200,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
>  	}
>  
>  	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
> -	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
> +	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
>  		lanes = &buscfg->bus.ccp2.lanecfg;
> -	else
> +		phy->num_data_lanes = 1;
> +	} else
>  		lanes = &buscfg->bus.csi2.lanecfg;
>  
>  	/* Clock and data lanes verification */
> @@ -302,13 +306,16 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
>  	if (rval < 0)
>  		goto done;
>  
> -	rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> -	if (rval) {
> -		regulator_disable(phy->vdd);
> -		goto done;
> +	if (phy->isp->revision == ISP_REVISION_15_0) {
> +		rval = csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_ON);
> +		if (rval) {
> +			regulator_disable(phy->vdd);
> +			goto done;
> +		}
> +		
> +		csiphy_power_autoswitch_enable(phy, true);		
>  	}
>  
> -	csiphy_power_autoswitch_enable(phy, true);
>  	phy->phy_in_use = 1;
>  
>  done:
> 



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1591057 — Re: [PATCHv2] omap3isp: add support for CSI1 bus

Hi Pavel,

On Thu, Mar 02, 2017 at 11:16:04AM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > > > >> @@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
> > > > >> *ccp2, u8 enable) return ret;
> > > > >> 
> > > > >>  	}
> > > > >> 
> > > > >> +	if (isp->revision == ISP_REVISION_2_0) {
> > > > > 
> > > > > The isp_csiphy.c code checks phy->isp->phy_type for the same purpose,
> > > > > shouldn't you use that too ?
> > > > 
> > > > Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check
> > > > here? Can do...
> > > 
> > > Yes that's what I meant.
> > 
> > Ok, that's something I can do.
> > 
> > But code is still somewhat "interesting". Code in omap3isp_csiphy_acquire()
> > assumes csi2, and I don't need most of it.. so I'll just not use it,
> > but it looks strange. I'll post new patch shortly.
> 
> Ok, how about this one?
> 
> 									Pavel
> 
> omap3isp: add rest of CSI1 support
>     
> CSI1 needs one more bit to be set up. Do just that.
>     
> It is not as straightforward as I'd like, see the comments in the code
> for explanation.
>     
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> index ca09523..b6e055e 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -21,6 +23,7 @@
>  #include <linux/mutex.h>
>  #include <linux/uaccess.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
>  
>  #include "isp.h"
>  #include "ispreg.h"
> @@ -160,6 +163,22 @@ static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable)
>  			return ret;
>  	}
>  
> +	if (isp->phy_type == ISP_PHY_TYPE_3430) {
> +		struct media_pad *pad;
> +		struct v4l2_subdev *sensor;
> +		const struct isp_ccp2_cfg *buscfg;
> +
> +		pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> +		sensor = media_entity_to_v4l2_subdev(pad->entity);
> +		/* Struct isp_bus_cfg has union inside */
> +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> +
> +		csiphy_routing_cfg_3430(&isp->isp_csiphy2,
> +					ISP_INTERFACE_CCP2B_PHY1,
> +					enable, !!buscfg->phy_layer,
> +					buscfg->strobe_clk_pol);

You should do this through omap3isp_csiphy_acquire(), and not call
csiphy_routing_cfg_3430() directly from here.


> +	}
> +
>  	/* Enable/Disable all the LCx channels */
>  	for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
>  		isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LCx_CTRL(i),
> @@ -1137,10 +1159,19 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>  	if (isp->revision == ISP_REVISION_2_0) {
>  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
>  		if (IS_ERR(ccp2->vdds_csib)) {
> +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;

This should go to a separate patch.

>  			dev_dbg(isp->dev,
>  				"Could not get regulator vdds_csib\n");
>  			ccp2->vdds_csib = NULL;
>  		}
> +		/*
> +		 * If we set up ccp2->phy here,
> +		 * omap3isp_csiphy_acquire() will go ahead and assume
> +		 * csi2, dereferencing some null pointers.
> +		 *
> +		 * ccp2->phy = &isp->isp_csiphy2;

That needs to be fixed separately.

> +		 */
>  	} else if (isp->revision == ISP_REVISION_15_0) {
>  		ccp2->phy = &isp->isp_csiphy1;
>  	}
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> index 871d4fe..897097b 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -68,8 +68,8 @@ static void csiphy_routing_cfg_3630(struct isp_csiphy *phy,
>  	regmap_write(phy->isp->syscon, phy->isp->syscon_offset, reg);
>  }
>  
> -static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
> -				    bool ccp2_strobe)
> +void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
> +			     bool ccp2_strobe, bool strobe_clk_pol)
>  {
>  	u32 csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ
>  		| OMAP343X_CONTROL_CSIRXFE_RESET;
> @@ -85,6 +85,9 @@ static void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
>  
>  	if (ccp2_strobe)
>  		csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> +	
> +	if (strobe_clk_pol)
> +		csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
>  
>  	regmap_write(phy->isp->syscon, phy->isp->syscon_offset, csirxfe);
>  }
> @@ -108,7 +111,7 @@ static void csiphy_routing_cfg(struct isp_csiphy *phy,
>  	if (phy->isp->phy_type == ISP_PHY_TYPE_3630 && on)
>  		return csiphy_routing_cfg_3630(phy, iface, ccp2_strobe);
>  	if (phy->isp->phy_type == ISP_PHY_TYPE_3430)
> -		return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe);
> +		return csiphy_routing_cfg_3430(phy, iface, on, ccp2_strobe, false);
>  }
>  
>  /*
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.h b/drivers/media/platform/omap3isp/ispcsiphy.h
> index 28b63b2..88c5c1e8 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.h
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.h
> @@ -40,4 +40,7 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
>  void omap3isp_csiphy_release(struct isp_csiphy *phy);
>  int omap3isp_csiphy_init(struct isp_device *isp);
>  
> +void csiphy_routing_cfg_3430(struct isp_csiphy *phy, u32 iface, bool on,
> +				    bool ccp2_strobe, bool strobe_clk_pol);
> +
>  #endif	/* OMAP3_ISP_CSI_PHY_H */
> 
> 
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web