Hi Sakari,
On Sunday 08 January 2012 11:26:02 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > 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.ailus@xxxxxx>
> >>>>
> >>>> Configure CSI-2 phy based on platform data in the ISP driver rather
> >>>> than in platform code.
> >>>>
> >>>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxx>
> >
> > [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 ?
>
> Yes, but which one?
Good question. The content of control.h doesn't seem to have been meant to be
exported. Maybe you should ask on linux-omap@xxxxxxxxxxxxxxx ?
> >>>> #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 ?
>
> It's mostly the number of lanes, and the polarity --- in theory it is
> possible to invert the signals on the bus, albeit I'm not sure if anyone
> does that; I can't see a reason for that, but hey, I don't know why it's
> possible to specify polarity either. :-)
>
> If both sides support mapping of the lanes, a mapping that matches on
> both sides has to be provided.
>
> I agree we should standardise the configuration of CSI-2 as well as the
> configuration of other busses. However, I would prefer to do that later
> on since I'm already depending on a number of other patches on the
> SMIA++ driver.
OK.
--
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
