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

[Laurent Pinchart]

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