diff mbox

[PATCHv2] omap3isp: add support for CSI1 bus

Message ID 20170302101603.GE27818@amd (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Machek March 2, 2017, 10:16 a.m. UTC 
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

Comments

Sakari Ailus March 2, 2017, 11:24 a.m. UTC | #1 
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 */
> 
> 
>
Pavel Machek March 2, 2017, 12:38 p.m. UTC | #2 
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
Sakari Ailus March 3, 2017, 10:17 p.m. UTC | #3 
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.
Sakari Ailus March 4, 2017, 1:03 p.m. UTC | #4 
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.
Sakari Ailus March 4, 2017, 3:39 p.m. UTC | #5 
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.
Laurent Pinchart March 4, 2017, 6:44 p.m. UTC | #6 
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.
Pavel Machek March 4, 2017, 10:53 p.m. UTC | #7 
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
Sakari Ailus May 3, 2017, 7:43 a.m. UTC | #8 
Hi Pavel,

On 03/04/17 15:03, 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.
> 

I've updated the CCP2 patches here on top of the latest fwnode patches:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>

No even compile testing this time though. I'm afraid I haven't had the
time to otherwise to work on the CCP2 support, so there are no other
changes besides the rebase.

I intend to send a pull request for the fwnode patches once we have the
next rc1 in media tree so then we can have the patches on plain media
tree master branch.
Pavel Machek May 3, 2017, 7:50 p.m. UTC | #9 
Hi!

> > 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.
> > 
> 
> I've updated the CCP2 patches here on top of the latest fwnode patches:
> 
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>
> 
> No even compile testing this time though. I'm afraid I haven't had the
> time to otherwise to work on the CCP2 support, so there are no other
> changes besides the rebase.

It seems they don't compile. Hmmm. Did I do something wrong? "struct
fwnode_endpoint" seems to be only used in v4l2-fwnode.h; that can't be right...?

  CC      drivers/media/i2c/smiapp/smiapp-core.o
  In file included from drivers/media/i2c/smiapp/smiapp-core.c:35:0:
  ./include/media/v4l2-fwnode.h:83:25: error: field 'base' has
  incomplete type
  drivers/media/i2c/smiapp/smiapp-core.c: In function
  'smiapp_get_hwconfig':
  drivers/media/i2c/smiapp/smiapp-core.c:2790:9: error: implicit
  declaration of function 'dev_fwnode'
  [-Werror=implicit-function-declaration]
  drivers/media/i2c/smiapp/smiapp-core.c:2790:33: warning:
  initialization makes pointer from integer without a cast [enabled by
  default]
  drivers/media/i2c/smiapp/smiapp-core.c:2797:2: error: implicit
  declaration of function 'fwnode_graph_get_next_endpoint'
  [-Werror=implicit-function-declaration]

Best regards,
									Pavel
diff mbox

Patch

--- 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 */