Hi Laurent,
On Thu, Jan 19, 2012 at 05:16:50PM +0100, Laurent Pinchart wrote:
> On Tuesday 17 January 2012 20:45:18 Sakari Ailus wrote:
> > On Mon, Jan 16, 2012 at 03:22:42PM +0100, Laurent Pinchart wrote:
> > > On Wednesday 11 January 2012 22:26:51 Sakari Ailus wrote:
> > > > Configure CSI-2 phy based on platform data in the ISP driver. For that,
> > > > the new V4L2_CID_IMAGE_SOURCE_PIXEL_RATE control is used. Previously
> > > > the same was configured from the board code.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxx>
> > > > ---
> > > >
> > > > drivers/media/video/omap3isp/isp.c | 29 ++++++++++-
> > > > drivers/media/video/omap3isp/isp.h | 4 --
> > > > drivers/media/video/omap3isp/ispcsi2.c | 42 ++++++++++++++-
> > > > drivers/media/video/omap3isp/ispcsiphy.c | 84
> > > >
> > > > ++++++++++++++++++++++++++---- drivers/media/video/omap3isp/ispcsiphy.h
> > > > |
> > > >
> > > > 5 ++
> > > >
> > > > 5 files changed, 148 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/media/video/omap3isp/isp.c
> > > > b/drivers/media/video/omap3isp/isp.c index b818cac..d268d55 100644
> > > > --- a/drivers/media/video/omap3isp/isp.c
> > > > +++ b/drivers/media/video/omap3isp/isp.c
> > > > @@ -765,6 +765,34 @@ static int isp_pipeline_enable(struct isp_pipeline
> > > > *pipe, if (ret < 0 && ret != -ENOIOCTLCMD)
> > > >
> > > > return ret;
> > > >
> > > > + /*
> > > > + * Configure CCDC pixel clock. host_priv != NULL so
> > > > + * this one is a sensor.
> > > > + */
> > > > + if (subdev->host_priv) {
> > > > + struct v4l2_ext_controls ctrls;
> > > > + struct v4l2_ext_control ctrl;
> > > > +
> > > > + memset(&ctrls, 0, sizeof(ctrls));
> > > > + memset(&ctrl, 0, sizeof(ctrl));
> > > > +
> > > > + ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;
> > > > +
> > > > + ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
> > > > + ctrls.count = 1;
> > > > + ctrls.controls = &ctrl;
> > > > +
> > > > + ret = v4l2_g_ext_ctrls(subdev->ctrl_handler, &ctrls);
> > > > + if (ret < 0) {
> > > > + dev_warn(isp->dev,
> > > > + "no pixel rate control in subdev %s\n",
> > > > + subdev->name);
> > > > + return -EPIPE;
> > > > + }
> > > > +
> > > > + isp_set_pixel_clock(isp, ctrl.value64);
> > >
> > > Isn't this too late ? The CCDC has already been started. What about
> > > moving this code to ccdc_config_vp() ?
> >
> > I think you're right. I've mostly rewritten this part of the patchset and I
> > think it's much cleaner now.
> >
> > I have validate_link() functions where I'm validating the link and also
> > gathering information alongside that process into a few fields in
> > isp_pipeline struct.
> >
> > Then those fields can be used in streamon time. Some additional checks for
> > those parameters are also done there and they may result into an error
> > which is then handled.
> >
> > > > + }
> > > > +
> > > >
> > > > if (subdev == &isp->isp_ccdc.subdev) {
> > > >
> > > > v4l2_subdev_call(&isp->isp_aewb.subdev, video,
> > > >
> > > > s_stream, mode);
> > > >
> > > > @@ -2072,7 +2100,6 @@ static int isp_probe(struct platform_device
> > > > *pdev)
> > > >
> > > > isp->autoidle = autoidle;
> > > > isp->platform_cb.set_xclk = isp_set_xclk;
> > > >
> > > > - isp->platform_cb.set_pixel_clock = isp_set_pixel_clock;
> > > >
> > > > mutex_init(&isp->isp_mutex);
> > > > spin_lock_init(&isp->stat_lock);
> > > >
> > > > diff --git a/drivers/media/video/omap3isp/isp.h
> > > > b/drivers/media/video/omap3isp/isp.h index ff1c422..dd1b61e 100644
> > > > --- a/drivers/media/video/omap3isp/isp.h
> > > > +++ b/drivers/media/video/omap3isp/isp.h
> > > > @@ -126,10 +126,6 @@ struct isp_reg {
> > > >
> > > > struct isp_platform_callback {
> > > >
> > > > u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
> > > >
> > > > - int (*csiphy_config)(struct isp_csiphy *phy,
> > > > - struct isp_csiphy_dphy_cfg *dphy,
> > > > - struct isp_csiphy_lanes_cfg *lanes);
> > > > - void (*set_pixel_clock)(struct isp_device *isp, unsigned int
> > > > pixelclk);
> > > >
> > > > };
> > > >
> > > > /*
> > > >
> > > > diff --git a/drivers/media/video/omap3isp/ispcsi2.c
> > > > b/drivers/media/video/omap3isp/ispcsi2.c index 0c5f1cb..0b3e705 100644
> > > > --- a/drivers/media/video/omap3isp/ispcsi2.c
> > > > +++ b/drivers/media/video/omap3isp/ispcsi2.c
> > > > @@ -1055,7 +1055,45 @@ static int csi2_set_stream(struct v4l2_subdev
> > > > *sd, int enable) struct isp_video *video_out = &csi2->video_out;
> > > >
> > > > switch (enable) {
> > > >
> > > > - case ISP_PIPELINE_STREAM_CONTINUOUS:
> > > > + case ISP_PIPELINE_STREAM_CONTINUOUS: {
> > > > + struct media_pad *remote_pad =
> > > > + media_entity_remote_source(&sd->entity.pads[0]);
> > >
> > > As you will need to locate the sensor in ccdc_config_vp() as well, you
> > > should store a pointer to the sensor in the pipeline structure in
> > > isp_video_streamon(). I think I've sent you code that does just that,
> > > originally written by Stan if my memory is correct.
> >
> > Stan's patch assumed all external subdevs will be sensors. That may not be
> > true in all possible cases. That's not the fault of the patch, things have
> > changed after it has been written.
> >
> > I'd be suspicious calling the SMIA++ sensor's scaler subdev a sensor.
> > Definitely an external ISP will not be a sensor.
>
> Then just rename the field to something else than sensor :-) We need to access
> the external subdev in various places in the driver, instead of trying to
> locate it every time, it would be better to locate it once in
> isp_video_streamon (or one of the helper functions it calls) and store the
> pointer in the isp_pipeline structure.
My point was that Stan's patch located the subdev based on its type. We
can't rely on that. But yes, I'll store the information to isp_pipeline. I'm
not even sure the external subdev has to be there, just some information
related to it.
>
> > The check I'm doing is that if something's connected to either ccp2 or csi2
> > (and they're subdevs!) it's the external subdev we're interested in. Same
> > for ccdc if the entity is neither of the csi2 receivers or the ccp2
> > receiver.
> >
> > > > + struct v4l2_subdev *remote_subdev =
> > > > + media_entity_to_v4l2_subdev(remote_pad->entity);
> > > > + struct v4l2_subdev_format fmt;
> > > > + struct v4l2_ext_controls ctrls;
> > > > + struct v4l2_ext_control ctrl;
> > > > + int ret;
> > > > +
> > > > + fmt.pad = remote_pad->index;
> > > > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > > + ret = v4l2_subdev_call(
> > > > + remote_subdev, pad, get_fmt, NULL, &fmt);
> > > > + if (ret < 0)
> > > > + return -EPIPE;
> > > > +
> > > > + memset(&ctrls, 0, sizeof(ctrls));
> > > > + memset(&ctrl, 0, sizeof(ctrl));
> > > > +
> > > > + ctrl.id = V4L2_CID_IMAGE_SOURCE_PIXEL_RATE;
> > > > +
> > > > + ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl.id);
> > > > + ctrls.count = 1;
> > > > + ctrls.controls = &ctrl;
> > > > +
> > > > + ret = v4l2_g_ext_ctrls(remote_subdev->ctrl_handler, &ctrls);
> > >
> > > Wouldn't v4l2_g_ctrl be easier ?
> >
> > Yes, but it's a 64-bit control so I must use extended controls.
>
> Right.
>
> > > An option to avoid duplicating this code in ccdc_config_vp() would be to
> > > move pixel rate retrieval to isp_video_streamon().
> >
> > It's gathered during link validation now.
> >
> > > > + if (ret < 0) {
> > > > + dev_warn(isp->dev,
> > > > + "no pixel rate control in subdev %s\n",
> > > > + remote_subdev->name);
> > > > + return -EPIPE;
> > > > + }
> > > > +
> > > > + ret = omap3isp_csiphy_config(
> > > > + isp, sd, remote_subdev, &fmt.format, ctrl.value64);
> > > > + if (ret < 0)
> > > > + return -EPIPE;
> > > > +
> > > >
> > > > if (omap3isp_csiphy_acquire(csi2->phy) < 0)
> > > >
> > > > return -ENODEV;
> > > >
> > > > csi2->use_fs_irq = pipe->do_propagation;
> > > >
> > > > @@ -1080,6 +1118,8 @@ static int csi2_set_stream(struct v4l2_subdev
> > > > *sd, int enable) isp_video_dmaqueue_flags_clr(video_out);
> > > >
> > > > break;
> > > >
> > > > + }
> > > > +
> > > >
> > > > case ISP_PIPELINE_STREAM_STOPPED:
> > > > if (csi2->state == ISP_PIPELINE_STREAM_STOPPED)
> > > >
> > > > return 0;
> > > >
> > > > diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
> > > > b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f286a01
> > > > 100644 --- a/drivers/media/video/omap3isp/ispcsiphy.c
> > > > +++ b/drivers/media/video/omap3isp/ispcsiphy.c
> > > > @@ -28,6 +28,8 @@
> > > >
> > > > #include <linux/device.h>
> > > > #include <linux/regulator/consumer.h>
> > > >
> > > > +#include "../../../../arch/arm/mach-omap2/control.h"
> > > > +
> > >
> > > Still no solution for this ?
> >
> > Not yet. I'll have to see the discussion again to say more on that. I think
> > we can find _something_ but it might not be more compatible with the device
> > tree than this.
>
> I proposed adding an exported function to arch/arm/mach-omap2/control.c to
> access the field, but there was no reply.
I remember that. I think it's a better solution but not ideal. We also must
provide the function on all platforms the OMAP 3 ISP driver is to be used,
i.e. OMAP 3s. On 3[45]xx it just wouldn't do anything.
> > > > #include "isp.h"
> > > > #include "ispreg.h"
> > > > #include "ispcsiphy.h"
> > > >
> > > > @@ -138,15 +140,79 @@ static void csiphy_dphy_config(struct isp_csiphy
> > > > *phy) isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
> > > >
> > > > }
> > > >
> > > > -static int csiphy_config(struct isp_csiphy *phy,
> > > > - struct isp_csiphy_dphy_cfg *dphy,
> > > > - struct isp_csiphy_lanes_cfg *lanes)
> > > > +/*
> > > > + * TCLK values are OK at their reset values
> > > > + */
> > > > +#define TCLK_TERM 0
> > > > +#define TCLK_MISS 1
> > > > +#define TCLK_SETTLE 14
> > > > +
> > > > +int omap3isp_csiphy_config(struct isp_device *isp,
> > > > + struct v4l2_subdev *csi2_subdev,
> > > > + struct v4l2_subdev *sensor,
> > > > + struct v4l2_mbus_framefmt *sensor_fmt,
> > > > + uint32_t pixel_rate)
> > > >
> > > > {
> > > >
> > > > + struct isp_v4l2_subdevs_group *subdevs = sensor->host_priv;
> > > > + struct isp_csi2_device *csi2 = v4l2_get_subdevdata(csi2_subdev);
> > > > + struct isp_csiphy_dphy_cfg csi2phy;
> > > > + int csi2_ddrclk_khz;
> > > > + struct isp_csiphy_lanes_cfg *lanes;
> > > >
> > > > unsigned int used_lanes = 0;
> > > > unsigned int i;
> > > >
> > > > + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
> > > > + || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
> > > > + lanes = &subdevs->bus.ccp2.lanecfg;
> > > > + else
> > > > + lanes = &subdevs->bus.csi2.lanecfg;
> > > > +
> > > > + if (!lanes) {
> > >
> > > This can't happen.
> >
> > Fixed.
Cheers,
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx
--
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
