| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 15 Feb 2017 10:42:29 +0100
From: Pavel Machek <pavel@....cz>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@....fi>, mchehab@...nel.org,
kernel list <linux-kernel@...r.kernel.org>,
ivo.g.dimitrov.75@...il.com, sre@...nel.org, pali.rohar@...il.com,
linux-media@...r.kernel.org
Subject: Re: [PATCH] omap3isp: add support for CSI1 bus
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
Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)
Powered by blists - more mailing lists