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

From: Pavel Machek <pavel@ucw.cz>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	mchehab@kernel.org, kernel list <linux-kernel@vger.kernel.org>,
	ivo.g.dimitrov.75@gmail.com, sre@kernel.org,
	pali.rohar@gmail.com, linux-media@vger.kernel.org
Subject: Re: [PATCHv2] omap3isp: add support for CSI1 bus
Date: Thu, 2 Mar 2017 13:38:48 +0100	[thread overview]
Message-ID: <20170302123848.GA28230@amd> (raw)
In-Reply-To: <20170302112401.GF3220@valkosipuli.retiisi.org.uk>

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]