Hi Sakari,

On Saturday 07 January 2012 23:51:24 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Tuesday 20 December 2011 21:28:05 Sakari Ailus wrote:
> >> From: Sakari Ailus <sakari.ai...@iki.fi>
> >> 
> >> Configure CSI-2 phy based on platform data in the ISP driver rather than
> >> in platform code.
> >> 
> >> Signed-off-by: Sakari Ailus <sakari.ai...@iki.fi>

[snip]

> >> diff --git a/drivers/media/video/omap3isp/ispcsiphy.c
> >> b/drivers/media/video/omap3isp/ispcsiphy.c index 5be37ce..f027ece 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"
> >> +
> > 
> > #include <mach/control.h>
> > 
> > (untested) ?
> 
> I'm afraid it won't work. The above directory isn't in any include path
> and likely shouldn't be. That file is included locally elsewhere, I
> believe.

You're right, I spoke too fast.

> I wonder what would be the right way to access that register definition
> I need from the file:
> 
>       OMAP343X_CTRL_BASE
>       OMAP3630_CONTROL_CAMERA_PHY_CTRL

Maybe the file (or part of it) should be moved to an include directory ?

> >>  #include "isp.h"
> >>  #include "ispreg.h"
> >>  #include "ispcsiphy.h"
> >> 
> >> @@ -138,15 +140,78 @@ 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)
> >> 
> >>  {
> >> 
> >> +  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;
> >> 
> >> +  u32 cam_phy_ctrl;
> >> +
> >> +  if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1
> >> +      || subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
> >> +          lanes = subdevs->bus.ccp2.lanecfg;
> >> +  else
> >> +          lanes = subdevs->bus.csi2.lanecfg;
> > 
> > Shouldn't lane configuration be retrieved from the sensor instead ?
> > Sensors could use different lane configuration depending on the mode.
> > This could also be implemented later when needed, but I don't think it
> > would be too difficult to get it right now.
> 
> I think we'd first need to standardise the CSI-2 bus configuration. I
> don't see a practical need to make the lane configuration dynamic. You
> could just use a lower frequency to achieve the same if you really need to.
> 
> Ideally it might be nice to do but there's really nothing I know that
> required or even benefited from it --- at least for now.

Does this mean that lane configuration needs to be duplicated in board code, 
on for the SMIA++ platform data and one of the OMAP3 ISP platform data ?

-- 
Regards,

Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to