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

Groups > linux.kernel > #1581164 > unrolled thread

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

Started byPavel Machek <pavel@ucw.cz>
First post2017-02-15 10:50 +0100
Last post2017-02-21 12:30 +0100
Articles 4 — 3 participants

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-02-15 10:50 +0100
    Re: [PATCH] omap3isp: add support for CSI1 bus Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-02-20 02:00 +0100
      [PATCH] omap3isp: avoid uninitialized memory Pavel Machek <pavel@ucw.cz> - 2017-02-20 13:10 +0100
        Re: [PATCH] omap3isp: avoid uninitialized memory Sakari Ailus <sakari.ailus@iki.fi> - 2017-02-21 12:30 +0100

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

FromPavel Machek <pavel@ucw.cz>
Date2017-02-15 10:50 +0100
SubjectRe: [PATCH] omap3isp: add support for CSI1 bus
Message-ID<tb4nM-6Eg-1@gated-at.bofh.it>

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

Hi!

> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2024,21 +2024,92 @@ enum isp_of_phy {
> >  	ISP_OF_PHY_CSIPHY2,
> >  };
> > 
> > -static int isp_of_parse_node(struct device *dev, struct device_node *node,
> > -			     struct isp_async_subdev *isd)
> > +void __isp_of_parse_node_csi1(struct device *dev,
> > +				   struct isp_ccp2_cfg *buscfg,
> > +				   struct v4l2_of_endpoint *vep)
> 
> This function isn't use anywhere else, you can merge it with 
> isp_of_parse_node_csi1().

I'd prefer not to. First, it will be used separately in future, and
second, expresions would be uglier.

> > +{
> > +	buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
> > +	buscfg->lanecfg.clk.pol =
> > +		vep->bus.mipi_csi1.lane_polarity[0];
> > +	dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> > +		buscfg->lanecfg.clk.pol,
> > +		buscfg->lanecfg.clk.pos);
> > +
> > +	buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
> > +	buscfg->lanecfg.data[0].pol =
> > +		vep->bus.mipi_csi2.lane_polarities[1];
> 
> bus.mipi_csi2 ?

Good catch. Fixed.

> > -	ret = v4l2_of_parse_endpoint(node, &vep);
> > -	if (ret)
> > -		return ret;
> > +	if (vep->base.port == ISP_OF_PHY_CSIPHY1)
> > +		buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> > +	else
> > +		buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> 
> I would keep this code in the caller to avoid code duplication with 
> isp_of_parse_node_csi1().

Take a closer look. Code in _csi1 is different.

> >  		break;
> > 
> >  	default:
> > +		return -1;
> 
> Please use the appropriate error code.

Ok.

> > +	return 0;
> > +}
> > +
> > +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.

> > +	dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> > +		vep.base.port);
> > +
> > +	if (isp_endpoint_to_buscfg(dev, vep, buscfg))
> 
> I'm fine splitting the CSI1/CSI2 parsing code to separate functions, but I 
> don't think there's a need to split isp_endpoint_to_buscfg(). You can keep 
> that part inline.

I'd prefer smaller functions here. I tried to read the original and it
was not too easy.

> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ 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...

> > +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > +
> > +
> 
> One blank line is enough.

Ok.

> > +		if (enable) {
> > +			csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> > +				  OMAP343X_CONTROL_CSIRXFE_RESET;
> > +
> > +			if (buscfg->phy_layer)
> > +				csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> > +
> > +			if (buscfg->strobe_clk_pol)
> > +				csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
> > +		} else
> > +			csirxfe = 0;
> 
> You need curly braces for the else statement too.

Easy enough.

> > +
> > +		regmap_write(isp->syscon, isp->syscon_offset, csirxfe);
> 
> Isn't this already configured by csiphy_routing_cfg_3430(), called through 
> omap3isp_csiphy_acquire() ? You'll need to add support for the strobe/clock 
> polarity there, but the rest should already be handled.

Let me check...

> > @@ -69,11 +69,15 @@
> >   * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
> >   * @V4L2_MBUS_BT656:	parallel interface with embedded synchronisation, can
> >   *			also be used for BT.1120
> > + * @V4L2_MBUS_CSI1:	MIPI CSI-1 serial interface
> > + * @V4L2_MBUS_CCP2:	CCP2 (Compact Camera Port 2)
> 
> It would help if you could provide, in comments or in the commit message, a 
> few pointers to information about CSI-1 and CCP2.

There's not much good information :-(.

http://electronics.stackexchange.com/questions/134395/differences-between-mipi-csi1-and-mipi-csi2
> 

