lkml.org 
[lkml]   [2020]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@cerno.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@bootlin.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

    \
     
     \ /
      Last update: 2020-08-26 11:18    [W:7.050 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site