Hi Laurent,
Laurent Pinchart wrote:
> 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.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?
>>>> #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.
--
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxxxxxxxxxxxxx
--
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
