Message-ID: <20200826111728.21d52c34@lhopital-XPS-13-9360>
Date:   Wed, 26 Aug 2020 11:17:28 +0200
From:   Kévin L'hôpital <kevin.lhopital@...tlin.com>
To:     Maxime Ripard <maxime@...no.tech>
Cc:     linux-media@...r.kernel.org, mchehab@...nel.org,
        robh+dt@...nel.org, mark.rutland@....com, wens@...e.org,
        yong.deng@...ewell.com, p.zabel@...gutronix.de,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        paul.kocialkowski@...tlin.com, thomas.petazzoni@...tlin.com
Subject: Re: [PATCH 5/7] media: sunxi: sun6i-csi: Add support of MIPI CSI-2
 for A83T

Hello,

Le Tue, 25 Aug 2020 16:37:04 +0200,
Maxime Ripard <maxime@...no.tech> a écrit :

> Hi,
> 
> On Fri, Aug 21, 2020 at 04:59:33PM +0200, Kévin L'hôpital wrote:
> > This patch add the support only for the Allwinner A83T MIPI CSI2
> > 
> > Currently, the driver is not supported the other Allwinner V3's
> > MIPI CSI2
> > 
> > It has been tested with the ov8865 image sensor.
> > 
> > Signed-off-by: Kévin L'hôpital <kevin.lhopital@...tlin.com>  
> 
> Explaining how it's different from the v3s would help
> 
> > ---
> >  .../media/platform/sunxi/sun6i-csi/Makefile   |   2 +-
> >  .../platform/sunxi/sun6i-csi/sun6i_csi.c      |  82 ++++--
> >  .../sunxi/sun6i-csi/sun8i_a83t_dphy.c         |  20 ++
> >  .../sunxi/sun6i-csi/sun8i_a83t_dphy.h         |  16 ++
> >  .../sunxi/sun6i-csi/sun8i_a83t_dphy_reg.h     |  15 ++
> >  .../sunxi/sun6i-csi/sun8i_a83t_mipi_csi2.c    | 249
> > ++++++++++++++++++ .../sunxi/sun6i-csi/sun8i_a83t_mipi_csi2.h    |
> > 16 ++ .../sun6i-csi/sun8i_a83t_mipi_csi2_reg.h      |  42 +++
> >  8 files changed, 425 insertions(+), 17 deletions(-)
> >  create mode 100644
> > drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy.c create
> > mode 100644
> > drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy.h create
> > mode 100644
> > drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy_reg.h create
> > mode 100644
> > drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2.c
> > create mode 100644
> > drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2.h
> > create mode 100644
> > drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2_reg.h
> > 
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/Makefile
> > b/drivers/media/platform/sunxi/sun6i-csi/Makefile index
> > e7e315347804..0f3849790463 100644 ---
> > a/drivers/media/platform/sunxi/sun6i-csi/Makefile +++
> > b/drivers/media/platform/sunxi/sun6i-csi/Makefile @@ -1,4 +1,4 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> > -sun6i-csi-y += sun6i_video.o sun6i_csi.o
> > +sun6i-csi-y += sun6i_video.o sun6i_csi.o sun8i_a83t_mipi_csi2.o
> > sun8i_a83t_dphy.o 
> >  obj-$(CONFIG_VIDEO_SUN6I_CSI) += sun6i-csi.o
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c index
> > 680fa31f380a..37aec0b57a46 100644 ---
> > a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c +++
> > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c @@ -26,6 +26,7
> > @@ 
> >  #include "sun6i_csi.h"
> >  #include "sun6i_csi_reg.h"
> > +#include "sun8i_a83t_mipi_csi2.h"
> >  
> >  #define MODULE_NAME	"sun6i-csi"
> >  
> > @@ -40,6 +41,18 @@ bool sun6i_csi_is_format_supported(struct
> > sun6i_csi *csi, {
> >  	struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> >  
> > +	if (csi->v4l2_ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > +		if (!sdev->clk_mipi) {
> > +			dev_err(sdev->dev, "Use MIPI-CSI2 device
> > with no MIPI clock\n");
> > +			return false;
> > +		}
> > +		if (!sdev->clk_misc) {
> > +			dev_err(sdev->dev, "Use MIPI-CSI2 device
> > with no misc clock\n");
> > +			return false;
> > +		}
> > +		return true;
> > +	}
> > +  
> 
> I'm not sure we need to check for that everywhere, just doing so in
> the fwnode parsing function sohuld be enough.
> 

All right, I will do it in the fwnode function.

> >  	/*
> >  	 * Some video receivers have the ability to be compatible
> > with
> >  	 * 8bit and 16bit bus width.
> > @@ -160,10 +173,14 @@ int sun6i_csi_set_power(struct sun6i_csi
> > *csi, bool enable) regmap_update_bits(regmap, CSI_EN_REG,
> > CSI_EN_CSI_EN, 0); 
> >  		clk_disable_unprepare(sdev->clk_ram);
> > +
> >  		if (of_device_is_compatible(dev->of_node,
> >  					    "allwinner,sun50i-a64-csi"))
> >  			clk_rate_exclusive_put(sdev->clk_mod);
> >  		clk_disable_unprepare(sdev->clk_mod);
> > +		if (csi->v4l2_ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> > +			sun6i_mipi_csi_clk_disable(csi);
> > +
> >  		reset_control_assert(sdev->rstc_bus);
> >  		return 0;
> >  	}
> > @@ -189,10 +206,18 @@ int sun6i_csi_set_power(struct sun6i_csi
> > *csi, bool enable) goto clk_ram_disable;
> >  	}
> >  
> > +	if (csi->v4l2_ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > +		ret = sun6i_mipi_csi_clk_enable(csi);
> > +		if (ret)
> > +			goto reset_control_assert;
> > +	}
> > +
> >  	regmap_update_bits(regmap, CSI_EN_REG, CSI_EN_CSI_EN,
> > CSI_EN_CSI_EN); 
> >  	return 0;
> >  
> > +reset_control_assert:
> > +	reset_control_assert(sdev->rstc_bus);
> >  clk_ram_disable:
> >  	clk_disable_unprepare(sdev->clk_ram);
> >  clk_mod_disable:
> > @@ -421,27 +446,33 @@ static void sun6i_csi_setup_bus(struct
> > sun6i_csi_dev *sdev) if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
> >  			cfg |= CSI_IF_CFG_CLK_POL_FALLING_EDGE;
> >  		break;
> > +	case V4L2_MBUS_CSI2_DPHY:
> > +		cfg |= CSI_IF_CFG_MIPI_IF_MIPI;
> > +		sun6i_mipi_csi_setup_bus(csi);
> > +		break;
> >  	default:
> >  		dev_warn(sdev->dev, "Unsupported bus type: %d\n",
> >  			 endpoint->bus_type);
> >  		break;
> >  	}
> >  
> > -	switch (bus_width) {
> > -	case 8:
> > -		cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT;
> > -		break;
> > -	case 10:
> > -		cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT;
> > -		break;
> > -	case 12:
> > -		cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT;
> > -		break;
> > -	case 16: /* No need to configure DATA_WIDTH for 16bit */
> > -		break;
> > -	default:
> > -		dev_warn(sdev->dev, "Unsupported bus width: %u\n",
> > bus_width);
> > -		break;
> > +	if (endpoint->bus_type != V4L2_MBUS_CSI2_DPHY) {
> > +		switch (bus_width) {
> > +		case 8:
> > +			cfg |= CSI_IF_CFG_IF_DATA_WIDTH_8BIT;
> > +			break;
> > +		case 10:
> > +			cfg |= CSI_IF_CFG_IF_DATA_WIDTH_10BIT;
> > +			break;
> > +		case 12:
> > +			cfg |= CSI_IF_CFG_IF_DATA_WIDTH_12BIT;
> > +			break;
> > +		case 16: /* No need to configure DATA_WIDTH for
> > 16bit */
> > +			break;
> > +		default:
> > +			dev_warn(sdev->dev, "Unsupported bus
> > width: %u\n", bus_width);
> > +			break;
> > +		}
> >  	}  
> 
> I'm not sure why we need to do some setup in both cases, and some only
> in the MIPI-CSI case here, can you clarify that a bit?

Yes I will add a comment to explain it.
> 
> >  
> >  	regmap_write(sdev->regmap, CSI_IF_CFG_REG, cfg);
> > @@ -593,6 +624,9 @@ void sun6i_csi_set_stream(struct sun6i_csi
> > *csi, bool enable) struct regmap *regmap = sdev->regmap;
> >  
> >  	if (!enable) {
> > +		if (csi->v4l2_ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> > +			sun6i_mipi_csi_set_stream(csi, 0);
> > +
> >  		regmap_update_bits(regmap, CSI_CAP_REG,
> > CSI_CAP_CH0_VCAP_ON, 0); regmap_write(regmap, CSI_CH_INT_EN_REG, 0);
> >  		return;
> > @@ -609,6 +643,9 @@ void sun6i_csi_set_stream(struct sun6i_csi
> > *csi, bool enable) 
> >  	regmap_update_bits(regmap, CSI_CAP_REG,
> > CSI_CAP_CH0_VCAP_ON, CSI_CAP_CH0_VCAP_ON);
> > +
> > +	if (csi->v4l2_ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> > +		sun6i_mipi_csi_set_stream(csi, 1);
> >  }
> >  
> >  /*
> > -----------------------------------------------------------------------------
> > @@ -692,6 +729,7 @@ static int sun6i_csi_fwnode_parse(struct device
> > *dev, } 
> >  	switch (vep->bus_type) {
> > +	case V4L2_MBUS_CSI2_DPHY:
> >  	case V4L2_MBUS_PARALLEL:
> >  	case V4L2_MBUS_BT656:
> >  		csi->v4l2_ep = *vep;
> > @@ -812,7 +850,7 @@ static const struct regmap_config
> > sun6i_csi_regmap_config = { .reg_bits       = 32,
> >  	.reg_stride     = 4,
> >  	.val_bits       = 32,
> > -	.max_register	= 0x9c,
> > +	.max_register	= 0x2000,
> >  };
> >  
> >  static int sun6i_csi_resource_request(struct sun6i_csi_dev *sdev,
> > @@ -847,6 +885,18 @@ static int sun6i_csi_resource_request(struct
> > sun6i_csi_dev *sdev, return PTR_ERR(sdev->clk_ram);
> >  	}
> >  
> > +	sdev->clk_mipi = devm_clk_get(&pdev->dev, "mipi");
> > +	if (IS_ERR(sdev->clk_mipi)) {
> > +		sdev->clk_mipi = NULL;
> > +		dev_warn(&pdev->dev, "Unable to acquire mipi
> > clock. No mipi support\n");
> > +	}
> > +
> > +	sdev->clk_misc = devm_clk_get(&pdev->dev, "misc");
> > +	if (IS_ERR(sdev->clk_misc)) {
> > +		sdev->clk_misc = NULL;
> > +		dev_warn(&pdev->dev, "Unable to acquire misc
> > clock. No mipi support\n");
> > +	}
> > +  
> 
> This will raise an error on platforms that use that driver for CSI but
> don't have MIPI-CSI (like the H3 IIRC?), this isn't really ok.
> 
> I guess we could have a per-soc flag where you'd say if MIPI-CSI is
> supported and only try to get the clock on the relevant SoCs.
> 
> Also, you're changing the DT binding, the documentation should be
> updated.
> 

Yes you are right, I will add this.

> >  	sdev->rstc_bus = devm_reset_control_get_shared(&pdev->dev,
> > NULL); if (IS_ERR(sdev->rstc_bus)) {
> >  		dev_err(&pdev->dev, "Cannot get reset
> > controller\n"); diff --git
> > a/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy.c
> > b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy.c new file
> > mode 100644 index 000000000000..3f5e4395aaa5 --- /dev/null
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy.c
> > @@ -0,0 +1,20 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * sun6i_dphy.c
> > + * Copyright Kévin L'hôpital (C) 2020
> > + */
> > +
> > +#include "sun8i_a83t_dphy.h"
> > +#include "sun8i_a83t_dphy_reg.h"
> > +
> > +void sun6i_dphy_first_init(struct sun6i_csi_dev *sdev)
> > +{
> > +	regmap_write(sdev->regmap, DPHY_CTRL_REG, 0xb8df698e);
> > +}
> > +
> > +void sun6i_dphy_second_init(struct sun6i_csi_dev *sdev)
> > +{
> > +	regmap_write(sdev->regmap, DPHY_CTRL_REG, 0x80008000);
> > +	regmap_write(sdev->regmap, DPHY_ANA0_REG, 0xa0200000);
> > +}  
> 
> Some documentation/comment on what that first init and second init is,
> and what those magic values are doing would be great here.
> 

Yes I will add some comments.

> > diff --git
> > a/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy.h
> > b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy.h new file
> > mode 100644 index 000000000000..f776ed098cb3 --- /dev/null
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * sun6i_dphy.h
> > + * Copyright Kévin L'hôpital (C) 2020
> > + */
> > +
> > +#ifndef __SUN8I_A83T_DPHY_H__
> > +#define __SUN8I_A83T_DPHY_H__
> > +
> > +#include <linux/regmap.h>
> > +#include "sun6i_csi.h"
> > +
> > +void sun6i_dphy_first_init(struct sun6i_csi_dev *sdev);
> > +void sun6i_dphy_second_init(struct sun6i_csi_dev *sdev);
> > +
> > +#endif /* __SUN8I_A83T_DPHY_H__ */
> > diff --git
> > a/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy_reg.h
> > b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy_reg.h new
> > file mode 100644 index 000000000000..c88824e4ec2e --- /dev/null
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_dphy_reg.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Allwinner A83t DPHY register description
> > + * Copyright Kévin L'hôpital (C) 2020
> > + */
> > +
> > +#ifndef __SUN8I_A83T_DPHY_REG_H__
> > +#define __SUN8I_A83T_DPHY_REG_H__
> > +
> > +
> > +#define DPHY_OFFSET			0x1000
> > +#define DPHY_CTRL_REG			(DPHY_OFFSET + 0x010)
> > +#define DPHY_ANA0_REG			(DPHY_OFFSET + 0x030)
> > +
> > +#endif /* __SUN8I_A83T_DPHY_REG_H__ */  
> 
> Ideally this should be a D-PHY driver under drivers/phy
> 
> > diff --git
> > a/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2.c
> > b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2.c new
> > file mode 100644 index 000000000000..3f117e8d447f --- /dev/null
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2.c
> > @@ -0,0 +1,249 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Allwinner A83t MIPI Camera Sensor Interface driver
> > + * Copyright Kévin L'hôpital (C) 2020
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include "sun8i_a83t_mipi_csi2.h"
> > +#include "sun8i_a83t_mipi_csi2_reg.h"
> > +#include "sun8i_a83t_dphy.h"
> > +#include <linux/delay.h>
> > +
> > +#define IS_FLAG(x, y) (((x) & (y)) == y)
> > +
> > +enum mipi_csi2_pkt_fmt {
> > +	MIPI_FS           = 0X00,
> > +	MIPI_FE           = 0X01,
> > +	MIPI_LS           = 0X02,
> > +	MIPI_LE           = 0X03,
> > +	MIPI_SDAT0          = 0X08,
> > +	MIPI_SDAT1          = 0X09,
> > +	MIPI_SDAT2          = 0X0A,
> > +	MIPI_SDAT3          = 0X0B,
> > +	MIPI_SDAT4          = 0X0C,
> > +	MIPI_SDAT5          = 0X0D,
> > +	MIPI_SDAT6          = 0X0E,
> > +	MIPI_SDAT7          = 0X0F,
> > +	MIPI_BLK            = 0X11,
> > +	MIPI_EMBD         = 0X12,
> > +	MIPI_YUV420       = 0X18,
> > +	MIPI_YUV420_10    = 0X19,
> > +	MIPI_YUV420_CSP   = 0X1C,
> > +	MIPI_YUV420_CSP_10 =  0X1D,
> > +	MIPI_YUV422       = 0X1E,
> > +	MIPI_YUV422_10    = 0X1F,
> > +	MIPI_RGB565       = 0X22,
> > +	MIPI_RGB888       = 0X24,
> > +	MIPI_RAW8         = 0X2A,
> > +	MIPI_RAW10          = 0X2B,
> > +	MIPI_RAW12          = 0X2C,
> > +	MIPI_USR_DAT0     = 0X30,
> > +	MIPI_USR_DAT1     = 0X31,
> > +	MIPI_USR_DAT2     = 0X32,
> > +	MIPI_USR_DAT3     = 0X33,
> > +	MIPI_USR_DAT4     = 0X34,
> > +	MIPI_USR_DAT5     = 0X35,
> > +	MIPI_USR_DAT6     = 0X36,
> > +	MIPI_USR_DAT7     = 0X37,
> > +};
> > +
> > +static inline struct sun6i_csi_dev *sun6i_csi_to_dev(struct
> > sun6i_csi *csi) +{
> > +	return container_of(csi, struct sun6i_csi_dev, csi);
> > +}
> > +
> > +static enum mipi_csi2_pkt_fmt get_pkt_fmt(u16 bus_pix_code)
> > +{
> > +	switch (bus_pix_code) {
> > +	case MEDIA_BUS_FMT_RGB565_1X16:
> > +		return MIPI_RGB565;
> > +	case MEDIA_BUS_FMT_UYVY8_2X8:
> > +	case MEDIA_BUS_FMT_UYVY8_1X16:
> > +		return MIPI_YUV422;
> > +	case MEDIA_BUS_FMT_UYVY10_2X10:
> > +		return MIPI_YUV422_10;
> > +	case MEDIA_BUS_FMT_RGB888_1X24:
> > +		return MIPI_RGB888;
> > +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +		return MIPI_RAW8;
> > +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +		return MIPI_RAW10;
> > +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> > +	case MEDIA_BUS_FMT_SGBRG12_1X12:
> > +	case MEDIA_BUS_FMT_SGRBG12_1X12:
> > +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +		return MIPI_RAW12;
> > +	default:
> > +		return MIPI_RAW8;
> > +	}
> > +}
> > +
> > +  
> 
> There's an extra new line here
> 
> > +void sun6i_mipi_csi_set_stream(struct sun6i_csi *csi, bool enable)
> > +{
> > +	struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > +
> > +	if (enable)
> > +		regmap_write_bits(sdev->regmap, MIPI_CSI2_CFG_REG,
> > +				  MIPI_CSI2_CFG_REG_SYNC_EN,
> > +				  MIPI_CSI2_CFG_REG_SYNC_EN);
> > +	else
> > +		regmap_write_bits(sdev->regmap, MIPI_CSI2_CFG_REG,
> > +				  MIPI_CSI2_CFG_REG_SYNC_EN, 0);  
> 
> Do you really need regmap_write_bits here, or is regmap_update_bits
> enough?

Yes I could use regmap_update_bits.
> 
> > +
> > +}
> > +
> > +void sun6i_mipi_csi_init(struct sun6i_csi_dev *sdev)
> > +{
> > +	regmap_write(sdev->regmap, MIPI_CSI2_CTRL_REG, 0xb8c39bec);
> > +	regmap_write(sdev->regmap, MIPI_CSI2_RX_PKT_NUM_REG,
> > 0xb8d257f8);
> > +	sun6i_dphy_first_init(sdev);
> > +	regmap_write(sdev->regmap, MIPI_CSI2_RSVD1_REG,
> > +		     HW_LOCK_REGISTER_VALUE_1);
> > +	regmap_write(sdev->regmap, MIPI_CSI2_RSVD2_REG,
> > +		     HW_LOCK_REGISTER_VALUE_2);
> > +	regmap_write(sdev->regmap, MIPI_CSI2_RX_PKT_NUM_REG, 0);
> > +	regmap_write(sdev->regmap, MIPI_CSI2_VCDT0_REG, 0);
> > +	regmap_write(sdev->regmap, MIPI_CSI2_CFG_REG, 0xb8c64f24);
> > +	sun6i_dphy_second_init(sdev);
> > +	regmap_write(sdev->regmap, MIPI_CSI2_CTRL_REG, 0x80000000);
> > +	regmap_write(sdev->regmap, MIPI_CSI2_CFG_REG,
> > 0x12200000);  
> 
> Again, some defines / comments would be great here.
> 
> > +}
> > +
> > +void sun6i_mipi_csi_setup_bus(struct sun6i_csi *csi)
> > +{
> > +	struct v4l2_fwnode_endpoint *endpoint = &csi->v4l2_ep;
> > +	struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > +	int lane_num = endpoint->bus.mipi_csi2.num_data_lanes;
> > +	int flags = endpoint->bus.mipi_csi2.flags;
> > +	int total_rx_ch = 0;
> > +	int vc[4];
> > +	int ch;
> > +
> > +	sun6i_mipi_csi_init(sdev);
> > +
> > +	if (IS_FLAG(flags, V4L2_MBUS_CSI2_CHANNEL_0)) {
> > +		vc[total_rx_ch] = 0;
> > +		total_rx_ch++;
> > +	}
> > +
> > +	if (IS_FLAG(flags, V4L2_MBUS_CSI2_CHANNEL_1)) {
> > +		vc[total_rx_ch] = 1;
> > +		total_rx_ch++;
> > +	}
> > +
> > +	if (IS_FLAG(flags, V4L2_MBUS_CSI2_CHANNEL_2)) {
> > +		vc[total_rx_ch] = 2;
> > +		total_rx_ch++;
> > +	}
> > +
> > +	if (IS_FLAG(flags, V4L2_MBUS_CSI2_CHANNEL_3)) {
> > +		vc[total_rx_ch] = 3;
> > +		total_rx_ch++;
> > +	}
> > +  
> 
> Is it supposed to handle multiple virtual channels at once? If so, how
> do you get the results of each virtual channel?
> 

Yes you are rigth, implement just one virtual channel makes more sens.

> > +	if (!total_rx_ch) {
> > +		dev_dbg(sdev->dev,
> > +			 "No receive channel assigned, using
> > channel 0.\n");
> > +		vc[total_rx_ch] = 0;
> > +		total_rx_ch++;
> > +	}
> > +	/* Set lane. */
> > +	regmap_write_bits(sdev->regmap, MIPI_CSI2_CFG_REG,
> > +			  MIPI_CSI2_CFG_REG_N_LANE_MASK, (lane_num
> > - 1) <<
> > +			  MIPI_CSI2_CFG_REG_N_LANE_SHIFT);
> > +	/* Set total channels. */
> > +	regmap_write_bits(sdev->regmap, MIPI_CSI2_CFG_REG,
> > +			  MIPI_CSI2_CFG_REG_N_CHANNEL_MASK,
> > (total_rx_ch - 1) <<
> > +			  MIPI_CSI2_CFG_REG_N_CHANNEL_SHIFT);
> > +
> > +	for (ch = 0; ch < total_rx_ch; ch++) {
> > +		switch (ch) {
> > +		case 0:
> > +			regmap_write_bits(sdev->regmap,
> > MIPI_CSI2_VCDT0_REG,
> > +
> > MIPI_CSI2_VCDT0_REG_CH0_DT_MASK,
> > +
> > get_pkt_fmt(csi->config.code));
> > +			regmap_write_bits(sdev->regmap,
> > MIPI_CSI2_VCDT0_REG,
> > +
> > MIPI_CSI2_VCDT0_REG_CH0_VC_MASK,
> > +					  vc[ch] <<
> > MIPI_CSI2_VCDT0_REG_CH0_VC_SHIFT);
> > +			break;
> > +		case 1:
> > +			regmap_write_bits(sdev->regmap,
> > MIPI_CSI2_VCDT0_REG,
> > +
> > MIPI_CSI2_VCDT0_REG_CH1_DT_MASK,
> > +
> > get_pkt_fmt(csi->config.code)
> > +					  <<
> > +
> > MIPI_CSI2_VCDT0_REG_CH1_DT_SHIFT);
> > +			regmap_write_bits(sdev->regmap,
> > MIPI_CSI2_VCDT0_REG,
> > +
> > MIPI_CSI2_VCDT0_REG_CH1_VC_MASK,
> > +					  vc[ch] <<
> > MIPI_CSI2_VCDT0_REG_CH1_VC_SHIFT);
> > +			break;
> > +		case 2:
> > +			regmap_write_bits(sdev->regmap,
> > MIPI_CSI2_VCDT0_REG,
> > +
> > MIPI_CSI2_VCDT0_REG_CH2_DT_MASK,
> > +
> > get_pkt_fmt(csi->config.code)
> > +					  <<
> > +
> > MIPI_CSI2_VCDT0_REG_CH2_DT_SHIFT);
> > +			regmap_write_bits(sdev->regmap,
> > MIPI_CSI2_VCDT0_REG,
> > +
> > MIPI_CSI2_VCDT0_REG_CH2_VC_MASK,
> > +					  vc[ch] <<
> > MIPI_CSI2_VCDT0_REG_CH2_VC_SHIFT);
> > +			break;
> > +		case 3:
> > +			regmap_write_bits(sdev->regmap,
> > MIPI_CSI2_VCDT0_REG,
> > +
> > MIPI_CSI2_VCDT0_REG_CH3_DT_MASK,
> > +
> > get_pkt_fmt(csi->config.code)
> > +					  <<
> > +
> > MIPI_CSI2_VCDT0_REG_CH3_DT_SHIFT);
> > +			regmap_write_bits(sdev->regmap,
> > MIPI_CSI2_VCDT0_REG,
> > +
> > MIPI_CSI2_VCDT0_REG_CH3_VC_MASK,
> > +					  vc[ch] <<
> > MIPI_CSI2_VCDT0_REG_CH3_VC_SHIFT);
> > +			break;
> > +		default:
> > +			regmap_write(sdev->regmap,
> > MIPI_CSI2_VCDT0_REG,
> > +				     MIPI_CSI2_VCDT0_REG_DEFAULT);
> > +			break;
> > +		}
> > +	}
> > +	mdelay(10);  
> 
> Why do you need an mdelay here?

yes a msleep could be more correct here.

> 
> > +
> > +}
> > +
> > +int sun6i_mipi_csi_clk_enable(struct sun6i_csi *csi)
> > +{
> > +	struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(sdev->clk_mipi);
> > +	if (ret) {
> > +		dev_err(sdev->dev, "Enable clk_mipi clk err %d\n",
> > ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(sdev->clk_misc);
> > +	if (ret) {
> > +		dev_err(sdev->dev, "Enable clk_misc clk err %d\n",
> > ret);
> > +		goto clk_mipi_disable;
> > +	}
> > +
> > +	return 0;
> > +
> > +clk_mipi_disable:
> > +	clk_disable_unprepare(sdev->clk_mipi);
> > +	return ret;
> > +}
> > +
> > +void sun6i_mipi_csi_clk_disable(struct sun6i_csi *csi)
> > +{
> > +	struct sun6i_csi_dev *sdev = sun6i_csi_to_dev(csi);
> > +
> > +	clk_disable_unprepare(sdev->clk_misc);
> > +	clk_disable_unprepare(sdev->clk_mipi);
> > +}
> > +
> > +
> > diff --git
> > a/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2.h
> > b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2.h new
> > file mode 100644 index 000000000000..a94c69ccee39 --- /dev/null
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright Kévin L'hôpital (C) 2020
> > + */
> > +
> > +#ifndef __SUN8I_A83T_MIPI_CSI2_H__
> > +#define __SUN8I_A83T_MIPI_CSI2_H__
> > +#include <linux/regmap.h>
> > +#include "sun6i_csi.h"
> > +
> > +void sun6i_mipi_csi_set_stream(struct sun6i_csi *csi, bool enable);
> > +void sun6i_mipi_csi_setup_bus(struct sun6i_csi *csi);
> > +int sun6i_mipi_csi_clk_enable(struct sun6i_csi *csi);
> > +void sun6i_mipi_csi_clk_disable(struct sun6i_csi *csi);
> > +
> > +#endif /* __SUN8I_A83T_MIPI_CSI2_H__ */
> > diff --git
> > a/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2_reg.h
> > b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2_reg.h
> > new file mode 100644 index 000000000000..4d6fde3e50ef --- /dev/null
> > +++
> > b/drivers/media/platform/sunxi/sun6i-csi/sun8i_a83t_mipi_csi2_reg.h
> > @@ -0,0 +1,42 @@ +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Allwinner A83t MIPI CSI register description
> > + * Copyright Kévin L'hôpital (C) 2020
> > + */
> > +
> > +#ifndef __SUN8I_A83T_MIPI_CSI2_REG_H__
> > +#define __SUN8I_A83T_MIPI_CSI2_REG_H__
> > +
> > +
> > +#define MIPI_CSI2_OFFSET			0x1000
> > +#define MIPI_CSI2_CTRL_REG
> > (MIPI_CSI2_OFFSET + 0x004) +#define
> > MIPI_CSI2_RX_PKT_NUM_REG		(MIPI_CSI2_OFFSET + 0x008)
> > +#define MIPI_CSI2_RSVD1_REG
> > (MIPI_CSI2_OFFSET + 0x018) +#define
> > HW_LOCK_REGISTER_VALUE_1		0xb8c8a30c +#define
> > MIPI_CSI2_RSVD2_REG			(MIPI_CSI2_OFFSET +
> > 0x01c) +#define HW_LOCK_REGISTER_VALUE_2		0xb8df8ad7  
> 
> We should have defines for those, or at least where they are coming
> from
> 
> > +#define MIPI_CSI2_CFG_REG			(MIPI_CSI2_OFFSET
> > + 0x100) +#define MIPI_CSI2_CFG_REG_SYNC_EN		BIT(31)
> > +#define MIPI_CSI2_CFG_REG_N_LANE_SHIFT		4
> > +#define MIPI_CSI2_CFG_REG_N_LANE_MASK		0x30  
> 
> GENMASK would be great here (and everywhere below too)
> 
> Maxime

Thank you very much for the review.
Kévin

-- 
Kevin L'Hopital, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Powered by blists - more mailing lists