Hi Sakari, On Mon, Jun 19, 2023 at 10:50:10AM +0000, Sakari Ailus wrote: > Hi Tommaso, > > On Fri, Jun 09, 2023 at 03:09:41PM +0200, Tommaso Merciai wrote: > > Hi Sakar, > > > > Thanks for your review. > > > > On Fri, Jun 09, 2023 at 09:17:42AM +0000, Sakari Ailus wrote: > > > Hi Tommaso, > > > > > > Thanks for the patch. > > > > > > On Thu, Jun 08, 2023 at 10:31:16AM +0200, Tommaso Merciai wrote: > > > > The Alvium camera is shipped with sensor + isp in the same housing. > > > > The camera can be equipped with one out of various sensor and abstract > > > > the user from this. Camera is connected via MIPI CSI-2. > > > > > > > > Most of the camera module features are supported, with the main exception > > > > being fw update. > > > > > > > > The driver provides all mandatory, optional and recommended V4L2 controls > > > > for maximum compatibility with libcamera > > > > > > > > References: > > > > - https://www.alliedvision.com/en/products/embedded-vision-solutions > > > > > > > > Signed-off-by: Tommaso Merciai <tomm.merciai@xxxxxxxxx> > > > > --- > > > > Changes since v2: > > > > - Removed gpios/clock handling as suggested by LPinchart > > > > - Added vcc-ext-in supply support as suggested by LPinchart > > > > - Fixed alvium_setup_mipi_fmt funct as suggested by CJAILLET > > > > - Removed upside_down/hshake_bit priv data as suggested by CJAILLET > > > > - Fixed commit body as suggested by LPinchart > > > > - Mv alvium_set_streamon_delay to yalvium_set_lp2hs_delay > > > > - Fixed comment on lp2hs prop as suggested by LPinchart > > > > - Added pm resume/suspend functs as suggested by LPinchart > > > > - Dropped alvium_link_setup/alvium_s_power as suggested by LPinchart > > > > - Fixed regs defines as suggested by LPinchart > > > > - Fixed typedef as suggested by LPinchart > > > > - Dropped bcrm_v/fw_v from priv data as suggested by LPinchart > > > > - Now driver use the subdev active state to store the active format and crop > > > > as suggested by LPinchart > > > > - Dropped alvium_is_csi2/i2c_to_alvium as suggested by LPinchart > > > > > > > > Changes since v3: > > > > - Fixed warnings Reported-by: kernel test robot <lkp@xxxxxxxxx> > > > > > > > > Changes since v4: > > > > - Removed print into alvium_get_dt_data for alliedvision,lp2hs-delay-us as > > > > suggested by CDooley > > > > > > > > drivers/media/i2c/Kconfig | 10 + > > > > drivers/media/i2c/Makefile | 1 + > > > > drivers/media/i2c/alvium-csi2.c | 3479 +++++++++++++++++++++++++++++++ > > > > drivers/media/i2c/alvium-csi2.h | 485 +++++ > > > > 4 files changed, 3975 insertions(+) > > > > create mode 100644 drivers/media/i2c/alvium-csi2.c > > > > create mode 100644 drivers/media/i2c/alvium-csi2.h > > > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > > > > index 256d55bb2b1d..a9fdf10ffc15 100644 > > > > --- a/drivers/media/i2c/Kconfig > > > > +++ b/drivers/media/i2c/Kconfig > > > > @@ -34,6 +34,16 @@ config VIDEO_APTINA_PLL > > > > config VIDEO_CCS_PLL > > > > tristate > > > > > > > > +config VIDEO_ALVIUM_CSI2 > > > > + tristate "Allied Vision ALVIUM MIPI CSI-2 camera support" > > > > + depends on I2C > > > > + select MEDIA_CONTROLLER > > > > + select VIDEO_V4L2_SUBDEV_API > > > > + select V4L2_FWNODE > > > > + help > > > > + This is a Video4Linux2 sensor driver for the Allied Vision > > > > + ALVIUM camera connected via MIPI CSI-2 interface. > > > > + > > > > config VIDEO_AR0521 > > > > tristate "ON Semiconductor AR0521 sensor support" > > > > depends on I2C && VIDEO_DEV > > > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > > > > index b44dacf935f4..be4dddd6fd98 100644 > > > > --- a/drivers/media/i2c/Makefile > > > > +++ b/drivers/media/i2c/Makefile > > > > @@ -17,6 +17,7 @@ obj-$(CONFIG_VIDEO_ADV7604) += adv7604.o > > > > obj-$(CONFIG_VIDEO_ADV7842) += adv7842.o > > > > obj-$(CONFIG_VIDEO_AK7375) += ak7375.o > > > > obj-$(CONFIG_VIDEO_AK881X) += ak881x.o > > > > +obj-$(CONFIG_VIDEO_ALVIUM_CSI2) += alvium-csi2.o > > > > obj-$(CONFIG_VIDEO_APTINA_PLL) += aptina-pll.o > > > > obj-$(CONFIG_VIDEO_AR0521) += ar0521.o > > > > obj-$(CONFIG_VIDEO_BT819) += bt819.o > > > > diff --git a/drivers/media/i2c/alvium-csi2.c b/drivers/media/i2c/alvium-csi2.c > > > > new file mode 100644 > > > > index 000000000000..52c9263075cf > > > > --- /dev/null > > > > +++ b/drivers/media/i2c/alvium-csi2.c > > > > @@ -0,0 +1,3479 @@ > > > > +/* SPDX-License-Identifier: GPL-2.0-only */ > > > > +/* > > > > + * Allied Vision Inc. Alvium camera driver > > > > + * > > > > + * Copyright (C) 2023 Tommaso Merciai > > > > + * Copyright (C) 2023 Martin Hecht > > > > + * Copyright (C) 2023 Avnet EMG GmbH > > > > + */ > > > > + > > > > +#include <asm/unaligned.h> > > > > +#include <linux/clk.h> > > > > +#include <linux/clk-provider.h> > > > > +#include <linux/clkdev.h> > > > > +#include <linux/ctype.h> > > > > +#include <linux/delay.h> > > > > +#include <linux/device.h> > > > > +#include <linux/gpio/consumer.h> > > > > +#include <linux/i2c.h> > > > > +#include <linux/init.h> > > > > > > Do you need this? > > > > No, Thanks :) > > > > > > > +#include <linux/module.h> > > > > +#include <linux/of_device.h> > > > > +#include <linux/of_graph.h> > > > > +#include <linux/pm_runtime.h> > > > > +#include <linux/regulator/consumer.h> > > > > +#include <linux/slab.h> > > > > +#include <linux/types.h> > > > > +#include <media/v4l2-async.h> > > > > +#include <media/v4l2-ctrls.h> > > > > +#include <media/mipi-csi2.h> > > > > > > Please order alphabetically. > > > > Oooks, ty > > > > > > > > > +#include <media/v4l2-device.h> > > > > +#include <media/v4l2-event.h> > > > > +#include <media/v4l2-fwnode.h> > > > > +#include <media/v4l2-subdev.h> > > > > + > > > > +#include "alvium-csi2.h" > > > > + > > > > +static const struct v4l2_mbus_framefmt alvium_csi2_default_fmt = { > > > > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > > > > + .width = 640, > > > > + .height = 480, > > > > + .colorspace = V4L2_COLORSPACE_SRGB, > > > > + .ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SRGB), > > > > + .quantization = V4L2_QUANTIZATION_FULL_RANGE, > > > > + .xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SRGB), > > > > + .field = V4L2_FIELD_NONE, > > > > +}; > > > > + > > > > +static const struct alvium_pixfmt alvium_csi2_fmts[] = { > > > > + { > > > > + /* UYVY8_2X8 */ > > > > + .id = ALVIUM_FMT_UYVY8_2X8, > > > > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > > > > + .colorspace = V4L2_COLORSPACE_SRGB, > > > > + .fmt_av_bit = ALVIUM_BIT_YUV422_8, > > > > + .bay_av_bit = ALVIUM_BIT_BAY_NONE, > > > > + .mipi_fmt_regval = 0x1E, > > > > > > Could you use MIPI_CSI2_DT_ definitions in mipi-csi2.h? > > > > ofc ty > > I'm not sure I understand all your comments. Sorry.. :) > > ... > > > > > +{ > > > > + struct i2c_client *client = alvium->i2c_client; > > > > + u16 addr = REG_BCRM_ADDR(reg); > > > > + unsigned int len = REG_BCRM_SIZE(reg); > > > > + struct i2c_msg msgs[2] = {0}; > > > > + u8 addr_buf[2] = {0}; > > > > + u8 data_buf[8] = {0}; > > > > + int ret; > > > > + > > > > + if (WARN_ON(len > 8)) > > > > + return -EINVAL; > > > > + > > > > + if (reg & REG_BCRM_V4L2) > > > > + addr += alvium->bcrm_addr; > > > > + > > > > + put_unaligned_be16(addr, addr_buf); > > > > + > > > > + msgs[0].addr = client->addr; > > > > + msgs[0].flags = 0; > > > > + msgs[0].len = ARRAY_SIZE(addr_buf); > > > > + msgs[0].buf = addr_buf; > > > > + > > > > + msgs[1].addr = client->addr; > > > > + msgs[1].flags = I2C_M_RD; > > > > + msgs[1].len = len; > > > > + msgs[1].buf = &data_buf[8 - len]; > > > > + > > > > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > > > > + if (ret != ARRAY_SIZE(msgs)) > > > > + return -EIO; > > > > + > > > > + *val = get_unaligned_be64(data_buf); > > > > > > Could you use CCI wrapper Hans has been working on? It'll be merged fairly > > > soon I presume: > > > <URL:https://lore.kernel.org/linux-media/20230606165808.70751-1-hdegoede@xxxxxxxxxx/T/#t>. > > > > Thanks for sharing. > > > > Actually I'm working/testing the driver on > > 6.4.0-rc2 I don't see this new functionality, sry. > > They're not there yet. I presume we'll be merging them soonish though. Please Check v6. In this version I'm using new cci API from Hans. > > > > > I can try. > > But this is only for CCI regs read/write right? > > > > Or also for v4l2 regs? > > Let me know, thanks! :) > > > > > > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_write(struct alvium_dev *alvium, > > > > + u32 reg, u64 val) > > > > +{ > > > > + struct i2c_client *client = alvium->i2c_client; > > > > + u16 addr = REG_BCRM_ADDR(reg); > > > > + unsigned int len = REG_BCRM_SIZE(reg); > > > > + u8 buf[10] = {0}; > > > > + > > > > + if (WARN_ON(len > 8)) > > > > + return -EINVAL; > > > > + > > > > + if (reg & REG_BCRM_V4L2) > > > > + addr += alvium->bcrm_addr; > > > > + > > > > + put_unaligned_be16(addr, buf); > > > > + put_unaligned_be64(val << (8 * (8 - len)), buf + 2); > > > > + if (i2c_master_send(client, buf, len + 2) != len + 2) > > > > + return -EIO; > > > > > > Same here. > > > > ^Same question. > > > > > > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_write_hshake(struct alvium_dev *alvium, > > > > + u32 reg, u64 val) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + u64 hshake_regval; > > > > + u8 hshake_bit; > > > > + int ret; > > > > + > > > > + if (!alvium->bcrm_addr) > > > > + return -EINVAL; > > > > + > > > > + /* reset handshake bit */ > > > > + ret = alvium_write(alvium, > > > > + REG_BCRM_WRITE_HANDSHAKE_RW, > > > > + BCRM_HANDSHAKE_W_RST); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to reset hshake reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + /* write alvium reg*/ > > > > + ret = alvium_write(alvium, reg, val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to write reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + /* poll handshake bit since bit0 = 1*/ > > > > + do { > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_WRITE_HANDSHAKE_RW, > > > > + &hshake_regval); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read hshake reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + hshake_bit = (hshake_regval & BCRM_HANDSHAKE_W_DONE_EN_BIT); > > > > + > > > > + } while (!hshake_bit); > > > > + > > > > + /* reset handshake bit, write 0 to bit0 */ > > > > + ret = alvium_write(alvium, > > > > + REG_BCRM_WRITE_HANDSHAKE_RW, > > > > + BCRM_HANDSHAKE_W_RST); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to reset hshake reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + /* poll handshake bit since bit0 = 0 */ > > > > + do { > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_WRITE_HANDSHAKE_RW, > > > > + &hshake_regval); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read hshake reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + hshake_bit = (hshake_regval & BCRM_HANDSHAKE_W_DONE_EN_BIT); > > > > + > > > > + } while (hshake_bit); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_get_bcrm_vers(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + int ret; > > > > + u64 val; > > > > + struct alvium_bcrm_vers *v; > > > > + > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_VERSION_R, > > > > + &val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read bcrm version reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + v = (struct alvium_bcrm_vers *) &val; > > > > + > > > > + v->minor = cpu_to_le16(v->minor); > > > > + v->major = cpu_to_le16(v->major); > > > > + > > > > + dev_info(dev, "bcrm version: %u.%u\n", v->minor, > > > > + v->major); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_get_fw_version(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + int ret; > > > > + u64 val; > > > > + struct alvium_fw_vers *fw_v; > > > > > > Please order temporary, loop etc. variables as last. Same for the other > > > functions. Reverse Christmas tree order is often the same. > > > > Can you explain me better pls? > > Your plan is the following order: > > > > struct device *dev = &alvium->i2c_client->dev; > > struct alvium_fw_vers *fw_v; > > u64 val; > > int ret; > > This is what I intended, yes. Fixed in v6. > > > > > or I'm wrong? Thanks. > > > > > > > > > + > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_DEVICE_FIRMWARE_VERSION_R, > > > > + &val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read fw version reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + fw_v = (struct alvium_fw_vers *) &val; > > > > + > > > > + fw_v->minor = cpu_to_le16(fw_v->minor); > > > > + fw_v->patch = cpu_to_le32(fw_v->patch); > > > > + > > > > + dev_info(dev, "fw version: %u.%u.%u.%u\n", > > > > + fw_v->special, > > > > > > Please align arguments to just right of the opening parenthesis. Also, more > > > fits on the same line (up to 80 chars). This applies to the entire patch. > > > > > > dev_info(dev, "fw version: %u.%u.%u.%u\n", fw_v->special, fw_v->major, > > fw_v->minor, fw_v->patch); > > > > Like this? > > Yes. Oooks > > > > > > > > > > + fw_v->major, > > > > + fw_v->minor, > > > > + fw_v->patch); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_get_bcrm_addr(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + int ret; > > > > + u64 val; > > > > + > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_REG_ADDR_R, > > > > + &val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read bcrm addr reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + alvium->bcrm_addr = (u16)val; > > > > + dev_dbg(dev, "bcrm addr: %d\n", alvium->bcrm_addr); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static bool alvium_is_alive(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + int ret; > > > > + u64 val = 0; > > > > + u8 hbeat_val = 0; > > > > + > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_VERSION_R, > > > > + &val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read bcrm version reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_HEARTBEAT_R, > > > > + &val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read heartbeat reg\n"); > > > > + return false; > > > > + } > > > > + > > > > + hbeat_val = (u8)val; > > > > + dev_dbg(dev, "hearthbeat = %d", hbeat_val); > > > > + > > > > + if (hbeat_val) > > > > + return true; > > > > + else > > > > + return false; > > > > > > return hbeat_val; > > > > Oks. > > > > > > > > > +} > > > > + > > > > +static void alvium_print_avail_mipi_fmt(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + > > > > + dev_dbg(dev, "avail mipi_fmt yuv420_8_leg: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_YUV420_8_LEG]); > > > > + dev_dbg(dev, "avail mipi_fmt yuv420_8: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_YUV420_8]); > > > > + dev_dbg(dev, "avail mipi_fmt yuv420_10: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_YUV420_10]); > > > > + dev_dbg(dev, "avail mipi_fmt yuv420_8_csps: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_YUV420_8_CSPS]); > > > > + dev_dbg(dev, "avail mipi_fmt yuv420_10_csps: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_YUV420_10_CSPS]); > > > > + dev_dbg(dev, "avail mipi_fmt yuv422_8: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_YUV422_8]); > > > > + dev_dbg(dev, "avail mipi_fmt yuv422_10: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_YUV422_10]); > > > > + dev_dbg(dev, "avail mipi_fmt rgb888: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RGB888]); > > > > + dev_dbg(dev, "avail mipi_fmt rgb666: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RGB666]); > > > > + dev_dbg(dev, "avail mipi_fmt rgb565: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RGB565]); > > > > + dev_dbg(dev, "avail mipi_fmt rgb555: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RGB555]); > > > > + dev_dbg(dev, "avail mipi_fmt rgb444: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RGB444]); > > > > + dev_dbg(dev, "avail mipi_fmt raw6: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RAW6]); > > > > + dev_dbg(dev, "avail mipi_fmt raw7: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RAW7]); > > > > + dev_dbg(dev, "avail mipi_fmt raw8: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RAW8]); > > > > + dev_dbg(dev, "avail mipi_fmt raw10: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RAW10]); > > > > + dev_dbg(dev, "avail mipi_fmt raw12: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RAW12]); > > > > + dev_dbg(dev, "avail mipi_fmt raw14: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_RAW14]); > > > > + dev_dbg(dev, "avail mipi_fmt jpeg: %u\n", > > > > + alvium->is_mipi_fmt_avail[ALVIUM_BIT_JPEG]); > > > > +} > > > > + > > > > +static void alvium_print_avail_feat(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + > > > > + dev_dbg(dev, "feature rev_x: %u\n", > > > > + alvium->avail_ft.rev_x); > > > > + dev_dbg(dev, "feature rev_y: %u\n", > > > > + alvium->avail_ft.rev_y); > > > > + dev_dbg(dev, "feature int_autop: %u\n", > > > > + alvium->avail_ft.int_autop); > > > > + dev_dbg(dev, "feature black_lvl: %u\n", > > > > + alvium->avail_ft.black_lvl); > > > > + dev_dbg(dev, "feature gain: %u\n", > > > > + alvium->avail_ft.gain); > > > > + dev_dbg(dev, "feature gamma: %u\n", > > > > + alvium->avail_ft.gamma); > > > > + dev_dbg(dev, "feature contrast: %u\n", > > > > + alvium->avail_ft.contrast); > > > > + dev_dbg(dev, "feature sat: %u\n", > > > > + alvium->avail_ft.sat); > > > > + dev_dbg(dev, "feature hue: %u\n", > > > > + alvium->avail_ft.hue); > > > > + dev_dbg(dev, "feature whiteb: %u\n", > > > > + alvium->avail_ft.whiteb); > > > > + dev_dbg(dev, "feature sharp: %u\n", > > > > + alvium->avail_ft.sharp); > > > > + dev_dbg(dev, "feature auto_exp: %u\n", > > > > + alvium->avail_ft.auto_exp); > > > > + dev_dbg(dev, "feature auto_gain: %u\n", > > > > + alvium->avail_ft.auto_gain); > > > > + dev_dbg(dev, "feature auto_whiteb: %u\n", > > > > + alvium->avail_ft.auto_whiteb); > > > > + dev_dbg(dev, "feature dev_temp: %u\n", > > > > + alvium->avail_ft.dev_temp); > > > > + dev_dbg(dev, "feature acq_abort: %u\n", > > > > + alvium->avail_ft.acq_abort); > > > > + dev_dbg(dev, "feature acq_fr: %u\n", > > > > + alvium->avail_ft.acq_fr); > > > > + dev_dbg(dev, "feature fr_trigger: %u\n", > > > > + alvium->avail_ft.fr_trigger); > > > > + dev_dbg(dev, "feature exp_acq_line: %u\n", > > > > + alvium->avail_ft.exp_acq_line); > > > > +} > > > > + > > > > +static void alvium_print_avail_bayer(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + > > > > + dev_dbg(dev, "avail bayer mono: %u\n", > > > > + alvium->is_bay_avail[ALVIUM_BIT_BAY_MONO]); > > > > + dev_dbg(dev, "avail bayer gr: %u\n", > > > > + alvium->is_bay_avail[ALVIUM_BIT_BAY_GR]); > > > > + dev_dbg(dev, "avail bayer rg: %u\n", > > > > + alvium->is_bay_avail[ALVIUM_BIT_BAY_RG]); > > > > + dev_dbg(dev, "avail bayer gb: %u\n", > > > > + alvium->is_bay_avail[ALVIUM_BIT_BAY_GB]); > > > > + dev_dbg(dev, "avail bayer bg: %u\n", > > > > + alvium->is_bay_avail[ALVIUM_BIT_BAY_BG]); > > > > +} > > > > + > > > > +static int alvium_get_feat_inq(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + struct alvium_avail_feat *f; > > > > + int ret; > > > > + u64 val; > > > > + > > > > + if (!alvium->bcrm_addr) > > > > + return -EINVAL; > > > > + > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_FEATURE_INQUIRY_R, > > > > + &val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to features inquiry reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + f = (struct alvium_avail_feat *) &val; > > > > + > > > > + alvium->avail_ft.rev_x = f->rev_x; > > > > + alvium->avail_ft.rev_y = f->rev_y; > > > > + alvium->avail_ft.int_autop = f->int_autop; > > > > + alvium->avail_ft.black_lvl = f->black_lvl; > > > > + alvium->avail_ft.gain = f->gain; > > > > + alvium->avail_ft.gamma = f->gamma; > > > > + alvium->avail_ft.contrast = f->contrast; > > > > + alvium->avail_ft.sat = f->sat; > > > > + alvium->avail_ft.hue = f->rev_y; > > > > + alvium->avail_ft.whiteb = f->whiteb; > > > > + alvium->avail_ft.sharp = f->sharp; > > > > + alvium->avail_ft.auto_exp = f->auto_exp; > > > > + alvium->avail_ft.auto_gain = f->auto_gain; > > > > + alvium->avail_ft.auto_whiteb = f->auto_whiteb; > > > > + alvium->avail_ft.dev_temp = f->dev_temp; > > > > + alvium->avail_ft.acq_abort = f->acq_abort; > > > > + alvium->avail_ft.acq_fr = f->acq_fr; > > > > + alvium->avail_ft.fr_trigger = f->fr_trigger; > > > > + alvium->avail_ft.exp_acq_line = f->exp_acq_line; > > > > > > You could instead do: > > > > > > alvium->avail_ft = *f; > > > > > > But is there an endianness issue here? I think you should use u64 also for > > > the struct fields. > > > > Mmmm.. I'm not sure are all u8 this is not safe (on endianess pesrpective)? > > Or I'm completely wrong? > > The u8 fields in the struct are fine but the u64 value is not. Instead you > should read a u8 array. This is fixed with new CCI Api right?? If yes, this is fixed in v6. > > > > > > > > > > + > > > > + alvium_print_avail_feat(alvium); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_get_host_supp_csi_lanes(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + int ret; > > > > + u64 val; > > > > + > > > > + if (!alvium->bcrm_addr) > > > > + return -EINVAL; > > > > + > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_CSI2_LANE_COUNT_RW, > > > > + &val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read host supported csi2 lanes reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + alvium->h_sup_csi_lanes = (u8)val; > > > > + dev_dbg(dev, "host supported csi2 lanes count: %d\n", > > > > + alvium->h_sup_csi_lanes); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_set_csi_lanes(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + int ret; > > > > + u64 num_lanes; > > > > + > > > > + if (!alvium->bcrm_addr) > > > > + return -EINVAL; > > > > + > > > > + num_lanes = alvium->ep.bus.mipi_csi2.num_data_lanes; > > > > + > > > > + if (num_lanes <= alvium->h_sup_csi_lanes) { > > > > + ret = alvium_write_hshake(alvium, > > > > + REG_BCRM_CSI2_LANE_COUNT_RW, > > > > + num_lanes); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to set csi lanes reg\n"); > > > > + return ret; > > > > + } > > > > + } else { > > > > + return -EINVAL; > > > > + } > > > > + > > > > + dev_dbg(dev, "Set n lanes = %llu", num_lanes); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_set_lp2hs_delay(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + int ret; > > > > + u64 lp2hs_delay; > > > > + > > > > + if (!alvium->bcrm_addr) > > > > + return -EINVAL; > > > > + > > > > + /* > > > > + * The purpose of this reg is force a DPhy reset > > > > + * for the period described by the microseconds on > > > > + * the reg, before it starts streaming. > > > > + * > > > > + * To be clear, with that value bigger than 0 the > > > > + * Alvium forces a dphy-reset on all lanes for that period. > > > > + * That means all lanes go up into low power state. > > > > + * > > > > + * This may help a csi2 rx ip to reset if that IP > > > > + * can't deal with a continous clock. > > > > + */ > > > > + lp2hs_delay = (u64)alvium->lp2hs_delay; > > > > + > > > > + ret = alvium_write_hshake(alvium, > > > > + REG_BCRM_STREAM_ON_DELAY_RW, > > > > + lp2hs_delay); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to set streamon delay reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_get_csi_clk_params(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + int ret; > > > > + u64 val; > > > > + > > > > + if (!alvium->bcrm_addr) > > > > + return -EINVAL; > > > > + > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_CSI2_CLOCK_MIN_R, > > > > + &val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read host supported csi2 lanes reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + alvium->min_csi_clk = (u32)val; > > > > + dev_dbg(dev, "Min host supported csi clock: %d\n", > > > > + alvium->min_csi_clk); > > > > + > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_CSI2_CLOCK_MAX_R, > > > > + &val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read host supported csi2 lanes reg\n"); > > > > + return ret; > > > > + } > > > > + > > > > + alvium->max_csi_clk = (u32)val; > > > > + dev_dbg(dev, "Max host supported csi clock: %d\n", > > > > + alvium->max_csi_clk); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_set_csi_clk(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + int ret; > > > > + u64 csi_clk; > > > > + > > > > + if (!alvium->bcrm_addr) > > > > + return -EINVAL; > > > > + > > > > + csi_clk = (u32)alvium->ep.link_frequencies[0]; > > > > + > > > > + if (csi_clk > alvium->min_csi_clk || > > > > + csi_clk < alvium->max_csi_clk) { > > > > + ret = alvium_write_hshake(alvium, > > > > + REG_BCRM_CSI2_CLOCK_RW, > > > > + csi_clk); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to set csi lanes reg\n"); > > > > + return ret; > > > > + } > > > > + } else { > > > > + return -EINVAL; > > > > + } > > > > + > > > > + alvium->link_freq = alvium->ep.link_frequencies[0]; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int alvium_get_img_width_params(struct alvium_dev *alvium) > > > > +{ > > > > + struct device *dev = &alvium->i2c_client->dev; > > > > + int ret; > > > > + u64 val; > > > > + > > > > + if (!alvium->bcrm_addr) > > > > + return -EINVAL; > > > > + > > > > + ret = alvium_read(alvium, > > > > + REG_BCRM_IMG_WIDTH_MIN_R, > > > > + &val); > > > > + if (ret) { > > > > + dev_err(dev, "Fail to read img min width reg\n"); > > > > + return ret; > > > > + } > > > > > > Could you add a macro that assigns the value to the variable (or a struct > > > field in this case) when the read is successful? Add the print if you think > > > you need it. > > > > I don't get this comment. > > Can you explain me better your plan please. > > You have exactly the same pattern repeated over and over in a number of > functions. I'd like you to add a macro (or a function) that takes what > varies as arguments, and call that function here. It would reduce a lot of > the repeated lines code here. Mmmm... I'll give a try on this in v7 > > ... > > > > > +static int alvium_s_ctrl(struct v4l2_ctrl *ctrl) > > > > +{ > > > > + struct v4l2_subdev *sd = ctrl_to_sd(ctrl); > > > > + struct alvium_dev *alvium = sd_to_alvium(sd); > > > > + int ret; > > > > + > > > > + switch (ctrl->id) { > > > > + case V4L2_CID_AUTOGAIN: > > > > + ret = alvium_set_ctrl_gain(alvium, ctrl->val); > > > > + break; > > > > + case V4L2_CID_EXPOSURE_AUTO: > > > > + ret = alvium_set_ctrl_exposure(alvium, ctrl->val); > > > > + break; > > > > + case V4L2_CID_AUTO_WHITE_BALANCE: > > > > + ret = alvium_set_ctrl_white_balance(alvium, ctrl->val); > > > > + break; > > > > + case V4L2_CID_HUE: > > > > + ret = alvium_set_ctrl_hue(alvium, ctrl->val); > > > > + break; > > > > + case V4L2_CID_CONTRAST: > > > > + ret = alvium_set_ctrl_contrast(alvium, ctrl->val); > > > > + break; > > > > + case V4L2_CID_SATURATION: > > > > + ret = alvium_set_ctrl_saturation(alvium, ctrl->val); > > > > + break; > > > > + case V4L2_CID_GAMMA: > > > > + ret = alvium_set_ctrl_gamma(alvium, ctrl->val); > > > > + break; > > > > + case V4L2_CID_SHARPNESS: > > > > + ret = alvium_set_ctrl_sharpness(alvium, ctrl->val); > > > > + break; > > > > + case V4L2_CID_HFLIP: > > > > + ret = alvium_set_ctrl_hflip(alvium, ctrl->val); > > > > + break; > > > > + case V4L2_CID_VFLIP: > > > > + ret = alvium_set_ctrl_vflip(alvium, ctrl->val); > > > > + break; > > > > > > No hsync or vsync controls? > > > > This is not a sensor Alvium ISP/Vision core > > is responsible for timings checking of the sensor mounted. > > And those values are unknown then? > > Generally raw cameras are controlled configuring the blanking. Of course if > these aren't available, we can't expose them to user space either. Right, these are not available. > > ... > > > > > + ret = fwnode_property_read_u32(dev_fwnode(dev), > > > > + "alliedvision,lp2hs-delay-us", > > > > + &alvium->lp2hs_delay); > > > > + > > > > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); > > > > + if (!endpoint) { > > > > + dev_err(dev, "endpoint node not found\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > > > As you have D-PHY, please assign bus type to D-PHY before parsing the > > > endpoint. > > > > I don't completely got this. > > You mean, check: > > > > if (alvium->ep.bus_type != V4L2_MBUS_CSI2_DPHY) { > > dev_err(dev, "unsupported bus type\n"); > > return -EINVAL; > > } > > > > before parsing or I'm wrong? > > Please read v4l2_fwnode_endpoint_alloc_parse() documentation in > include/media/v4l2-fwnode.h . I Think this is fixed in v6. Pls check. > > ... > > > > > +struct alvium_avail_mipi_fmt { > > > > + u8 yuv420_8_leg:1; > > > > + u8 yuv420_8:1; > > > > + u8 yuv420_10:1; > > > > + u8 yuv420_8_csps:1; > > > > + u8 yuv420_10_csps:1; > > > > + u8 yuv422_8:1; > > > > + u8 yuv422_10:1; > > > > + u8 rgb888:1; > > > > + u8 rgb666:1; > > > > + u8 rgb565:1; > > > > + u8 rgb555:1; > > > > + u8 rgb444:1; > > > > + u8 raw6:1; > > > > + u8 raw7:1; > > > > + u8 raw8:1; > > > > + u8 raw10:1; > > > > + u8 raw12:1; > > > > + u8 raw14:1; > > > > + u8 jpeg:1; > > > > > > I don't think C defines a particular location for the bit fields if their > > > size is less than that of the type. Could you add padding to ensure that? > > > Same for other structs. > > > > I take "struct s5k5baf" of drivers/media/i2c/s5k5baf.c > > as reference. I'm missing something? Can you explain me better? > > How is this related to the comment, other than that it uses bit fields? Yes. > > > > > Thanks! > > > > > > > > > > > > > +}; > > > > + > > > > +struct alvium_avail_bayer { > > > > + u8 mono:1; > > > > + u8 gr:1; > > > > + u8 rg:1; > > > > + u8 gb:1; > > > > + u8 bg:1; > > > > +}; > > > > + > > > > +struct alvium_mode { > > > > + struct v4l2_rect crop; > > > > + struct v4l2_mbus_framefmt fmt; > > > > + u32 width; > > > > + u32 height; > > > > +}; > > > > + > > > > +struct alvium_pixfmt { > > > > + u8 id; > > > > + u32 code; > > > > + u32 colorspace; > > > > + u8 fmt_av_bit; > > > > + u8 bay_av_bit; > > > > + u64 mipi_fmt_regval; > > > > + u64 bay_fmt_regval; > > > > + u8 is_raw; > > > > > > This would require less memory if you moved all 8-bit fields to the end of > > > the struct. > > > > Also Laurent and Cristophe point me about this, thanks. > > > > If is possible I prefer to keep this order this is more > > clear for me. > > > > But If not fit your plan never mind I pick your order. > > Thanks to all! :) > > If you've received the same comment from three reviewers, then please do > it. :-) Pls check v6. Resume: I'm going to send v7 with some hints from Laurent, and some suggestion from you and Hans. Also I have to fix .h indentation. Please meanwhile take your time to check v6. Thanks again for your review! :) Regards, Tommaso > > -- > Kind regards, > > Sakari Ailus
