Re: [PATCH 08/43] imx-drm: ipu-v3: Add units required for video capture

[Sascha Hauer <s.hauer@pengutronix.de>]

On Sat, Jun 07, 2014 at 02:56:10PM -0700, Steve Longerbeam wrote:
> Adds the following new IPU units:
> 
> - Camera Sensor Interface (csi)
> - Sensor Multi FIFO Controller (smfc)
> - Image Converter (ic)
> - Image Rotator (irt)
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/staging/imx-drm/ipu-v3/Makefile     |    3 +-
>  drivers/staging/imx-drm/ipu-v3/ipu-common.c |   67 ++-
>  drivers/staging/imx-drm/ipu-v3/ipu-csi.c    |  821 ++++++++++++++++++++++++++
>  drivers/staging/imx-drm/ipu-v3/ipu-ic.c     |  835 +++++++++++++++++++++++++++
>  drivers/staging/imx-drm/ipu-v3/ipu-irt.c    |  103 ++++
>  drivers/staging/imx-drm/ipu-v3/ipu-prv.h    |   24 +
>  drivers/staging/imx-drm/ipu-v3/ipu-smfc.c   |  348 +++++++++++
>  include/linux/platform_data/imx-ipu-v3.h    |  151 +++++
>  8 files changed, 2350 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-csi.c
>  create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-ic.c
>  create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-irt.c
>  create mode 100644 drivers/staging/imx-drm/ipu-v3/ipu-smfc.c
> 
> diff --git a/drivers/staging/imx-drm/ipu-v3/Makefile b/drivers/staging/imx-drm/ipu-v3/Makefile
> index 28ed72e..79c0c88 100644
> --- a/drivers/staging/imx-drm/ipu-v3/Makefile
> +++ b/drivers/staging/imx-drm/ipu-v3/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_DRM_IMX_IPUV3_CORE) += imx-ipu-v3.o
>  
> -imx-ipu-v3-objs := ipu-common.o ipu-dc.o ipu-di.o ipu-dp.o ipu-dmfc.o
> +imx-ipu-v3-objs := ipu-common.o ipu-csi.o ipu-dc.o ipu-di.o \
> +	ipu-dp.o ipu-dmfc.o ipu-ic.o ipu-irt.o ipu-smfc.o
> diff --git a/drivers/staging/imx-drm/ipu-v3/ipu-common.c b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> index 1c606b5..3d7e28d 100644
> --- a/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> +++ b/drivers/staging/imx-drm/ipu-v3/ipu-common.c
> @@ -834,6 +834,10 @@ struct ipu_devtype {
>  	unsigned long cpmem_ofs;
>  	unsigned long srm_ofs;
>  	unsigned long tpm_ofs;
> +	unsigned long csi0_ofs;
> +	unsigned long csi1_ofs;
> +	unsigned long smfc_ofs;
> +	unsigned long ic_ofs;
>  	unsigned long disp0_ofs;
>  	unsigned long disp1_ofs;
>  	unsigned long dc_tmpl_ofs;
> @@ -873,8 +877,12 @@ static struct ipu_devtype ipu_type_imx6q = {
>  	.cpmem_ofs = 0x00300000,
>  	.srm_ofs = 0x00340000,
>  	.tpm_ofs = 0x00360000,
> +	.csi0_ofs = 0x00230000,
> +	.csi1_ofs = 0x00238000,
>  	.disp0_ofs = 0x00240000,
>  	.disp1_ofs = 0x00248000,
> +	.smfc_ofs =  0x00250000,
> +	.ic_ofs = 0x00220000,
>  	.dc_tmpl_ofs = 0x00380000,
>  	.vdi_ofs = 0x00268000,
>  	.type = IPUV3H,
> @@ -915,8 +923,44 @@ static int ipu_submodules_init(struct ipu_soc *ipu,
>  	struct device *dev = &pdev->dev;
>  	const struct ipu_devtype *devtype = ipu->devtype;
>  
> +	ret = ipu_csi_init(ipu, dev, 0, ipu_base + devtype->csi0_ofs,
> +			   IPU_CONF_CSI0_EN, ipu_clk);
> +	if (ret) {
> +		unit = "csi0";
> +		goto err_csi_0;
> +	}

Please be nice to other SoCs. You set csi0_ofs for i.MX6, but not for
i.MX5. For i.MX5 you silently initialize the CSI with bogus register
offsets. Either test for _ofs == 0 before initializing it or add the
correct values for i.MX51/53 (preferred).

> +int ipu_csi_set_src(struct ipu_csi *csi, u32 vc, bool select_mipi_csi2)
> +{
> +	struct ipu_soc *ipu = csi->ipu;
> +	int ipu_id = ipu_get_num(ipu);
> +	u32 val, bit;
> +
> +	/*
> +	 * Set the muxes that choose between mipi-csi2 and parallel inputs
> +	 * to the CSI's.
> +	 */
> +	if (csi->ipu->ipu_type == IPUV3HDL) {
> +		/*
> +		 * On D/L, the mux is in GPR13. The TRM is unclear,
> +		 * but it appears the mux allows selecting the MIPI
> +		 * CSI-2 virtual channel number to route to the CSIs.
> +		 */
> +		bit = GPR13_IPUV3HDL_CSI_MUX_CTL_SHIFT + csi->id * 3;
> +		val = select_mipi_csi2 ? vc << bit : 4 << bit;
> +		regmap_update_bits(ipu->gp_reg, IOMUXC_GPR13,
> +				   0x7 << bit, val);
> +	} else if (csi->ipu->ipu_type == IPUV3H) {
> +		/*
> +		 * For Q/D, the mux only exists on IPU0-CSI0 and IPU1-CSI1,
> +		 * and the routed virtual channel numbers are fixed at 0 and
> +		 * 3 respectively.
> +		 */
> +		if ((ipu_id == 0 && csi->id == 0) ||
> +		    (ipu_id == 1 && csi->id == 1)) {
> +			bit = GPR1_IPU_CSI_MUX_CTL_SHIFT + csi->ipu->id;
> +			val = select_mipi_csi2 ? 0 : 1 << bit;
> +			regmap_update_bits(ipu->gp_reg, IOMUXC_GPR1,
> +					   0x1 << bit, val);
> +		}
> +	} else {
> +		dev_err(csi->ipu->dev,
> +			"ERROR: %s: unsupported CPU version!\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * there is another mux in the IPU config register that
> +	 * must be set as well
> +	 */
> +	ipu_set_csi_src_mux(ipu, csi->id, select_mipi_csi2);

This is not a property of the IPU but how the IPU is integrated into the
SoC. I'm unsure this should be integrated like this, it sounds more like
a job for mediactrl.

> +#include <linux/types.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/spinlock.h>
> +#include <linux/videodev2.h>
> +#include <linux/bitrev.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/clkdev.h>

Please clean up the include list. There is nothing used from
clk-provider.h for example.

> +static int init_csc(struct ipu_ic *ic,
> +		    enum ipu_color_space in_format,
> +		    enum ipu_color_space out_format,
> +		    int csc_index);
> +static int calc_resize_coeffs(struct ipu_ic *ic,
> +			      u32 in_size, u32 out_size,
> +			      u32 *resize_coeff,
> +			      u32 *downsize_coeff);

Please reorder functions to get rid of static function declarations.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |