Hi Sakari,
Thank you for the review.
On Sunday 06 October 2013 00:49:28 Sakari Ailus wrote:
> Hi Laurent,
>
> Thanks for the patch! Some comments below.
>
> On Thu, Oct 03, 2013 at 01:55:28AM +0200, Laurent Pinchart wrote:
> ...
>
> > +int omap4iss_get_external_info(struct iss_pipeline *pipe,
> > + struct media_link *link)
> > +{
> > + struct iss_device *iss =
> > + container_of(pipe, struct iss_video, pipe)->iss;
> > + struct v4l2_subdev_format fmt;
> > + struct v4l2_ext_controls ctrls;
> > + struct v4l2_ext_control ctrl;
> > + int ret;
> > +
> > + if (!pipe->external)
> > + return 0;
> > +
> > + if (pipe->external_rate)
> > + return 0;
> > +
> > + memset(&fmt, 0, sizeof(fmt));
> > +
> > + fmt.pad = link->source->index;
> > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + ret = v4l2_subdev_call(media_entity_to_v4l2_subdev(link->sink-
>entity),
> > + pad, get_fmt, NULL, &fmt);
> > + if (ret < 0)
> > + return -EPIPE;
> > +
> > + pipe->external_bpp = omap4iss_video_format_info(fmt.format.code)-
>bpp;
> > +
> > + memset(&ctrls, 0, sizeof(ctrls));
> > + memset(&ctrl, 0, sizeof(ctrl));
> > +
>
> As a general note, you can replace memsets of local structs and arrays by
> assingning them as {0}. No need to worry about size for instance.
Good point. That code is going away though :-)
> > + ctrl.id = V4L2_CID_PIXEL_RATE;
> > +
> > + ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
> > + ctrls.count = 1;
> > + ctrls.controls = &ctrl;
> > +
> > + ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, &ctrls);
> > + if (ret < 0) {
> > + dev_warn(iss->dev, "no pixel rate control in subdev %s\n",
> > + pipe->external->name);
> > + return ret;
> > + }
> > +
> > + pipe->external_rate = ctrl.value64;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Configure the bridge. Valid inputs are
> > + *
> > + * IPIPEIF_INPUT_CSI2A: CSI2a receiver
> > + * IPIPEIF_INPUT_CSI2B: CSI2b receiver
> > + *
> > + * The bridge and lane shifter are configured according to the selected
> > input + * and the ISP platform data.
> > + */
> > +void omap4iss_configure_bridge(struct iss_device *iss,
> > + enum ipipeif_input_entity input)
> > +{
> > + u32 issctrl_val;
> > + u32 isp5ctrl_val;
> > +
> > + issctrl_val = readl(iss->regs[OMAP4_ISS_MEM_TOP] + ISS_CTRL);
> > + issctrl_val &= ~ISS_CTRL_INPUT_SEL_MASK;
> > + issctrl_val &= ~ISS_CTRL_CLK_DIV_MASK;
> > +
> > + isp5ctrl_val = readl(iss->regs[OMAP4_ISS_MEM_ISP_SYS1] + ISP5_CTRL);
> > +
> > + switch (input) {
> > + case IPIPEIF_INPUT_CSI2A:
> > + issctrl_val |= ISS_CTRL_INPUT_SEL_CSI2A;
> > + isp5ctrl_val |= ISP5_CTRL_VD_PULSE_EXT;
> > + break;
> > +
> > + case IPIPEIF_INPUT_CSI2B:
> > + issctrl_val |= ISS_CTRL_INPUT_SEL_CSI2B;
> > + isp5ctrl_val |= ISP5_CTRL_VD_PULSE_EXT;
>
> This assignment is independent of the case. You could do that a little later
> just once.
I'll fix that.
> > + break;
> > +
> > + default:
> > + return;
>
> Isn't this an error?
This should never happen, it would be a driver bug. If the input isn't
configured the pipeline isn't valid, which should be caught when starting the
stream before calling this function.
> > + }
> > +
> > + issctrl_val |= ISS_CTRL_SYNC_DETECT_VS_RAISING;
> > +
> > + isp5ctrl_val |= ISP5_CTRL_PSYNC_CLK_SEL | ISP5_CTRL_SYNC_ENABLE;
> > +
> > + writel(issctrl_val, iss->regs[OMAP4_ISS_MEM_TOP] + ISS_CTRL);
> > + writel(isp5ctrl_val, iss->regs[OMAP4_ISS_MEM_ISP_SYS1] + ISP5_CTRL);
> > +}
>
> ...
>
> > +
> > +/*
> > + * iss_pipeline_enable - Enable streaming on a pipeline
> > + * @pipe: ISS pipeline
> > + * @mode: Stream mode (single shot or continuous)
> > + *
> > + * Walk the entities chain starting at the pipeline output video node and
> > start + * all modules in the chain in the given mode.
> > + *
> > + * Return 0 if successful, or the return value of the failed
> > video::s_stream + * operation otherwise.
> > + */
> > +static int iss_pipeline_enable(struct iss_pipeline *pipe,
> > + enum iss_pipeline_stream_state mode)
> > +{
> > + struct media_entity *entity;
> > + struct media_pad *pad;
> > + struct v4l2_subdev *subdev;
> > + unsigned long flags;
> > + int ret;
> > +
> > + spin_lock_irqsave(&pipe->lock, flags);
> > + pipe->state &= ~(ISS_PIPELINE_IDLE_INPUT | ISS_PIPELINE_IDLE_OUTPUT);
> > + spin_unlock_irqrestore(&pipe->lock, flags);
> > +
> > + pipe->do_propagation = false;
> > +
> > + entity = &pipe->output->video.entity;
> > + while (1) {
> > + pad = &entity->pads[0];
> > + if (!(pad->flags & MEDIA_PAD_FL_SINK))
> > + break;
> > +
> > + pad = media_entity_remote_pad(pad);
> > + if (pad == NULL ||
> > + media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> > + break;
> > +
> > + entity = pad->entity;
> > + subdev = media_entity_to_v4l2_subdev(entity);
> > +
> > + ret = v4l2_subdev_call(subdev, video, s_stream, mode);
> > + if (ret < 0 && ret != -ENOIOCTLCMD)
> > + return ret;
>
> This loop looks very similar to that in the omap3isp driver.
>
> It'd be nice to do this in a generic way. Something to think about in the
> future perhaps. This is still queite easy; it gets somewhat more difficult
> once the pipeline isn't linear.
That's a good idea. I'll check whether we have other common needs.
> > + }
> > + iss_print_status(pipe->output->iss);
> > + return 0;
> > +}
> > +
> > +/*
> > + * iss_pipeline_disable - Disable streaming on a pipeline
> > + * @pipe: ISS pipeline
> > + *
> > + * Walk the entities chain starting at the pipeline output video node and
> > stop + * all modules in the chain. Wait synchronously for the modules to
> > be stopped if + * necessary.
> > + */
> > +static int iss_pipeline_disable(struct iss_pipeline *pipe)
> > +{
> > + struct media_entity *entity;
> > + struct media_pad *pad;
> > + struct v4l2_subdev *subdev;
> > + int failure = 0;
> > +
> > + entity = &pipe->output->video.entity;
> > + while (1) {
> > + pad = &entity->pads[0];
> > + if (!(pad->flags & MEDIA_PAD_FL_SINK))
> > + break;
> > +
> > + pad = media_entity_remote_pad(pad);
> > + if (pad == NULL ||
> > + media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> > + break;
> > +
> > + entity = pad->entity;
> > + subdev = media_entity_to_v4l2_subdev(entity);
> > +
> > + v4l2_subdev_call(subdev, video, s_stream, 0);
>
> What would you think about combining the loop with enabling the pipeline?
> The only difference is that you're disabling streaming here, not enabling
> it, and no error handling is done either. Just and idea.
I'll try this when factorizing common code between the omap3isp and omap4iss
drivers.
> > + }
> > +
> > + return failure;
>
> You always return zero here.
I'll fix that.
> > +}
[snip]
> > +/*
> > + * iss_pipeline_is_last - Verify if entity has an enabled link to the
> > output + * video node
> > + * @me: ISS module's media entity
> > + *
> > + * Returns 1 if the entity has an enabled link to the output video node
> > or 0 + * otherwise. It's true only while pipeline can have no more than
> > one output + * node.
> > + */
> > +static int iss_pipeline_is_last(struct media_entity *me)
> > +{
> > + struct iss_pipeline *pipe;
> > + struct media_pad *pad;
> > +
> > + if (!me->pipe)
> > + return 0;
> > + pipe = to_iss_pipeline(me);
> > + if (pipe->stream_state == ISS_PIPELINE_STREAM_STOPPED)
> > + return 0;
> > + pad = media_entity_remote_pad(&pipe->output->pad);
> > + return pad->entity == me;
> > +}
> > +
> > +static int iss_reset(struct iss_device *iss)
> > +{
> > + unsigned long timeout = 0;
> > +
> > + writel(readl(iss->regs[OMAP4_ISS_MEM_TOP] + ISS_HL_SYSCONFIG) |
> > + ISS_HL_SYSCONFIG_SOFTRESET,
> > + iss->regs[OMAP4_ISS_MEM_TOP] + ISS_HL_SYSCONFIG);
> > +
> > + while (readl(iss->regs[OMAP4_ISS_MEM_TOP] + ISS_HL_SYSCONFIG) &
> > + ISS_HL_SYSCONFIG_SOFTRESET) {
> > + if (timeout++ > 10000) {
> > + dev_alert(iss->dev, "cannot reset ISS\n");
> > + return -ETIMEDOUT;
> > + }
> > + udelay(1);
>
> You only seem to do this in probe. How about e.g. usleep_range(10, 10) and
> timeout of 1000?
Good point, I'll fix that.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int iss_isp_reset(struct iss_device *iss)
> > +{
> > + unsigned long timeout = 0;
> > +
> > + /* Fist, ensure that the ISP is IDLE (no transactions happening) */
> > + writel((readl(iss->regs[OMAP4_ISS_MEM_ISP_SYS1] + ISP5_SYSCONFIG) &
> > + ~ISP5_SYSCONFIG_STANDBYMODE_MASK) |
> > + ISP5_SYSCONFIG_STANDBYMODE_SMART,
> > + iss->regs[OMAP4_ISS_MEM_ISP_SYS1] + ISP5_SYSCONFIG);
> > +
> > + writel(readl(iss->regs[OMAP4_ISS_MEM_ISP_SYS1] + ISP5_CTRL) |
> > + ISP5_CTRL_MSTANDBY,
> > + iss->regs[OMAP4_ISS_MEM_ISP_SYS1] + ISP5_CTRL);
> > +
> > + for (;;) {
> > + if (readl(iss->regs[OMAP4_ISS_MEM_ISP_SYS1] + ISP5_CTRL) &
> > + ISP5_CTRL_MSTANDBY_WAIT)
> > + break;
> > + if (timeout++ > 1000) {
> > + dev_alert(iss->dev, "cannot set ISP5 to standby\n");
> > + return -ETIMEDOUT;
> > + }
> > + msleep(1);
>
> Is this time critical in any way? msleep(1) may end up sleeping quite a lot
> longer. I propose usleep_range().
I'll fix that.
> > + }
> > +
> > + /* Now finally, do the reset */
> > + writel(readl(iss->regs[OMAP4_ISS_MEM_ISP_SYS1] + ISP5_SYSCONFIG) |
> > + ISP5_SYSCONFIG_SOFTRESET,
> > + iss->regs[OMAP4_ISS_MEM_ISP_SYS1] + ISP5_SYSCONFIG);
> > +
> > + timeout = 0;
> > + while (readl(iss->regs[OMAP4_ISS_MEM_ISP_SYS1] + ISP5_SYSCONFIG) &
> > + ISP5_SYSCONFIG_SOFTRESET) {
> > + if (timeout++ > 1000) {
> > + dev_alert(iss->dev, "cannot reset ISP5\n");
> > + return -ETIMEDOUT;
> > + }
> > + msleep(1);
>
> Wow. :) Remember msleep() is usually notoriously imprecise, too. E.g.
> CONFIG_HZ at 100 could mean msleep(1) sleeps up to 20 ms, and 1000 that can
> be up to 20 seconds. Is this intended?
I doubt it :-)
> > + }
> > +
> > + return 0;
> > +}
[snip]
> > diff --git a/include/media/omap4iss.h b/include/media/omap4iss.h
> > new file mode 100644
> > index 0000000..0d7620d
> > --- /dev/null
> > +++ b/include/media/omap4iss.h
> > @@ -0,0 +1,65 @@
>
> There's no copyright notice in this file. Is it intentional? I don't demand
> it; just wondering. :)
It's not my code so I don't know :-)
> > +#ifndef ARCH_ARM_PLAT_OMAP4_ISS_H
> > +#define ARCH_ARM_PLAT_OMAP4_ISS_H
> > +
> > +#include <linux/i2c.h>
> > +
> > +struct iss_device;
> > +
> > +enum iss_interface_type {
> > + ISS_INTERFACE_CSI2A_PHY1,
> > + ISS_INTERFACE_CSI2B_PHY2,
> > +};
> > +
> > +/**
> > + * struct iss_csiphy_lane: CSI2 lane position and polarity
> > + * @pos: position of the lane
> > + * @pol: polarity of the lane
> > + */
> > +struct iss_csiphy_lane {
> > + u8 pos;
> > + u8 pol;
> > +};
> > +
> > +#define ISS_CSIPHY1_NUM_DATA_LANES 4
> > +#define ISS_CSIPHY2_NUM_DATA_LANES 1
> > +
> > +/**
> > + * struct iss_csiphy_lanes_cfg - CSI2 lane configuration
> > + * @data: Configuration of one or two data lanes
> > + * @clk: Clock lane configuration
> > + */
> > +struct iss_csiphy_lanes_cfg {
> > + struct iss_csiphy_lane data[ISS_CSIPHY1_NUM_DATA_LANES];
> > + struct iss_csiphy_lane clk;
> > +};
> > +
> > +/**
> > + * struct iss_csi2_platform_data - CSI2 interface platform data
> > + * @crc: Enable the cyclic redundancy check
> > + * @vpclk_div: Video port output clock control
> > + */
> > +struct iss_csi2_platform_data {
> > + unsigned crc:1;
> > + unsigned vpclk_div:2;
> > + struct iss_csiphy_lanes_cfg lanecfg;
> > +};
> > +
> > +struct iss_subdev_i2c_board_info {
> > + struct i2c_board_info *board_info;
> > + int i2c_adapter_id;
> > +};
> > +
> > +struct iss_v4l2_subdevs_group {
> > + struct iss_subdev_i2c_board_info *subdevs;
> > + enum iss_interface_type interface;
> > + union {
> > + struct iss_csi2_platform_data csi2;
> > + } bus; /* gcc < 4.6.0 chokes on anonymous union initializers */
> > +};
> > +
> > +struct iss_platform_data {
> > + struct iss_v4l2_subdevs_group *subdevs;
> > + void (*set_constraints)(struct iss_device *iss, bool enable);
> > +};
> > +
> > +#endif
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
