Hi Laurent,
Thanks for the review!!!
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>
>> ---
>> drivers/media/video/omap3isp/isp.c | 38 ++++++++++++--
>> drivers/media/video/omap3isp/isp.h | 3 -
>> drivers/media/video/omap3isp/ispcsiphy.c | 83 +++++++++++++++++++++++----
>> drivers/media/video/omap3isp/ispcsiphy.h | 4 ++
>> 4 files changed, 111 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/media/video/omap3isp/isp.c
>> b/drivers/media/video/omap3isp/isp.c index b818cac..6020fd7 100644
>> --- a/drivers/media/video/omap3isp/isp.c
>> +++ b/drivers/media/video/omap3isp/isp.c
>> @@ -737,7 +737,7 @@ static int isp_pipeline_enable(struct isp_pipeline
>> *pipe, struct isp_device *isp = pipe->output->isp;
>> struct media_entity *entity;
>> struct media_pad *pad;
>> - struct v4l2_subdev *subdev;
>> + struct v4l2_subdev *subdev = NULL, *prev_subdev;
>> unsigned long flags;
>> int ret;
>>
>> @@ -759,11 +759,41 @@ static int isp_pipeline_enable(struct isp_pipeline
>> *pipe, break;
>>
>> entity = pad->entity;
>> + prev_subdev = subdev;
>> subdev = media_entity_to_v4l2_subdev(entity);
>>
>> - ret = v4l2_subdev_call(subdev, video, s_stream, mode);
>> - if (ret < 0 && ret != -ENOIOCTLCMD)
>> - return ret;
>> + /* Configure CSI-2 receiver based on sensor format. */
>> + if (prev_subdev == &isp->isp_csi2a.subdev
>> + || prev_subdev == &isp->isp_csi2c.subdev) {
>> + struct v4l2_subdev_format fmt;
>> +
>> + fmt.pad = pad->index;
>> + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>> + ret = v4l2_subdev_call(subdev, pad, get_fmt,
>> + NULL, &fmt);
>> + if (ret < 0)
>> + return -EPIPE;
>> +
>> + ret = omap3isp_csiphy_config(
>> + isp, prev_subdev, subdev,
>> + &fmt.format);
>> + if (ret < 0)
>> + return -EPIPE;
>> +
>> + /* Start CSI-2 after configuration. */
>> + ret = v4l2_subdev_call(prev_subdev, video,
>> + s_stream, mode);
>> + if (ret < 0 && ret != -ENOIOCTLCMD)
>> + return ret;
>> + }
>> +
>> + /* Start any other subdev except the CSI-2 receivers. */
>> + if (subdev != &isp->isp_csi2a.subdev
>> + && subdev != &isp->isp_csi2c.subdev) {
>> + ret = v4l2_subdev_call(subdev, video, s_stream, mode);
>> + if (ret < 0 && ret != -ENOIOCTLCMD)
>> + return ret;
>> + }
>
> What about moving this to the CSI2 s_stream subdev operation ?
Done.
>>
>> if (subdev == &isp->isp_ccdc.subdev) {
>> v4l2_subdev_call(&isp->isp_aewb.subdev, video,
>> diff --git a/drivers/media/video/omap3isp/isp.h
>> b/drivers/media/video/omap3isp/isp.h index 705946e..c5935ae 100644
>> --- a/drivers/media/video/omap3isp/isp.h
>> +++ b/drivers/media/video/omap3isp/isp.h
>> @@ -126,9 +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/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.
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
>> #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.
>> +
>> + if (!lanes) {
>> + dev_err(isp->dev, "no lane configuration\n");
>> + return -EINVAL;
>> + }
>> +
>> + cam_phy_ctrl = omap_readl(
>> + OMAP343X_CTRL_BASE + OMAP3630_CONTROL_CAMERA_PHY_CTRL);
>> + /*
>> + * SCM.CONTROL_CAMERA_PHY_CTRL
>> + * - bit[4] : CSIPHY1 data sent to CSIB
>> + * - bit [3:2] : CSIPHY1 config: 00 d-phy, 01/10 ccp2
>> + * - bit [1:0] : CSIPHY2 config: 00 d-phy, 01/10 ccp2
>> + */
>> + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY1)
>> + cam_phy_ctrl |= 1 << 2;
>> + else if (subdevs->interface == ISP_INTERFACE_CSI2C_PHY1)
>> + cam_phy_ctrl &= 1 << 2;
>> +
>> + if (subdevs->interface == ISP_INTERFACE_CCP2B_PHY2)
>> + cam_phy_ctrl |= 1;
>> + else if (subdevs->interface == ISP_INTERFACE_CSI2A_PHY2)
>> + cam_phy_ctrl &= 1;
>> +
>> + /* FIXME: Do 34xx / 35xx require something here? */
>> + if (cpu_is_omap3630())
>> + omap_writel(cam_phy_ctrl,
>> + OMAP343X_CTRL_BASE
>> + + OMAP3630_CONTROL_CAMERA_PHY_CTRL);
>
> You use cam_phy_ctrl inside the if statement only, you can move its
> computation there.
Will fix.
>> +
>> + csi2_ddrclk_khz = sensor_fmt->pixelrate
>> + / (2 * csi2->phy->num_data_lanes)
>> + * omap3isp_video_format_info(sensor_fmt->code)->bpp;
>> +
>> + /*
>> + * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.
>> + * THS_SETTLE: Programmed value = ceil(90 ns/DDRClk period) + 3.
>> + */
>> + csi2phy.ths_term = DIV_ROUND_UP(25 * csi2_ddrclk_khz, 2000000) - 1;
>> + csi2phy.ths_settle = DIV_ROUND_UP(90 * csi2_ddrclk_khz, 1000000) + 3;
>> + csi2phy.tclk_term = TCLK_TERM;
>> + csi2phy.tclk_miss = TCLK_MISS;
>> + csi2phy.tclk_settle = TCLK_SETTLE;
>>
>> /* Clock and data lanes verification */
>> - for (i = 0; i < phy->num_data_lanes; i++) {
>> + for (i = 0; i < csi2->phy->num_data_lanes; i++) {
>> if (lanes->data[i].pol > 1 || lanes->data[i].pos > 3)
>> return -EINVAL;
>>
>> @@ -162,10 +227,10 @@ static int csiphy_config(struct isp_csiphy *phy,
>> if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
>> return -EINVAL;
>>
>> - mutex_lock(&phy->mutex);
>> - phy->dphy = *dphy;
>> - phy->lanes = *lanes;
>> - mutex_unlock(&phy->mutex);
>> + mutex_lock(&csi2->phy->mutex);
>> + csi2->phy->dphy = csi2phy;
>> + csi2->phy->lanes = *lanes;
>> + mutex_unlock(&csi2->phy->mutex);
>>
>> return 0;
>> }
>> @@ -225,8 +290,6 @@ int omap3isp_csiphy_init(struct isp_device *isp)
>> struct isp_csiphy *phy1 = &isp->isp_csiphy1;
>> struct isp_csiphy *phy2 = &isp->isp_csiphy2;
>>
>> - isp->platform_cb.csiphy_config = csiphy_config;
>> -
>> phy2->isp = isp;
>> phy2->csi2 = &isp->isp_csi2a;
>> phy2->num_data_lanes = ISP_CSIPHY2_NUM_DATA_LANES;
>> diff --git a/drivers/media/video/omap3isp/ispcsiphy.h
>> b/drivers/media/video/omap3isp/ispcsiphy.h index e93a661..9f93222 100644
>> --- a/drivers/media/video/omap3isp/ispcsiphy.h
>> +++ b/drivers/media/video/omap3isp/ispcsiphy.h
>> @@ -56,6 +56,10 @@ struct isp_csiphy {
>> struct isp_csiphy_dphy_cfg dphy;
>> };
>>
>> +int omap3isp_csiphy_config(struct isp_device *isp,
>> + struct v4l2_subdev *csi2_subdev,
>> + struct v4l2_subdev *sensor,
>> + struct v4l2_mbus_framefmt *fmt);
>> int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
>> void omap3isp_csiphy_release(struct isp_csiphy *phy);
>> int omap3isp_csiphy_init(struct isp_device *isp);
>
--
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
