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