> >  /**
> > + * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
> > + * @clock_inv: polarity of clock/strobe signal
> > + *	       false - not inverted, true - inverted
> > + * @strobe: false - data/clock, true - data/strobe
> > + * @data_lane: the number of the data lane
> > + * @clock_lane: the number of the clock lane
> > + */
> > +struct v4l2_of_bus_mipi_csi1 {
> > +	bool clock_inv;
> > +	bool strobe;
> > +	bool lane_polarity[2];
> 
> This field isn't documented.

Yep, automatic checker already told me. Plus, similar field elsewhere
is called "lane_polarities" but I believe "polarity" is a better name.

									Pavel

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

[toc] | [next] | [standalone]

#1584295

FromLaurent Pinchart <laurent.pinchart@ideasonboard.com>
Date2017-02-20 02:00 +0100
Message-ID<tcKuB-5Yh-3@gated-at.bofh.it>
In reply to#1581164
Hi Pavel,

On Wednesday 15 Feb 2017 10:42:29 Pavel Machek wrote:
> Hi!
> 
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -2024,21 +2024,92 @@ enum isp_of_phy {
> >>  	ISP_OF_PHY_CSIPHY2,
> >>  };
> >> 
> >> -static int isp_of_parse_node(struct device *dev, struct device_node
> >> *node,
> >> -			     struct isp_async_subdev *isd)
> >> +void __isp_of_parse_node_csi1(struct device *dev,
> >> +				   struct isp_ccp2_cfg *buscfg,
> >> +				   struct v4l2_of_endpoint *vep)
> > 
> > This function isn't use anywhere else, you can merge it with
> > isp_of_parse_node_csi1().
> 
> I'd prefer not to. First, it will be used separately in future, and
> second, expresions would be uglier.

Where will it be used ? As for the uglier part, I don't agree, otherwise I 
wouldn't have proposed it.

> >> +{
> >> +	buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
> >> +	buscfg->lanecfg.clk.pol =
> >> +		vep->bus.mipi_csi1.lane_polarity[0];
> >> +	dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> >> +		buscfg->lanecfg.clk.pol,
> >> +		buscfg->lanecfg.clk.pos);
> >> +
> >> +	buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
> >> +	buscfg->lanecfg.data[0].pol =
> >> +		vep->bus.mipi_csi2.lane_polarities[1];
> > 
> > bus.mipi_csi2 ?
> 
> Good catch. Fixed.
> 
> >> -	ret = v4l2_of_parse_endpoint(node, &vep);
> >> -	if (ret)
> >> -		return ret;
> >> +	if (vep->base.port == ISP_OF_PHY_CSIPHY1)
> >> +		buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> >> +	else
> >> +		buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> > 
> > I would keep this code in the caller to avoid code duplication with
> > isp_of_parse_node_csi1().
> 
> Take a closer look. Code in _csi1 is different.
>
> >>  		break;
> >>  	
> >>  	default:
> >> +		return -1;
> > 
> > Please use the appropriate error code.
> 
> Ok.
> 
> >> +	return 0;
> >> +}
> >> +
> >> +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 ?

> >> +	dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> >> +		vep.base.port);
> >> +
> >> +	if (isp_endpoint_to_buscfg(dev, vep, buscfg))
> > 
> > I'm fine splitting the CSI1/CSI2 parsing code to separate functions, but I
> > don't think there's a need to split isp_endpoint_to_buscfg(). You can keep
> > that part inline.
> 
> I'd prefer smaller functions here. I tried to read the original and it
> was not too easy.

