| Message ID | 20200821145935.20346-1-kevin.lhopital@bootlin.com |
|---|---|
| Headers | show |
| Series | Support of MIPI CSI-2 for A83T and OV8865 camera | expand |
Hi, On Fri, Aug 21, 2020 at 04:59:28PM +0200, Kévin L'hôpital wrote: > > Kévin L'hôpital (7): > media: sun6i-csi: Fix the bpp for 10-bit bayer formats > dt-bindings: media: i2c: Add documentation for ov8865 > media: i2c: Add support for the OV8865 image sensor > media: sunxi: sun6i-csi: Move the sun6i_csi_dev structure to the > common header > media: sunxi: sun6i-csi: Add support of MIPI CSI-2 for A83T > ARM: dts: sun8i: a83t: Add support for the MIPI CSI-2 in CSI node > [NOT FOR MERGE] ARM: dts: sun8i: a83t: bananapi-m3: Enable OV8865 > camera You should have a cover letter here to provide some context. There's a bunch of things that would need to be explained and / or argued for here, in particular: - Why did you need to plumb it into sun6i-csi? - You're naming the CSI part as the A83t CSI, while MIPI-CSI has been supported since the A31(?), is there a reason for that? - This is not documented anywhere, what did you base this work on? Also, I think that documenting the general challenges you faced (which were likely because of the first bullet point above) and how you solved them here would be great to start a discussion if needed. Finally, iirc, Hans requires a v4l2-compliance run for any new driver, which isn't strictly the case for this driver, but isn't really *not* the case either. Maxime
On Fri, Aug 21, 2020 at 04:59:29PM +0200, Kévin L'hôpital wrote: > 10-bit bayer formats are aligned to 16 bits in memory, so this is what > needs to be used as bpp for calculating the size of the buffers to > allocate. > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> Generally speaking, you should also explain why it's not an issue for the callers. Depending on what that function is supposed to be doing (returning the padded bits or the padded bits per pixel), your patch could be either right or wrong. Since all the callers are using it to generate the number of bytes per line, your patch is indeed correct. But it should be mentionned in the commit log. Maxime
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. > /* > * 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? > > 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. > 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. > 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? > + > +} > + > +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? > + 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? > + > +} > + > +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
Hi, On Fri, Aug 21, 2020 at 04:59:35PM +0200, Kévin L'hôpital wrote: > The Bananapi M3 supports a camera module which includes an > OV8865 sensor connected via the parallel CSI interface and > an OV8865 sensor connected via MIPI CSI-2. > > The I2C2 bus is shared by the two sensors as well as active-low > reset signal but each sensor has it own shutdown line. > > The I2c address for the OV8865 is 0x36. > > The bus type is hardcoded to 4 due to the lack of available > define usable in the device-tree. > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > > --- > arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 99 ++++++++++++++++++++ > 1 file changed, 99 insertions(+) > > diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts > index 9d34eabba121..f7839094695e 100644 > --- a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts > +++ b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts > @@ -85,6 +85,38 @@ > }; > }; > > + reg_ov8865_avdd: ov8865-avdd { > + compatible = "regulator-fixed"; > + regulator-name = "ov8865-avdd"; > + regulator-min-microvolt = <2800000>; > + regulator-max-microvolt = <2800000>; > + vin-supply = <®_dldo4>; > + }; > + > + reg_ov8865_dovdd: ov8865-dovdd { > + compatible = "regulator-fixed"; > + regulator-name = "ov8865-dovdd"; > + regulator-min-microvolt = <2800000>; > + regulator-max-microvolt = <2800000>; > + vin-supply = <®_dldo4>; > + }; > + > + reg_ov8865_afvdd: ov8865-afvdd { > + compatible = "regulator-fixed"; > + regulator-name = "ov8865-afvdd"; > + regulator-min-microvolt = <2800000>; > + regulator-max-microvolt = <2800000>; > + vin-supply = <®_dldo4>; > + }; > + > + reg_ov8865_vdd2: ov8865-vdd2 { > + compatible = "regulator-fixed"; > + regulator-name = "ov8865-vdd2"; > + regulator-min-microvolt = <1200000>; > + regulator-max-microvolt = <1200000>; > + vin-supply = <®_eldo1>; > + }; > + > reg_usb1_vbus: reg-usb1-vbus { > compatible = "regulator-fixed"; > regulator-name = "usb1-vbus"; > @@ -115,10 +147,59 @@ > cpu-supply = <®_dcdc3>; > }; > > +&ccu { > + assigned-clocks = <&ccu CLK_CSI_MCLK>; > + assigned-clock-parents = <&osc24M>; > + assigned-clock-rates = <24000000>; > +}; Why do you need to use assigned-clocks here? > +&csi { > + pinctrl-names = "default"; > + status = "okay"; > +}; pinctrl-names alone is useless > + > +&csi_in { > + mipi_csi2_from_ov8865: endpoint { > + remote-endpoint = <&ov8865_to_mipi_csi2>; > + clock-lanes = <0>; > + data-lanes = <1 2 3 4>; > + bus-type = <4>; > + }; > +}; > + > &de { > status = "okay"; > }; > > +&i2c2 { > + pinctrl-names = "default"; > + pinctrl-0 = <&i2c2_pe_pins>; > + status = "okay"; > + > + ov8865: camera@36 { > + compatible = "ovti,ov8865"; > + reg = <0x36>; > + clocks = <&ccu CLK_CSI_MCLK>; > + clock-names ="xclk"; > + AVDD-supply = <®_ov8865_avdd>; > + DOVDD-supply = <®_ov8865_dovdd>; > + VDD2-supply = <®_ov8865_vdd2>; > + AFVDD-supply = <®_ov8865_afvdd>; > + powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* PE17 */ > + reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 */ > + rotation = <180>; > + > + port { > + ov8865_to_mipi_csi2: endpoint { > + remote-endpoint = <&mipi_csi2_from_ov8865>; > + data-lanes = <1 2 3 4>; > + clock-lanes = <0>; > + bus-type = <4>; /* V4L2_FWNODE_BUS_TYPE_CSI2_DPHY */ > + }; > + }; > + }; > +}; > + > &ehci0 { > /* Terminus Tech FE 1.1s 4-port USB 2.0 hub here */ > status = "okay"; > @@ -191,6 +272,11 @@ > status = "okay"; > }; > > +&pio { > + pinctrl-names = "default"; > + pinctrl-0 = <&csi_mclk_pin>; > +}; I'm not sure why you'd need to use the MCLK pin as a hog, assigning it to the camera device should be enough? Maxime
Le Mon, 24 Aug 2020 18:52:44 +0200, Maxime Ripard <maxime@cerno.tech> a écrit : > Hi, > > On Fri, Aug 21, 2020 at 04:59:28PM +0200, Kévin L'hôpital wrote: > > > > Kévin L'hôpital (7): > > media: sun6i-csi: Fix the bpp for 10-bit bayer formats > > dt-bindings: media: i2c: Add documentation for ov8865 > > media: i2c: Add support for the OV8865 image sensor > > media: sunxi: sun6i-csi: Move the sun6i_csi_dev structure to the > > common header > > media: sunxi: sun6i-csi: Add support of MIPI CSI-2 for A83T > > ARM: dts: sun8i: a83t: Add support for the MIPI CSI-2 in CSI node > > [NOT FOR MERGE] ARM: dts: sun8i: a83t: bananapi-m3: Enable OV8865 > > camera > > You should have a cover letter here to provide some context. > > There's a bunch of things that would need to be explained and / or > argued for here, in particular: > - Why did you need to plumb it into sun6i-csi? > - You're naming the CSI part as the A83t CSI, while MIPI-CSI has > been supported since the A31(?), is there a reason for that? > - This is not documented anywhere, what did you base this work on? > > Also, I think that documenting the general challenges you faced (which > were likely because of the first bullet point above) and how you > solved them here would be great to start a discussion if needed. > > Finally, iirc, Hans requires a v4l2-compliance run for any new driver, > which isn't strictly the case for this driver, but isn't really *not* > the case either. > > Maxime Thank you very much for the review, I will add all this context. Kévin
Hello, Le Mon, 24 Aug 2020 18:55:36 +0200, Maxime Ripard <maxime@cerno.tech> a écrit : > On Fri, Aug 21, 2020 at 04:59:29PM +0200, Kévin L'hôpital wrote: > > 10-bit bayer formats are aligned to 16 bits in memory, so this is > > what needs to be used as bpp for calculating the size of the > > buffers to allocate. > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > > Generally speaking, you should also explain why it's not an issue for > the callers. Depending on what that function is supposed to be doing > (returning the padded bits or the padded bits per pixel), your patch > could be either right or wrong. > > Since all the callers are using it to generate the number of bytes per > line, your patch is indeed correct. But it should be mentionned in the > commit log. > > Maxime All right, I will add this explanation. Thank you very much for the review Kévin
On Fri, Aug 21, 2020 at 11:00 PM Kévin L'hôpital <kevin.lhopital@bootlin.com> wrote: > > 10-bit bayer formats are aligned to 16 bits in memory, so this is what > needs to be used as bpp for calculating the size of the buffers to > allocate. > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> Please add: Fixes: 5cc7522d8965 ("media: sun6i: Add support for Allwinner CSI V3s") > --- > drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > index c626821aaedb..8b83d15de0d0 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > @@ -100,7 +100,7 @@ static inline int sun6i_csi_get_bpp(unsigned int pixformat) > case V4L2_PIX_FMT_SGBRG10: > case V4L2_PIX_FMT_SGRBG10: > case V4L2_PIX_FMT_SRGGB10: > - return 10; > + return 16; > case V4L2_PIX_FMT_SBGGR12: > case V4L2_PIX_FMT_SGBRG12: > case V4L2_PIX_FMT_SGRBG12: > -- > 2.17.1 >
Hello, Le Tue, 25 Aug 2020 16:40:22 +0200, Maxime Ripard <maxime@cerno.tech> a écrit : > Hi, > > On Fri, Aug 21, 2020 at 04:59:35PM +0200, Kévin L'hôpital wrote: > > The Bananapi M3 supports a camera module which includes an > > OV8865 sensor connected via the parallel CSI interface and > > an OV8865 sensor connected via MIPI CSI-2. > > > > The I2C2 bus is shared by the two sensors as well as active-low > > reset signal but each sensor has it own shutdown line. > > > > The I2c address for the OV8865 is 0x36. > > > > The bus type is hardcoded to 4 due to the lack of available > > define usable in the device-tree. > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > > > > --- > > arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 99 > > ++++++++++++++++++++ 1 file changed, 99 insertions(+) > > > > diff --git a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts > > b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts index > > 9d34eabba121..f7839094695e 100644 --- > > a/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts +++ > > b/arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts @@ -85,6 +85,38 @@ > > }; > > }; > > > > + reg_ov8865_avdd: ov8865-avdd { > > + compatible = "regulator-fixed"; > > + regulator-name = "ov8865-avdd"; > > + regulator-min-microvolt = <2800000>; > > + regulator-max-microvolt = <2800000>; > > + vin-supply = <®_dldo4>; > > + }; > > + > > + reg_ov8865_dovdd: ov8865-dovdd { > > + compatible = "regulator-fixed"; > > + regulator-name = "ov8865-dovdd"; > > + regulator-min-microvolt = <2800000>; > > + regulator-max-microvolt = <2800000>; > > + vin-supply = <®_dldo4>; > > + }; > > + > > + reg_ov8865_afvdd: ov8865-afvdd { > > + compatible = "regulator-fixed"; > > + regulator-name = "ov8865-afvdd"; > > + regulator-min-microvolt = <2800000>; > > + regulator-max-microvolt = <2800000>; > > + vin-supply = <®_dldo4>; > > + }; > > + > > + reg_ov8865_vdd2: ov8865-vdd2 { > > + compatible = "regulator-fixed"; > > + regulator-name = "ov8865-vdd2"; > > + regulator-min-microvolt = <1200000>; > > + regulator-max-microvolt = <1200000>; > > + vin-supply = <®_eldo1>; > > + }; > > + > > reg_usb1_vbus: reg-usb1-vbus { > > compatible = "regulator-fixed"; > > regulator-name = "usb1-vbus"; > > @@ -115,10 +147,59 @@ > > cpu-supply = <®_dcdc3>; > > }; > > > > +&ccu { > > + assigned-clocks = <&ccu CLK_CSI_MCLK>; > > + assigned-clock-parents = <&osc24M>; > > + assigned-clock-rates = <24000000>; > > +}; > > Why do you need to use assigned-clocks here? I could do it in the ov8865 node, does it sound good to you ? > > > +&csi { > > + pinctrl-names = "default"; > > + status = "okay"; > > +}; > > pinctrl-names alone is useless > > > + > > +&csi_in { > > + mipi_csi2_from_ov8865: endpoint { > > + remote-endpoint = <&ov8865_to_mipi_csi2>; > > + clock-lanes = <0>; > > + data-lanes = <1 2 3 4>; > > + bus-type = <4>; > > + }; > > +}; > > + > > &de { > > status = "okay"; > > }; > > > > +&i2c2 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&i2c2_pe_pins>; > > + status = "okay"; > > + > > + ov8865: camera@36 { > > + compatible = "ovti,ov8865"; > > + reg = <0x36>; > > + clocks = <&ccu CLK_CSI_MCLK>; > > + clock-names ="xclk"; > > + AVDD-supply = <®_ov8865_avdd>; > > + DOVDD-supply = <®_ov8865_dovdd>; > > + VDD2-supply = <®_ov8865_vdd2>; > > + AFVDD-supply = <®_ov8865_afvdd>; > > + powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_LOW>; /* > > PE17 */ > > + reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* PE16 > > */ > > + rotation = <180>; > > + > > + port { > > + ov8865_to_mipi_csi2: endpoint { > > + remote-endpoint = > > <&mipi_csi2_from_ov8865>; > > + data-lanes = <1 2 3 4>; > > + clock-lanes = <0>; > > + bus-type = <4>; /* > > V4L2_FWNODE_BUS_TYPE_CSI2_DPHY */ > > + }; > > + }; > > + }; > > +}; > > + > > &ehci0 { > > /* Terminus Tech FE 1.1s 4-port USB 2.0 hub here */ > > status = "okay"; > > @@ -191,6 +272,11 @@ > > status = "okay"; > > }; > > > > +&pio { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&csi_mclk_pin>; > > +}; > > I'm not sure why you'd need to use the MCLK pin as a hog, assigning it > to the camera device should be enough? Yes you are right, I will put it in the ov8865 node. > > Maxime Thank you very much for the review. Kévin
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
Hello, Le Tue, 25 Aug 2020 23:50:30 +0800, Chen-Yu Tsai <wens@csie.org> a écrit : > On Fri, Aug 21, 2020 at 11:00 PM Kévin L'hôpital > <kevin.lhopital@bootlin.com> wrote: > > > > 10-bit bayer formats are aligned to 16 bits in memory, so this is > > what needs to be used as bpp for calculating the size of the > > buffers to allocate. > > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com> > > Please add: > > Fixes: 5cc7522d8965 ("media: sun6i: Add support for Allwinner CSI > V3s") > > I will add this, thank you very much for the review. Kévin > > > --- > > drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h index > > c626821aaedb..8b83d15de0d0 100644 --- > > a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h +++ > > b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h @@ -100,7 > > +100,7 @@ static inline int sun6i_csi_get_bpp(unsigned int > > pixformat) case V4L2_PIX_FMT_SGBRG10: case V4L2_PIX_FMT_SGRBG10: > > case V4L2_PIX_FMT_SRGGB10: > > - return 10; > > + return 16; > > case V4L2_PIX_FMT_SBGGR12: > > case V4L2_PIX_FMT_SGBRG12: > > case V4L2_PIX_FMT_SGRBG12: > > -- > > 2.17.1 > >
Hi, On Wed, Aug 26, 2020 at 10:58:34AM +0200, Kévin L'hôpital wrote: > > > +&ccu { > > > + assigned-clocks = <&ccu CLK_CSI_MCLK>; > > > + assigned-clock-parents = <&osc24M>; > > > + assigned-clock-rates = <24000000>; > > > +}; > > > > Why do you need to use assigned-clocks here? > > I could do it in the ov8865 node, does it sound good to you ? I mean, it depends on why you want to do it :) If that's because the sensor expects a clock in a particular clock range, then it should be enforced in the sensor driver itself. Maxime
On Wed, Aug 26, 2020 at 11:17:28AM +0200, Kévin L'hôpital wrote: > > > + mdelay(10); > > > > Why do you need an mdelay here? > > yes a msleep could be more correct here. My question was more about whether/why you need one in the first place, not necessarily how you would implement that delay. Maxime
Hello, Le Thu, 27 Aug 2020 17:38:43 +0200, Maxime Ripard <maxime@cerno.tech> a écrit : > Hi, > > On Wed, Aug 26, 2020 at 10:58:34AM +0200, Kévin L'hôpital wrote: > > > > +&ccu { > > > > + assigned-clocks = <&ccu CLK_CSI_MCLK>; > > > > + assigned-clock-parents = <&osc24M>; > > > > + assigned-clock-rates = <24000000>; > > > > +}; > > > > > > Why do you need to use assigned-clocks here? > > > > I could do it in the ov8865 node, does it sound good to you ? > > I mean, it depends on why you want to do it :) > > If that's because the sensor expects a clock in a particular clock > range, then it should be enforced in the sensor driver itself. > > Maxime Yes I will put it in the sensor driver. Thanks for the answer. Kévin
Hello, Le Thu, 27 Aug 2020 17:41:19 +0200, Maxime Ripard <maxime@cerno.tech> a écrit : > On Wed, Aug 26, 2020 at 11:17:28AM +0200, Kévin L'hôpital wrote: > > > > + mdelay(10); > > > > > > Why do you need an mdelay here? > > > > yes a msleep could be more correct here. > > My question was more about whether/why you need one in the first place, > not necessarily how you would implement that delay. > > Maxime It was a mistake, I have removed it. Thanks for the answer. Kévin
Kévin L'hôpital (7): media: sun6i-csi: Fix the bpp for 10-bit bayer formats dt-bindings: media: i2c: Add documentation for ov8865 media: i2c: Add support for the OV8865 image sensor media: sunxi: sun6i-csi: Move the sun6i_csi_dev structure to the common header media: sunxi: sun6i-csi: Add support of MIPI CSI-2 for A83T ARM: dts: sun8i: a83t: Add support for the MIPI CSI-2 in CSI node [NOT FOR MERGE] ARM: dts: sun8i: a83t: bananapi-m3: Enable OV8865 camera .../devicetree/bindings/media/i2c/ov8865.txt | 51 + arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts | 99 + arch/arm/boot/dts/sun8i-a83t.dtsi | 11 +- drivers/media/i2c/Kconfig | 12 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/ov8865.c | 2540 +++++++++++++++++ .../media/platform/sunxi/sun6i-csi/Makefile | 2 +- .../platform/sunxi/sun6i-csi/sun6i_csi.c | 94 +- .../platform/sunxi/sun6i-csi/sun6i_csi.h | 14 +- .../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 + 15 files changed, 3148 insertions(+), 34 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/i2c/ov8865.txt create mode 100644 drivers/media/i2c/ov8865.c 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