omap3isp: add support for CSI1 bus
Commit Message
Obtain the CSI1/CCP2 bus parameters from the OF node.
ISP CSI1 module needs all the bits correctly set to work.
OMAP3430 needs various syscon CONTROL_CSIRXFE bits set in order to
operate. Implement the missing functionality.
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
---
> How about the rest? :-) I guess we could get the CCP2 support in omap3isp
> without the video bus switch. It'd be nice to have this in a single
> patchset.
Ok, here you go, what about this?
Pavel
Comments
Hi Pavel,
[auto build test WARNING on v4.9-rc8]
[cannot apply to linuxtv-media/master next-20170208]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Pavel-Machek/omap3isp-add-support-for-CSI1-bus/20170208-222249
reproduce: make htmldocs
All warnings (new ones prefixed by >>):
make[3]: warning: jobserver unavailable: using -j1. Add '+' to parent make rule.
include/linux/init.h:1: warning: no structured comments found
include/linux/workqueue.h:392: warning: No description found for parameter '...'
include/linux/workqueue.h:392: warning: Excess function parameter 'args' description in 'alloc_workqueue'
include/linux/workqueue.h:413: warning: No description found for parameter '...'
include/linux/workqueue.h:413: warning: Excess function parameter 'args' description in 'alloc_ordered_workqueue'
include/linux/kthread.h:26: warning: No description found for parameter '...'
kernel/sys.c:1: warning: no structured comments found
drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
include/linux/fence-array.h:61: warning: No description found for parameter 'fence'
include/sound/core.h:324: warning: No description found for parameter '...'
include/sound/core.h:335: warning: No description found for parameter '...'
include/sound/core.h:388: warning: No description found for parameter '...'
include/media/media-entity.h:1054: warning: No description found for parameter '...'
>> include/media/v4l2-of.h:69: warning: No description found for parameter 'lane_polarity[2]'
include/net/mac80211.h:3207: ERROR: Unexpected indentation.
include/net/mac80211.h:3210: WARNING: Block quote ends without a blank line; unexpected unindent.
include/net/mac80211.h:3212: ERROR: Unexpected indentation.
include/net/mac80211.h:3213: WARNING: Block quote ends without a blank line; unexpected unindent.
include/net/mac80211.h:1772: ERROR: Unexpected indentation.
include/net/mac80211.h:1776: WARNING: Block quote ends without a blank line; unexpected unindent.
kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without end-string.
kernel/time/timer.c:1240: ERROR: Unexpected indentation.
kernel/time/timer.c:1242: ERROR: Unexpected indentation.
kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/wait.h:121: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/wait.h:124: ERROR: Unexpected indentation.
include/linux/wait.h:126: WARNING: Block quote ends without a blank line; unexpected unindent.
kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; unexpected unindent.
kernel/signal.c:317: WARNING: Inline literal start-string without end-string.
drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a blank line; unexpected unindent.
drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without a blank line; unexpected unindent.
drivers/tty/serial/serial_core.c:1893: WARNING: Definition list ends without a blank line; unexpected unindent.
include/linux/spi/spi.h:369: ERROR: Unexpected indentation.
WARNING: dvipng command 'dvipng' cannot be run (needed for math display), check the imgmath_dvipng setting
vim +69 include/media/v4l2-of.h
53 };
54
55 /**
56 * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
57 * @clock_inv: polarity of clock/strobe signal
58 * false - not inverted, true - inverted
59 * @strobe: false - data/clock, true - data/strobe
60 * @data_lane: the number of the data lane
61 * @clock_lane: the number of the clock lane
62 */
63 struct v4l2_of_bus_mipi_csi1 {
64 bool clock_inv;
65 bool strobe;
66 bool lane_polarity[2];
67 unsigned char data_lane;
68 unsigned char clock_lane;
> 69 };
70
71 /**
72 * struct v4l2_of_endpoint - the endpoint data structure
73 * @base: struct of_endpoint containing port, id, and local of_node
74 * @bus_type: bus type
75 * @bus: bus configuration data structure
76 * @link_frequencies: array of supported link frequencies
77 * @nr_of_link_frequencies: number of elements in link_frequenccies array
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Pavel,
Thank you for the patch.
On Wednesday 08 Feb 2017 13:57:38 Pavel Machek wrote:
> Obtain the CSI1/CCP2 bus parameters from the OF node.
>
> ISP CSI1 module needs all the bits correctly set to work.
>
> OMAP3430 needs various syscon CONTROL_CSIRXFE bits set in order to
> operate. Implement the missing functionality.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>
> ---
>
> > How about the rest? :-) I guess we could get the CCP2 support in omap3isp
> > without the video bus switch. It'd be nice to have this in a single
> > patchset.
>
> Ok, here you go, what about this?
>
> Pavel
>
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2024,21 +2024,92 @@ enum isp_of_phy {
> ISP_OF_PHY_CSIPHY2,
> };
>
> -static int isp_of_parse_node(struct device *dev, struct device_node *node,
> - struct isp_async_subdev *isd)
> +void __isp_of_parse_node_csi1(struct device *dev,
> + struct isp_ccp2_cfg *buscfg,
> + struct v4l2_of_endpoint *vep)
This function isn't use anywhere else, you can merge it with
isp_of_parse_node_csi1().
> +{
> + buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
> + buscfg->lanecfg.clk.pol =
> + vep->bus.mipi_csi1.lane_polarity[0];
> + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> + buscfg->lanecfg.clk.pol,
> + buscfg->lanecfg.clk.pos);
> +
> + buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
> + buscfg->lanecfg.data[0].pol =
> + vep->bus.mipi_csi2.lane_polarities[1];
bus.mipi_csi2 ?
> + dev_dbg(dev, "data lane polarity %u, pos %u\n",
> + buscfg->lanecfg.data[0].pol,
> + buscfg->lanecfg.data[0].pos);
> +
> + buscfg->strobe_clk_pol = vep->bus.mipi_csi1.clock_inv;
> + buscfg->phy_layer = vep->bus.mipi_csi1.strobe;
> + buscfg->ccp2_mode = vep->bus_type == V4L2_MBUS_CCP2;
> +
> + dev_dbg(dev, "clock_inv %u strobe %u ccp2 %u\n",
> + buscfg->strobe_clk_pol,
> + buscfg->phy_layer,
> + buscfg->ccp2_mode);
> + /*
> + * FIXME: now we assume the CRC is always there.
> + * Implement a way to obtain this information from the
> + * sensor. Frame descriptors, perhaps?
> + */
> + buscfg->crc = 1;
> +
> + buscfg->vp_clk_pol = 1;
> +}
> +
> +static void isp_of_parse_node_csi1(struct device *dev,
> + struct isp_bus_cfg *buscfg,
> + struct v4l2_of_endpoint *vep)
> +{
> + if (vep->base.port == ISP_OF_PHY_CSIPHY1)
> + buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
> + else
> + buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
> + __isp_of_parse_node_csi1(dev, &buscfg->bus.ccp2, vep);
> +}
> +
> +static void isp_of_parse_node_csi2(struct device *dev,
> + struct isp_bus_cfg *buscfg,
> + struct v4l2_of_endpoint *vep)
> {
> - struct isp_bus_cfg *buscfg = &isd->bus;
> - struct v4l2_of_endpoint vep;
> unsigned int i;
> - int ret;
>
> - ret = v4l2_of_parse_endpoint(node, &vep);
> - if (ret)
> - return ret;
> + if (vep->base.port == ISP_OF_PHY_CSIPHY1)
> + buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> + else
> + buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
I would keep this code in the caller to avoid code duplication with
isp_of_parse_node_csi1().
> + buscfg->bus.csi2.lanecfg.clk.pos = vep->bus.mipi_csi2.clock_lane;
> + buscfg->bus.csi2.lanecfg.clk.pol =
> + vep->bus.mipi_csi2.lane_polarities[0];
> + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> + buscfg->bus.csi2.lanecfg.clk.pol,
> + buscfg->bus.csi2.lanecfg.clk.pos);
> +
> + for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> + buscfg->bus.csi2.lanecfg.data[i].pos =
> + vep->bus.mipi_csi2.data_lanes[i];
> + buscfg->bus.csi2.lanecfg.data[i].pol =
> + vep->bus.mipi_csi2.lane_polarities[i + 1];
> + dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> + buscfg->bus.csi2.lanecfg.data[i].pol,
> + buscfg->bus.csi2.lanecfg.data[i].pos);
> + }
>
> - dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> - vep.base.port);
> + /*
> + * FIXME: now we assume the CRC is always there.
> + * Implement a way to obtain this information from the
> + * sensor. Frame descriptors, perhaps?
> + */
> + buscfg->bus.csi2.crc = 1;
> +}
>
> +static int isp_endpoint_to_buscfg(struct device *dev,
> + struct v4l2_of_endpoint vep,
> + struct isp_bus_cfg *buscfg)
> +{
> switch (vep.base.port) {
> case ISP_OF_PHY_PARALLEL:
> buscfg->interface = ISP_INTERFACE_PARALLEL;
> @@ -2059,45 +2130,42 @@ static int isp_of_parse_node(struct device *dev,
> struct device_node *node,
>
> case ISP_OF_PHY_CSIPHY1:
> case ISP_OF_PHY_CSIPHY2:
> - /* FIXME: always assume CSI-2 for now. */
> - switch (vep.base.port) {
> - case ISP_OF_PHY_CSIPHY1:
> - buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> - break;
> - case ISP_OF_PHY_CSIPHY2:
> - buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> - break;
> - }
> - buscfg->bus.csi2.lanecfg.clk.pos =
vep.bus.mipi_csi2.clock_lane;
> - buscfg->bus.csi2.lanecfg.clk.pol =
> - vep.bus.mipi_csi2.lane_polarities[0];
> - dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> - buscfg->bus.csi2.lanecfg.clk.pol,
> - buscfg->bus.csi2.lanecfg.clk.pos);
> -
> - for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> - buscfg->bus.csi2.lanecfg.data[i].pos =
> - vep.bus.mipi_csi2.data_lanes[i];
> - buscfg->bus.csi2.lanecfg.data[i].pol =
> - vep.bus.mipi_csi2.lane_polarities[i + 1];
> - dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> - buscfg->bus.csi2.lanecfg.data[i].pol,
> - buscfg->bus.csi2.lanecfg.data[i].pos);
> - }
> -
> - /*
> - * FIXME: now we assume the CRC is always there.
> - * Implement a way to obtain this information from the
> - * sensor. Frame descriptors, perhaps?
> - */
> - buscfg->bus.csi2.crc = 1;
> + if (vep.bus_type == V4L2_MBUS_CSI2)
> + isp_of_parse_node_csi2(dev, buscfg, &vep);
> + else
> + isp_of_parse_node_csi1(dev, buscfg, &vep);
> break;
>
> default:
> + return -1;
Please use the appropriate error code.
> + }
> + return 0;
> +}
> +
> +static int isp_of_parse_node_endpoint(struct device *dev,
> + struct device_node *node,
> + struct isp_async_subdev *isd)
> +{
> + struct isp_bus_cfg *buscfg;
> + struct v4l2_of_endpoint vep;
> + int ret;
> +
> + isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
Why do you now need to allocate this manually ?
> + if (!isd->bus)
> + return -ENOMEM;
> +
> + buscfg = isd->bus;
> +
> + ret = (node, &vep);
> + if (ret)
> + return ret;
> +
> + dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> + vep.base.port);
> +
> + if (isp_endpoint_to_buscfg(dev, vep, buscfg))
I'm fine splitting the CSI1/CSI2 parsing code to separate functions, but I
don't think there's a need to split isp_endpoint_to_buscfg(). You can keep
that part inline.
> dev_warn(dev, "%s: invalid interface %u\n", node->full_name,
> vep.base.port);
> - break;
> - }
>
> return 0;
> }
> @@ -2124,7 +2192,7 @@ static int isp_of_parse_nodes(struct device *dev,
>
> notifier->subdevs[notifier->num_subdevs] = &isd->asd;
>
> - if (isp_of_parse_node(dev, node, isd)) {
> + if (isp_of_parse_node_endpoint(dev, node, isd)) {
> of_node_put(node);
> return -EINVAL;
> }
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index 7e6f663..c0b9d1d 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -228,7 +228,7 @@ struct isp_device {
>
> struct isp_async_subdev {
> struct v4l2_subdev *sd;
> - struct isp_bus_cfg bus;
> + struct isp_bus_cfg *bus;
> struct v4l2_async_subdev asd;
> };
>
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -21,6 +21,9 @@
> #include <linux/mutex.h>
> #include <linux/uaccess.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/regmap.h>
> +
> +#include <media/v4l2-of.h>
>
> #include "isp.h"
> #include "ispreg.h"
> @@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device *ccp2,
> u8 enable) return ret;
> }
>
> + if (isp->revision == ISP_REVISION_2_0) {
The isp_csiphy.c code checks phy->isp->phy_type for the same purpose,
shouldn't you use that too ?
> + struct media_pad *pad;
> + struct v4l2_subdev *sensor;
> + const struct isp_ccp2_cfg *buscfg;
> + u32 csirxfe;
> +
> + pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> + sensor = media_entity_to_v4l2_subdev(pad->entity);
> + /* Struct isp_bus_cfg has union inside */
> + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> +
> +
One blank line is enough.
> + if (enable) {
> + csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> + OMAP343X_CONTROL_CSIRXFE_RESET;
> +
> + if (buscfg->phy_layer)
> + csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> +
> + if (buscfg->strobe_clk_pol)
> + csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
> + } else
> + csirxfe = 0;
You need curly braces for the else statement too.
> +
> + regmap_write(isp->syscon, isp->syscon_offset, csirxfe);
Isn't this already configured by csiphy_routing_cfg_3430(), called through
omap3isp_csiphy_acquire() ? You'll need to add support for the strobe/clock
polarity there, but the rest should already be handled.
> + }
> +
> /* Enable/Disable all the LCx channels */
> for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
> isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2,
ISPCCP2_LCx_CTRL(i),
> @@ -213,14 +243,17 @@ static int ccp2_phyif_config(struct isp_ccp2_device
> *ccp2, struct isp_device *isp = to_isp_device(ccp2);
> u32 val;
>
> - /* CCP2B mode */
> val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL) |
> - ISPCCP2_CTRL_IO_OUT_SEL | ISPCCP2_CTRL_MODE;
> + ISPCCP2_CTRL_MODE;
> /* Data/strobe physical layer */
> BIT_SET(val, ISPCCP2_CTRL_PHY_SEL_SHIFT, ISPCCP2_CTRL_PHY_SEL_MASK,
> buscfg->phy_layer);
> + BIT_SET(val, ISPCCP2_CTRL_IO_OUT_SEL_SHIFT,
> + ISPCCP2_CTRL_IO_OUT_SEL_MASK, buscfg->ccp2_mode);
> BIT_SET(val, ISPCCP2_CTRL_INV_SHIFT, ISPCCP2_CTRL_INV_MASK,
> buscfg->strobe_clk_pol);
> + BIT_SET(val, ISPCCP2_CTRL_VP_CLK_POL_SHIFT,
> + ISPCCP2_CTRL_VP_CLK_POL_MASK, buscfg->vp_clk_pol);
> isp_reg_writel(isp, val, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL);
>
> val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL);
> @@ -339,6 +372,9 @@ static void ccp2_lcx_config(struct isp_ccp2_device
> *ccp2, isp_reg_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LC01_IRQENABLE, val);
> }
>
> +void __isp_of_parse_node_csi1(struct device *dev,
> + struct isp_ccp2_cfg *buscfg,
> + struct v4l2_of_endpoint *vep);
This isn't needed.
> /*
> * ccp2_if_configure - Configure ccp2 with data from sensor
> * @ccp2: Pointer to ISP CCP2 device
> @@ -1137,6 +1173,8 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> if (isp->revision == ISP_REVISION_2_0) {
> ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> if (IS_ERR(ccp2->vdds_csib)) {
> + if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> dev_dbg(isp->dev,
> "Could not get regulator vdds_csib\n");
> ccp2->vdds_csib = NULL;
> diff --git a/drivers/media/platform/omap3isp/ispreg.h
> b/drivers/media/platform/omap3isp/ispreg.h index b5ea8da..d084839 100644
> --- a/drivers/media/platform/omap3isp/ispreg.h
> +++ b/drivers/media/platform/omap3isp/ispreg.h
> @@ -87,6 +87,8 @@
> #define ISPCCP2_CTRL_PHY_SEL_MASK 0x1
> #define ISPCCP2_CTRL_PHY_SEL_SHIFT 1
> #define ISPCCP2_CTRL_IO_OUT_SEL (1 << 2)
> +#define ISPCCP2_CTRL_IO_OUT_SEL_MASK 0x1
> +#define ISPCCP2_CTRL_IO_OUT_SEL_SHIFT 2
> #define ISPCCP2_CTRL_MODE (1 << 4)
> #define ISPCCP2_CTRL_VP_CLK_FORCE_ON (1 << 9)
> #define ISPCCP2_CTRL_INV (1 << 10)
> @@ -94,6 +96,8 @@
> #define ISPCCP2_CTRL_INV_SHIFT 10
> #define ISPCCP2_CTRL_VP_ONLY_EN (1 << 11)
> #define ISPCCP2_CTRL_VP_CLK_POL (1 << 12)
> +#define ISPCCP2_CTRL_VP_CLK_POL_MASK 0x1
> +#define ISPCCP2_CTRL_VP_CLK_POL_SHIFT 12
> #define ISPCCP2_CTRL_VPCLK_DIV_SHIFT 15
> #define ISPCCP2_CTRL_VPCLK_DIV_MASK 0x1ffff /* [31:15] */
> #define ISPCCP2_CTRL_VP_OUT_CTRL_SHIFT 8 /* 3430 bits */
> diff --git a/drivers/media/platform/omap3isp/omap3isp.h
> b/drivers/media/platform/omap3isp/omap3isp.h index 443e8f7..f6d1d0d 100644
> --- a/drivers/media/platform/omap3isp/omap3isp.h
> +++ b/drivers/media/platform/omap3isp/omap3isp.h
> @@ -108,6 +108,7 @@ struct isp_ccp2_cfg {
> unsigned int ccp2_mode:1;
> unsigned int phy_layer:1;
> unsigned int vpclk_div:2;
> + unsigned int vp_clk_pol:1;
> struct isp_csiphy_lanes_cfg lanecfg;
> };
>
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 34cc99e..315c167 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
You should split the V4L2 core changes to a separate patch.
> @@ -69,11 +69,15 @@
> * @V4L2_MBUS_PARALLEL: parallel interface with hsync and vsync
> * @V4L2_MBUS_BT656: parallel interface with embedded synchronisation, can
> * also be used for BT.1120
> + * @V4L2_MBUS_CSI1: MIPI CSI-1 serial interface
> + * @V4L2_MBUS_CCP2: CCP2 (Compact Camera Port 2)
It would help if you could provide, in comments or in the commit message, a
few pointers to information about CSI-1 and CCP2.
> * @V4L2_MBUS_CSI2: MIPI CSI-2 serial interface
> */
> enum v4l2_mbus_type {
> V4L2_MBUS_PARALLEL,
> V4L2_MBUS_BT656,
> + V4L2_MBUS_CSI1,
> + V4L2_MBUS_CCP2,
That's nice, but v4l2_of_parse_endpoint() never sets the bus type to CSI1 or
CCP2. You need to fix that (and update the related DT bindings).
> V4L2_MBUS_CSI2,
> };
>
> diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
> index 4dc34b2..63a52ee 100644
> --- a/include/media/v4l2-of.h
> +++ b/include/media/v4l2-of.h
> @@ -53,6 +53,22 @@ struct v4l2_of_bus_parallel {
> };
>
> /**
> + * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
> + * @clock_inv: polarity of clock/strobe signal
> + * false - not inverted, true - inverted
> + * @strobe: false - data/clock, true - data/strobe
> + * @data_lane: the number of the data lane
> + * @clock_lane: the number of the clock lane
> + */
> +struct v4l2_of_bus_mipi_csi1 {
> + bool clock_inv;
> + bool strobe;
> + bool lane_polarity[2];
This field isn't documented.
> + unsigned char data_lane;
> + unsigned char clock_lane;
> +};
> +
> +/**
> * struct v4l2_of_endpoint - the endpoint data structure
> * @base: struct of_endpoint containing port, id, and local of_node
> * @bus_type: bus type
> @@ -66,6 +82,7 @@ struct v4l2_of_endpoint {
> enum v4l2_mbus_type bus_type;
> union {
> struct v4l2_of_bus_parallel parallel;
> + struct v4l2_of_bus_mipi_csi1 mipi_csi1;
> struct v4l2_of_bus_mipi_csi2 mipi_csi2;
> } bus;
> u64 *link_frequencies;
Hi!
> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2024,21 +2024,92 @@ enum isp_of_phy {
> > ISP_OF_PHY_CSIPHY2,
> > };
> >
> > -static int isp_of_parse_node(struct device *dev, struct device_node *node,
> > - struct isp_async_subdev *isd)
> > +void __isp_of_parse_node_csi1(struct device *dev,
> > + struct isp_ccp2_cfg *buscfg,
> > + struct v4l2_of_endpoint *vep)
>
> This function isn't use anywhere else, you can merge it with
> isp_of_parse_node_csi1().
I'd prefer not to. First, it will be used separately in future, and
second, expresions would be uglier.
> > +{
> > + buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
> > + buscfg->lanecfg.clk.pol =
> > + vep->bus.mipi_csi1.lane_polarity[0];
> > + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> > + buscfg->lanecfg.clk.pol,
> > + buscfg->lanecfg.clk.pos);
> > +
> > + buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
> > + buscfg->lanecfg.data[0].pol =
> > + vep->bus.mipi_csi2.lane_polarities[1];
>
> bus.mipi_csi2 ?
Good catch. Fixed.
> > - ret = v4l2_of_parse_endpoint(node, &vep);
> > - if (ret)
> > - return ret;
> > + if (vep->base.port == ISP_OF_PHY_CSIPHY1)
> > + buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> > + else
> > + buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
>
> I would keep this code in the caller to avoid code duplication with
> isp_of_parse_node_csi1().
Take a closer look. Code in _csi1 is different.
> > break;
> >
> > default:
> > + return -1;
>
> Please use the appropriate error code.
Ok.
> > + return 0;
> > +}
> > +
> > +static int isp_of_parse_node_endpoint(struct device *dev,
> > + struct device_node *node,
> > + struct isp_async_subdev *isd)
> > +{
> > + struct isp_bus_cfg *buscfg;
> > + struct v4l2_of_endpoint vep;
> > + int ret;
> > +
> > + isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
>
> Why do you now need to allocate this manually ?
bus is now a pointer.
> > + dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> > + vep.base.port);
> > +
> > + if (isp_endpoint_to_buscfg(dev, vep, buscfg))
>
> I'm fine splitting the CSI1/CSI2 parsing code to separate functions, but I
> don't think there's a need to split isp_endpoint_to_buscfg(). You can keep
> that part inline.
I'd prefer smaller functions here. I tried to read the original and it
was not too easy.
> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device *ccp2,
> > u8 enable) return ret;
> > }
> >
> > + if (isp->revision == ISP_REVISION_2_0) {
>
> The isp_csiphy.c code checks phy->isp->phy_type for the same purpose,
> shouldn't you use that too ?
Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check
here? Can do...
> > + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> > +
> > +
>
> One blank line is enough.
Ok.
> > + if (enable) {
> > + csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> > + OMAP343X_CONTROL_CSIRXFE_RESET;
> > +
> > + if (buscfg->phy_layer)
> > + csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> > +
> > + if (buscfg->strobe_clk_pol)
> > + csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
> > + } else
> > + csirxfe = 0;
>
> You need curly braces for the else statement too.
Easy enough.
> > +
> > + regmap_write(isp->syscon, isp->syscon_offset, csirxfe);
>
> Isn't this already configured by csiphy_routing_cfg_3430(), called through
> omap3isp_csiphy_acquire() ? You'll need to add support for the strobe/clock
> polarity there, but the rest should already be handled.
Let me check...
> > @@ -69,11 +69,15 @@
> > * @V4L2_MBUS_PARALLEL: parallel interface with hsync and vsync
> > * @V4L2_MBUS_BT656: parallel interface with embedded synchronisation, can
> > * also be used for BT.1120
> > + * @V4L2_MBUS_CSI1: MIPI CSI-1 serial interface
> > + * @V4L2_MBUS_CCP2: CCP2 (Compact Camera Port 2)
>
> It would help if you could provide, in comments or in the commit message, a
> few pointers to information about CSI-1 and CCP2.
There's not much good information :-(.
http://electronics.stackexchange.com/questions/134395/differences-between-mipi-csi1-and-mipi-csi2
>
> > /**
> > + * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
> > + * @clock_inv: polarity of clock/strobe signal
> > + * false - not inverted, true - inverted
> > + * @strobe: false - data/clock, true - data/strobe
> > + * @data_lane: the number of the data lane
> > + * @clock_lane: the number of the clock lane
> > + */
> > +struct v4l2_of_bus_mipi_csi1 {
> > + bool clock_inv;
> > + bool strobe;
> > + bool lane_polarity[2];
>
> This field isn't documented.
Yep, automatic checker already told me. Plus, similar field elsewhere
is called "lane_polarities" but I believe "polarity" is a better name.
Pavel
Hi Pavel,
On Wednesday 15 Feb 2017 10:42:29 Pavel Machek wrote:
> Hi!
>
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c index 0321d84..88bc7c6 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -2024,21 +2024,92 @@ enum isp_of_phy {
> >> ISP_OF_PHY_CSIPHY2,
> >> };
> >>
> >> -static int isp_of_parse_node(struct device *dev, struct device_node
> >> *node,
> >> - struct isp_async_subdev *isd)
> >> +void __isp_of_parse_node_csi1(struct device *dev,
> >> + struct isp_ccp2_cfg *buscfg,
> >> + struct v4l2_of_endpoint *vep)
> >
> > This function isn't use anywhere else, you can merge it with
> > isp_of_parse_node_csi1().
>
> I'd prefer not to. First, it will be used separately in future, and
> second, expresions would be uglier.
Where will it be used ? As for the uglier part, I don't agree, otherwise I
wouldn't have proposed it.
> >> +{
> >> + buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
> >> + buscfg->lanecfg.clk.pol =
> >> + vep->bus.mipi_csi1.lane_polarity[0];
> >> + dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> >> + buscfg->lanecfg.clk.pol,
> >> + buscfg->lanecfg.clk.pos);
> >> +
> >> + buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
> >> + buscfg->lanecfg.data[0].pol =
> >> + vep->bus.mipi_csi2.lane_polarities[1];
> >
> > bus.mipi_csi2 ?
>
> Good catch. Fixed.
>
> >> - ret = v4l2_of_parse_endpoint(node, &vep);
> >> - if (ret)
> >> - return ret;
> >> + if (vep->base.port == ISP_OF_PHY_CSIPHY1)
> >> + buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> >> + else
> >> + buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> >
> > I would keep this code in the caller to avoid code duplication with
> > isp_of_parse_node_csi1().
>
> Take a closer look. Code in _csi1 is different.
>
> >> break;
> >>
> >> default:
> >> + return -1;
> >
> > Please use the appropriate error code.
>
> Ok.
>
> >> + return 0;
> >> +}
> >> +
> >> +static int isp_of_parse_node_endpoint(struct device *dev,
> >> + struct device_node *node,
> >> + struct isp_async_subdev *isd)
> >> +{
> >> + struct isp_bus_cfg *buscfg;
> >> + struct v4l2_of_endpoint vep;
> >> + int ret;
> >> +
> >> + isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
> >
> > Why do you now need to allocate this manually ?
>
> bus is now a pointer.
I've seen that, but why have you changed it ?
> >> + dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
> >> + vep.base.port);
> >> +
> >> + if (isp_endpoint_to_buscfg(dev, vep, buscfg))
> >
> > I'm fine splitting the CSI1/CSI2 parsing code to separate functions, but I
> > don't think there's a need to split isp_endpoint_to_buscfg(). You can keep
> > that part inline.
>
> I'd prefer smaller functions here. I tried to read the original and it
> was not too easy.
This function became a kzalloc (which I still don't see why you need it), a
call to v4l2_of_parse_endpoint(), and then isp_endpoint_to_buscfg(). That's
too small to be a function of its own. Please merge
isp_of_parse_node_endpoint() and isp_endpoint_to_buscfg().
> >> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> >> b/drivers/media/platform/omap3isp/ispccp2.c index ca09523..4edb55a
> >> 100644
> >> --- a/drivers/media/platform/omap3isp/ispccp2.c
> >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> >> @@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
> >> *ccp2, u8 enable) return ret;
> >>
> >> }
> >>
> >> + if (isp->revision == ISP_REVISION_2_0) {
> >
> > The isp_csiphy.c code checks phy->isp->phy_type for the same purpose,
> > shouldn't you use that too ?
>
> Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check
> here? Can do...
Yes that's what I meant.
> >> + buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
> >> +
> >> +
> >
> > One blank line is enough.
>
> Ok.
>
> >> + if (enable) {
> >> + csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
> >> + OMAP343X_CONTROL_CSIRXFE_RESET;
> >> +
> >> + if (buscfg->phy_layer)
> >> + csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
> >> +
> >> + if (buscfg->strobe_clk_pol)
> >> + csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
> >> + } else
> >> + csirxfe = 0;
> >
> > You need curly braces for the else statement too.
>
> Easy enough.
>
> >> +
> >> + regmap_write(isp->syscon, isp->syscon_offset, csirxfe);
> >
> > Isn't this already configured by csiphy_routing_cfg_3430(), called through
> > omap3isp_csiphy_acquire() ? You'll need to add support for the
> > strobe/clock polarity there, but the rest should already be handled.
>
> Let me check...
>
> >> @@ -69,11 +69,15 @@
> >> * @V4L2_MBUS_PARALLEL: parallel interface with hsync and vsync
> >> * @V4L2_MBUS_BT656: parallel interface with embedded
> >> synchronisation, can
> >> * also be used for BT.1120
> > > + * @V4L2_MBUS_CSI1: MIPI CSI-1 serial interface
> > > + * @V4L2_MBUS_CCP2: CCP2 (Compact Camera Port 2)
> >
> > It would help if you could provide, in comments or in the commit message,
> > a few pointers to information about CSI-1 and CCP2.
>
> There's not much good information :-(.
>
> http://electronics.stackexchange.com/questions/134395/differences-between-mi
> pi-csi1-and-mipi-csi2
> >> /**
> >>
> >> + * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
> >> + * @clock_inv: polarity of clock/strobe signal
> >> + * false - not inverted, true - inverted
> >> + * @strobe: false - data/clock, true - data/strobe
> >> + * @data_lane: the number of the data lane
> >> + * @clock_lane: the number of the clock lane
> >> + */
> >> +struct v4l2_of_bus_mipi_csi1 {
> >> + bool clock_inv;
> >> + bool strobe;
> >> + bool lane_polarity[2];
> >
> > This field isn't documented.
>
> Yep, automatic checker already told me. Plus, similar field elsewhere
> is called "lane_polarities" but I believe "polarity" is a better name.
Hi!
> > >> +
> > >> +static int isp_of_parse_node_endpoint(struct device *dev,
> > >> + struct device_node *node,
> > >> + struct isp_async_subdev *isd)
> > >> +{
> > >> + struct isp_bus_cfg *buscfg;
> > >> + struct v4l2_of_endpoint vep;
> > >> + int ret;
> > >> +
> > >> + isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
> > >
> > > Why do you now need to allocate this manually ?
> >
> > bus is now a pointer.
>
> I've seen that, but why have you changed it ?
subdev support. Needs to go into separate patch. Will be done shortly.
> > >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > >> @@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device
> > >> *ccp2, u8 enable) return ret;
> > >>
> > >> }
> > >>
> > >> + if (isp->revision == ISP_REVISION_2_0) {
> > >
> > > The isp_csiphy.c code checks phy->isp->phy_type for the same purpose,
> > > shouldn't you use that too ?
> >
> > Do you want me to do phy->isp->phy_type == ISP_PHY_TYPE_3430 check
> > here? Can do...
>
> Yes that's what I meant.
Ok, that's something I can do.
But code is still somewhat "interesting". Code in omap3isp_csiphy_acquire()
assumes csi2, and I don't need most of it.. so I'll just not use it,
but it looks strange. I'll post new patch shortly.
Best regards,
Pavel
@@ -2024,21 +2024,92 @@ enum isp_of_phy {
ISP_OF_PHY_CSIPHY2,
};
-static int isp_of_parse_node(struct device *dev, struct device_node *node,
- struct isp_async_subdev *isd)
+void __isp_of_parse_node_csi1(struct device *dev,
+ struct isp_ccp2_cfg *buscfg,
+ struct v4l2_of_endpoint *vep)
+{
+ buscfg->lanecfg.clk.pos = vep->bus.mipi_csi1.clock_lane;
+ buscfg->lanecfg.clk.pol =
+ vep->bus.mipi_csi1.lane_polarity[0];
+ dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+ buscfg->lanecfg.clk.pol,
+ buscfg->lanecfg.clk.pos);
+
+ buscfg->lanecfg.data[0].pos = vep->bus.mipi_csi2.data_lanes[0];
+ buscfg->lanecfg.data[0].pol =
+ vep->bus.mipi_csi2.lane_polarities[1];
+ dev_dbg(dev, "data lane polarity %u, pos %u\n",
+ buscfg->lanecfg.data[0].pol,
+ buscfg->lanecfg.data[0].pos);
+
+ buscfg->strobe_clk_pol = vep->bus.mipi_csi1.clock_inv;
+ buscfg->phy_layer = vep->bus.mipi_csi1.strobe;
+ buscfg->ccp2_mode = vep->bus_type == V4L2_MBUS_CCP2;
+
+ dev_dbg(dev, "clock_inv %u strobe %u ccp2 %u\n",
+ buscfg->strobe_clk_pol,
+ buscfg->phy_layer,
+ buscfg->ccp2_mode);
+ /*
+ * FIXME: now we assume the CRC is always there.
+ * Implement a way to obtain this information from the
+ * sensor. Frame descriptors, perhaps?
+ */
+ buscfg->crc = 1;
+
+ buscfg->vp_clk_pol = 1;
+}
+
+static void isp_of_parse_node_csi1(struct device *dev,
+ struct isp_bus_cfg *buscfg,
+ struct v4l2_of_endpoint *vep)
+{
+ if (vep->base.port == ISP_OF_PHY_CSIPHY1)
+ buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
+ else
+ buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
+ __isp_of_parse_node_csi1(dev, &buscfg->bus.ccp2, vep);
+}
+
+static void isp_of_parse_node_csi2(struct device *dev,
+ struct isp_bus_cfg *buscfg,
+ struct v4l2_of_endpoint *vep)
{
- struct isp_bus_cfg *buscfg = &isd->bus;
- struct v4l2_of_endpoint vep;
unsigned int i;
- int ret;
- ret = v4l2_of_parse_endpoint(node, &vep);
- if (ret)
- return ret;
+ if (vep->base.port == ISP_OF_PHY_CSIPHY1)
+ buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
+ else
+ buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
+ buscfg->bus.csi2.lanecfg.clk.pos = vep->bus.mipi_csi2.clock_lane;
+ buscfg->bus.csi2.lanecfg.clk.pol =
+ vep->bus.mipi_csi2.lane_polarities[0];
+ dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+ buscfg->bus.csi2.lanecfg.clk.pol,
+ buscfg->bus.csi2.lanecfg.clk.pos);
+
+ for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
+ buscfg->bus.csi2.lanecfg.data[i].pos =
+ vep->bus.mipi_csi2.data_lanes[i];
+ buscfg->bus.csi2.lanecfg.data[i].pol =
+ vep->bus.mipi_csi2.lane_polarities[i + 1];
+ dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
+ buscfg->bus.csi2.lanecfg.data[i].pol,
+ buscfg->bus.csi2.lanecfg.data[i].pos);
+ }
- dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
- vep.base.port);
+ /*
+ * FIXME: now we assume the CRC is always there.
+ * Implement a way to obtain this information from the
+ * sensor. Frame descriptors, perhaps?
+ */
+ buscfg->bus.csi2.crc = 1;
+}
+static int isp_endpoint_to_buscfg(struct device *dev,
+ struct v4l2_of_endpoint vep,
+ struct isp_bus_cfg *buscfg)
+{
switch (vep.base.port) {
case ISP_OF_PHY_PARALLEL:
buscfg->interface = ISP_INTERFACE_PARALLEL;
@@ -2059,45 +2130,42 @@ static int isp_of_parse_node(struct device *dev, struct device_node *node,
case ISP_OF_PHY_CSIPHY1:
case ISP_OF_PHY_CSIPHY2:
- /* FIXME: always assume CSI-2 for now. */
- switch (vep.base.port) {
- case ISP_OF_PHY_CSIPHY1:
- buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
- break;
- case ISP_OF_PHY_CSIPHY2:
- buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
- break;
- }
- buscfg->bus.csi2.lanecfg.clk.pos = vep.bus.mipi_csi2.clock_lane;
- buscfg->bus.csi2.lanecfg.clk.pol =
- vep.bus.mipi_csi2.lane_polarities[0];
- dev_dbg(dev, "clock lane polarity %u, pos %u\n",
- buscfg->bus.csi2.lanecfg.clk.pol,
- buscfg->bus.csi2.lanecfg.clk.pos);
-
- for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
- buscfg->bus.csi2.lanecfg.data[i].pos =
- vep.bus.mipi_csi2.data_lanes[i];
- buscfg->bus.csi2.lanecfg.data[i].pol =
- vep.bus.mipi_csi2.lane_polarities[i + 1];
- dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
- buscfg->bus.csi2.lanecfg.data[i].pol,
- buscfg->bus.csi2.lanecfg.data[i].pos);
- }
-
- /*
- * FIXME: now we assume the CRC is always there.
- * Implement a way to obtain this information from the
- * sensor. Frame descriptors, perhaps?
- */
- buscfg->bus.csi2.crc = 1;
+ if (vep.bus_type == V4L2_MBUS_CSI2)
+ isp_of_parse_node_csi2(dev, buscfg, &vep);
+ else
+ isp_of_parse_node_csi1(dev, buscfg, &vep);
break;
default:
+ return -1;
+ }
+ return 0;
+}
+
+static int isp_of_parse_node_endpoint(struct device *dev,
+ struct device_node *node,
+ struct isp_async_subdev *isd)
+{
+ struct isp_bus_cfg *buscfg;
+ struct v4l2_of_endpoint vep;
+ int ret;
+
+ isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
+ if (!isd->bus)
+ return -ENOMEM;
+
+ buscfg = isd->bus;
+
+ ret = v4l2_of_parse_endpoint(node, &vep);
+ if (ret)
+ return ret;
+
+ dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
+ vep.base.port);
+
+ if (isp_endpoint_to_buscfg(dev, vep, buscfg))
dev_warn(dev, "%s: invalid interface %u\n", node->full_name,
vep.base.port);
- break;
- }
return 0;
}
@@ -2124,7 +2192,7 @@ static int isp_of_parse_nodes(struct device *dev,
notifier->subdevs[notifier->num_subdevs] = &isd->asd;
- if (isp_of_parse_node(dev, node, isd)) {
+ if (isp_of_parse_node_endpoint(dev, node, isd)) {
of_node_put(node);
return -EINVAL;
}
@@ -228,7 +228,7 @@ struct isp_device {
struct isp_async_subdev {
struct v4l2_subdev *sd;
- struct isp_bus_cfg bus;
+ struct isp_bus_cfg *bus;
struct v4l2_async_subdev asd;
};
@@ -21,6 +21,9 @@
#include <linux/mutex.h>
#include <linux/uaccess.h>
#include <linux/regulator/consumer.h>
+#include <linux/regmap.h>
+
+#include <media/v4l2-of.h>
#include "isp.h"
#include "ispreg.h"
@@ -160,6 +163,33 @@ static int ccp2_if_enable(struct isp_ccp2_device *ccp2, u8 enable)
return ret;
}
+ if (isp->revision == ISP_REVISION_2_0) {
+ struct media_pad *pad;
+ struct v4l2_subdev *sensor;
+ const struct isp_ccp2_cfg *buscfg;
+ u32 csirxfe;
+
+ pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
+ sensor = media_entity_to_v4l2_subdev(pad->entity);
+ /* Struct isp_bus_cfg has union inside */
+ buscfg = &((struct isp_bus_cfg *)sensor->host_priv)->bus.ccp2;
+
+
+ if (enable) {
+ csirxfe = OMAP343X_CONTROL_CSIRXFE_PWRDNZ |
+ OMAP343X_CONTROL_CSIRXFE_RESET;
+
+ if (buscfg->phy_layer)
+ csirxfe |= OMAP343X_CONTROL_CSIRXFE_SELFORM;
+
+ if (buscfg->strobe_clk_pol)
+ csirxfe |= OMAP343X_CONTROL_CSIRXFE_CSIB_INV;
+ } else
+ csirxfe = 0;
+
+ regmap_write(isp->syscon, isp->syscon_offset, csirxfe);
+ }
+
/* Enable/Disable all the LCx channels */
for (i = 0; i < CCP2_LCx_CHANS_NUM; i++)
isp_reg_clr_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LCx_CTRL(i),
@@ -213,14 +243,17 @@ static int ccp2_phyif_config(struct isp_ccp2_device *ccp2,
struct isp_device *isp = to_isp_device(ccp2);
u32 val;
- /* CCP2B mode */
val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL) |
- ISPCCP2_CTRL_IO_OUT_SEL | ISPCCP2_CTRL_MODE;
+ ISPCCP2_CTRL_MODE;
/* Data/strobe physical layer */
BIT_SET(val, ISPCCP2_CTRL_PHY_SEL_SHIFT, ISPCCP2_CTRL_PHY_SEL_MASK,
buscfg->phy_layer);
+ BIT_SET(val, ISPCCP2_CTRL_IO_OUT_SEL_SHIFT,
+ ISPCCP2_CTRL_IO_OUT_SEL_MASK, buscfg->ccp2_mode);
BIT_SET(val, ISPCCP2_CTRL_INV_SHIFT, ISPCCP2_CTRL_INV_MASK,
buscfg->strobe_clk_pol);
+ BIT_SET(val, ISPCCP2_CTRL_VP_CLK_POL_SHIFT,
+ ISPCCP2_CTRL_VP_CLK_POL_MASK, buscfg->vp_clk_pol);
isp_reg_writel(isp, val, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL);
val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL);
@@ -339,6 +372,9 @@ static void ccp2_lcx_config(struct isp_ccp2_device *ccp2,
isp_reg_set(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_LC01_IRQENABLE, val);
}
+void __isp_of_parse_node_csi1(struct device *dev,
+ struct isp_ccp2_cfg *buscfg,
+ struct v4l2_of_endpoint *vep);
/*
* ccp2_if_configure - Configure ccp2 with data from sensor
* @ccp2: Pointer to ISP CCP2 device
@@ -1137,6 +1173,8 @@ int omap3isp_ccp2_init(struct isp_device *isp)
if (isp->revision == ISP_REVISION_2_0) {
ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
if (IS_ERR(ccp2->vdds_csib)) {
+ if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
dev_dbg(isp->dev,
"Could not get regulator vdds_csib\n");
ccp2->vdds_csib = NULL;
@@ -87,6 +87,8 @@
#define ISPCCP2_CTRL_PHY_SEL_MASK 0x1
#define ISPCCP2_CTRL_PHY_SEL_SHIFT 1
#define ISPCCP2_CTRL_IO_OUT_SEL (1 << 2)
+#define ISPCCP2_CTRL_IO_OUT_SEL_MASK 0x1
+#define ISPCCP2_CTRL_IO_OUT_SEL_SHIFT 2
#define ISPCCP2_CTRL_MODE (1 << 4)
#define ISPCCP2_CTRL_VP_CLK_FORCE_ON (1 << 9)
#define ISPCCP2_CTRL_INV (1 << 10)
@@ -94,6 +96,8 @@
#define ISPCCP2_CTRL_INV_SHIFT 10
#define ISPCCP2_CTRL_VP_ONLY_EN (1 << 11)
#define ISPCCP2_CTRL_VP_CLK_POL (1 << 12)
+#define ISPCCP2_CTRL_VP_CLK_POL_MASK 0x1
+#define ISPCCP2_CTRL_VP_CLK_POL_SHIFT 12
#define ISPCCP2_CTRL_VPCLK_DIV_SHIFT 15
#define ISPCCP2_CTRL_VPCLK_DIV_MASK 0x1ffff /* [31:15] */
#define ISPCCP2_CTRL_VP_OUT_CTRL_SHIFT 8 /* 3430 bits */
@@ -108,6 +108,7 @@ struct isp_ccp2_cfg {
unsigned int ccp2_mode:1;
unsigned int phy_layer:1;
unsigned int vpclk_div:2;
+ unsigned int vp_clk_pol:1;
struct isp_csiphy_lanes_cfg lanecfg;
};
@@ -69,11 +69,15 @@
* @V4L2_MBUS_PARALLEL: parallel interface with hsync and vsync
* @V4L2_MBUS_BT656: parallel interface with embedded synchronisation, can
* also be used for BT.1120
+ * @V4L2_MBUS_CSI1: MIPI CSI-1 serial interface
+ * @V4L2_MBUS_CCP2: CCP2 (Compact Camera Port 2)
* @V4L2_MBUS_CSI2: MIPI CSI-2 serial interface
*/
enum v4l2_mbus_type {
V4L2_MBUS_PARALLEL,
V4L2_MBUS_BT656,
+ V4L2_MBUS_CSI1,
+ V4L2_MBUS_CCP2,
V4L2_MBUS_CSI2,
};
@@ -53,6 +53,22 @@ struct v4l2_of_bus_parallel {
};
/**
+ * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
+ * @clock_inv: polarity of clock/strobe signal
+ * false - not inverted, true - inverted
+ * @strobe: false - data/clock, true - data/strobe
+ * @data_lane: the number of the data lane
+ * @clock_lane: the number of the clock lane
+ */
+struct v4l2_of_bus_mipi_csi1 {
+ bool clock_inv;
+ bool strobe;
+ bool lane_polarity[2];
+ unsigned char data_lane;
+ unsigned char clock_lane;
+};
+
+/**
* struct v4l2_of_endpoint - the endpoint data structure
* @base: struct of_endpoint containing port, id, and local of_node
* @bus_type: bus type
@@ -66,6 +82,7 @@ struct v4l2_of_endpoint {
enum v4l2_mbus_type bus_type;
union {
struct v4l2_of_bus_parallel parallel;
+ struct v4l2_of_bus_mipi_csi1 mipi_csi1;
struct v4l2_of_bus_mipi_csi2 mipi_csi2;
} bus;
u64 *link_frequencies;