This function became a kzalloc (which I still don't see why you need it), a 
call to v4l2_of_parse_endpoint(), and then isp_endpoint_to_buscfg(). That's 
too small to be a function of its own. Please merge 
isp_of_parse_node_endpoint() and isp_endpoint_to_buscfg().

> >> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> >> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a
> >> 100644
> >> --- a/drivers/media/platform/omap3isp/ispccp2.c
> >> +++ 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.

> >> +		buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> >> +
> >> +
> > 
> > One blank line is enough.
> 
> Ok.
> 
> >> +		if (enable) {
> >> +			csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> >> +				  OMAP343X_CONTROL_CSIRXFE_RESET;
> >> +
> >> +			if (buscfg->phy_layer)
> >> +				csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> >> +
> >> +			if (buscfg->strobe_clk_pol)
> >> +				csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
> >> +		} else
> >> +			csirxfe = 0;
> > 
> > You need curly braces for the else statement too.
> 
> Easy enough.
> 
> >> +
> >> +		regmap_write(isp->syscon, isp->syscon_offset, csirxfe);
> > 
> > Isn't this already configured by csiphy_routing_cfg_3430(), called through
> > omap3isp_csiphy_acquire() ? You'll need to add support for the
> > strobe/clock polarity there, but the rest should already be handled.
> 
> Let me check...
> 
> >> @@ -69,11 +69,15 @@
> >>   * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
> >>   * @V4L2_MBUS_BT656:	parallel interface with embedded
> >> synchronisation, can
> >>   *			also be used for BT.1120
> > > + * @V4L2_MBUS_CSI1:	MIPI CSI-1 serial interface
> > > + * @V4L2_MBUS_CCP2:	CCP2 (Compact Camera Port 2)
> > 
> > It would help if you could provide, in comments or in the commit message,
> > a few pointers to information about CSI-1 and CCP2.
> 
> There's not much good information :-(.
> 
> http://electronics.stackexchange.com/questions/134395/differences-between-mi
> pi-csi1-and-mipi-csi2

> >>  /**
> >> 
> >> + * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
> >> + * @clock_inv: polarity of clock/strobe signal
> >> + *	       false - not inverted, true - inverted
> >> + * @strobe: false - data/clock, true - data/strobe
> >> + * @data_lane: the number of the data lane
> >> + * @clock_lane: the number of the clock lane
> >> + */
> >> +struct v4l2_of_bus_mipi_csi1 {
> >> +	bool clock_inv;
> >> +	bool strobe;
> >> +	bool lane_polarity[2];
> > 
> > This field isn't documented.
> 
> Yep, automatic checker already told me. Plus, similar field elsewhere
> is called "lane_polarities" but I believe "polarity" is a better name.

-- 
Regards,

Laurent Pinchart

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

#1584563 — [PATCH] omap3isp: avoid uninitialized memory

FromPavel Machek <pavel@ucw.cz>
Date2017-02-20 13:10 +0100
Subject[PATCH] omap3isp: avoid uninitialized memory
Message-ID<tcUWZ-4uF-9@gated-at.bofh.it>
In reply to#1584295

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

Code in ispcsiphy is quite confusing, and does not initialize phy1 in
case of isp rev. 2. Set it to zero, to prevent confusion.

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

index 8f73f6d..a2474b6 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -362,14 +374,16 @@ int omap3isp_csiphy_init(struct isp_device *isp)
 	phy2->phy_regs = OMAP3_ISP_IOMEM_CSIPHY2;
 	mutex_init(&phy2->mutex);
 
-	if (isp->revision == ISP_REVISION_15_0) {
-		phy1->isp = isp;
-		phy1->csi2 = &isp->isp_csi2c;
-		phy1->num_data_lanes = ISP_CSIPHY1_NUM_DATA_LANES;
-		phy1->cfg_regs = OMAP3_ISP_IOMEM_CSI2C_REGS1;
-		phy1->phy_regs = OMAP3_ISP_IOMEM_CSIPHY1;
-		mutex_init(&phy1->mutex);
+	if (isp->revision != ISP_REVISION_15_0) {
+		memset(phy1, sizeof(*phy1), 0);
+		return 0;
 	}
 
+	phy1->isp = isp;
+	phy1->csi2 = &isp->isp_csi2c;
+	phy1->num_data_lanes = ISP_CSIPHY1_NUM_DATA_LANES;
+	phy1->cfg_regs = OMAP3_ISP_IOMEM_CSI2C_REGS1;
+	phy1->phy_regs = OMAP3_ISP_IOMEM_CSIPHY1;
+	mutex_init(&phy1->mutex);
 	return 0;
 }


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

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

#1585234 — Re: [PATCH] omap3isp: avoid uninitialized memory

FromSakari Ailus <sakari.ailus@iki.fi>
Date2017-02-21 12:30 +0100
SubjectRe: [PATCH] omap3isp: avoid uninitialized memory
Message-ID<tdgNQ-1UC-7@gated-at.bofh.it>
In reply to#1584563
Hi Pavel,

On Mon, Feb 20, 2017 at 01:06:47PM +0100, Pavel Machek wrote:
> 
> Code in ispcsiphy is quite confusing, and does not initialize phy1 in
> case of isp rev. 2. Set it to zero, to prevent confusion.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> index 8f73f6d..a2474b6 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -362,14 +374,16 @@ int omap3isp_csiphy_init(struct isp_device *isp)
>  	phy2->phy_regs = OMAP3_ISP_IOMEM_CSIPHY2;
>  	mutex_init(&phy2->mutex);
>  
> -	if (isp->revision == ISP_REVISION_15_0) {
> -		phy1->isp = isp;
> -		phy1->csi2 = &isp->isp_csi2c;
> -		phy1->num_data_lanes = ISP_CSIPHY1_NUM_DATA_LANES;
> -		phy1->cfg_regs = OMAP3_ISP_IOMEM_CSI2C_REGS1;
> -		phy1->phy_regs = OMAP3_ISP_IOMEM_CSIPHY1;
> -		mutex_init(&phy1->mutex);
> +	if (isp->revision != ISP_REVISION_15_0) {
> +		memset(phy1, sizeof(*phy1), 0);
> +		return 0;

Isn't the memory allocated using kzalloc(), meaning it's already zero?

>  	}
>  
> +	phy1->isp = isp;
> +	phy1->csi2 = &isp->isp_csi2c;
> +	phy1->num_data_lanes = ISP_CSIPHY1_NUM_DATA_LANES;
> +	phy1->cfg_regs = OMAP3_ISP_IOMEM_CSI2C_REGS1;
> +	phy1->phy_regs = OMAP3_ISP_IOMEM_CSIPHY1;
> +	mutex_init(&phy1->mutex);
>  	return 0;
>  }
> 
> 

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