Hi Laurent,
On 22/04/2022 15:29, Laurent Pinchart wrote:
> Hi Benjamin,
>
> On Fri, Apr 22, 2022 at 10:44:13AM +0200, Benjamin Mugnier wrote:
>> On 20/04/2022 18:48, Laurent Pinchart wrote:
>>> On Wed, Apr 20, 2022 at 03:01:24PM +0200, Benjamin Mugnier wrote:
>>>> On 15/04/2022 17:39, Laurent Pinchart wrote:
>>>>> On Fri, Apr 15, 2022 at 01:18:45PM +0200, Benjamin Mugnier wrote:
>>>>>> The VGXY61 has a quad lanes CSI-2 output port running at 800mbps per
>>>>>> lane, and supports RAW8, RAW10, RAW12, RAW14 and RAW16 formats.
>>>>>> The driver handles both sensor types:
>>>>>> - VG5661 and VG6661: 1.6 Mpx (1464 x 1104) 75fps.
>>>>>> - VG5761 and VG6761: 2.3 Mpx (1944 x 1204) 60 fps.
>>>>>> The driver supports:
>>>>>> - HDR linearize mode, HDR substraction mode, and no HDR
>>>>>> - GPIOs LEDs strobing
>>>>>> - Digital binning and analog subsampling
>>>>>> - Horizontal and vertical flip
>>>>>> - Manual exposure
>>>>>> - Analog and digital gains
>>>>>> - Test patterns
>>>>>>
>>>>>> Signed-off-by: Benjamin Mugnier <benjamin.mugnier@xxxxxxxxxxx>
>>>>>> ---
>>>>>> drivers/media/i2c/Kconfig | 10 +
>>>>>> drivers/media/i2c/Makefile | 1 +
>>>>>> drivers/media/i2c/st-vgxy61.c | 1997 +++++++++++++++++++++++++++++++++
>>>>>> 3 files changed, 2008 insertions(+)
>>>>>> create mode 100644 drivers/media/i2c/st-vgxy61.c
>>>>>>
>>>>>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>>>>>> index fae2baabb773..734f3e1130b0 100644
>>>>>> --- a/drivers/media/i2c/Kconfig
>>>>>> +++ b/drivers/media/i2c/Kconfig
>>>>>> @@ -763,6 +763,16 @@ config VIDEO_VS6624
>>>>>> To compile this driver as a module, choose M here: the
>>>>>> module will be called vs6624.
>>>>>>
>>>>>> +config VIDEO_ST_VGXY61
>>>>>
>>>>> Alphabetical order please.
>>>>>
>>>>>> + tristate "ST VGXY61 sensor support"
>>>>>> + depends on OF && GPIOLIB && VIDEO_DEV && I2C
>>>>>> + select MEDIA_CONTROLLER
>>>>>> + select VIDEO_V4L2_SUBDEV_API
>>>>>> + select V4L2_FWNODE
>>>>>> + help
>>>>>> + This is a Video4Linux2 sensor driver for the ST VGXY61
>>>>>> + camera sensor.
>>>>>> +
>>>>>> source "drivers/media/i2c/ccs/Kconfig"
>>>>>> source "drivers/media/i2c/et8ek8/Kconfig"
>>>>>> source "drivers/media/i2c/m5mols/Kconfig"
>>>>>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>>>>>> index 3e1696963e7f..7f2362228484 100644
>>>>>> --- a/drivers/media/i2c/Makefile
>>>>>> +++ b/drivers/media/i2c/Makefile
>>>>>> @@ -116,6 +116,7 @@ obj-$(CONFIG_VIDEO_SAA7185) += saa7185.o
>>>>>> obj-$(CONFIG_VIDEO_SONY_BTF_MPX) += sony-btf-mpx.o
>>>>>> obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o
>>>>>> obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
>>>>>> +obj-$(CONFIG_VIDEO_ST_VGXY61) += st-vgxy61.o
>>>>>> obj-$(CONFIG_VIDEO_TC358743) += tc358743.o
>>>>>> obj-$(CONFIG_VIDEO_TDA1997X) += tda1997x.o
>>>>>> obj-$(CONFIG_VIDEO_TDA7432) += tda7432.o
>>>>>> diff --git a/drivers/media/i2c/st-vgxy61.c b/drivers/media/i2c/st-vgxy61.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..1219d278e648
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/media/i2c/st-vgxy61.c
>>>>>> @@ -0,0 +1,1997 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/*
>>>>>> + * Driver for VGXY61 global shutter sensor family driver
>>>>>> + *
>>>>>> + * Copyright (C) 2022 STMicroelectronics SA
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/clk.h>
>>>>>> +#include <linux/delay.h>
>>>>>> +#include <linux/gpio/consumer.h>
>>>>>> +#include <linux/i2c.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/regulator/consumer.h>
>>>>>> +#include <linux/pm_runtime.h>
>>>>>
>>>>> Alphabetical order in headers too please.
>>>>>
>>>>>> +#include <linux/units.h>
>>>>>> +#include <linux/iopoll.h>
>>>>>> +#include <media/mipi-csi2.h>
>>>>>> +#include <media/v4l2-async.h>
>>>>>> +#include <media/v4l2-ctrls.h>
>>>>>> +#include <media/v4l2-device.h>
>>>>>> +#include <media/v4l2-fwnode.h>
>>>>>> +#include <media/v4l2-subdev.h>
>>>>>> +
>>>>>> +#define US_PER_MS 1000
>>>>>
>>>>> I don't think this deserves a macro, but if you want to keep it, you
>>>>> should avoid generic names that may cause namespace clashes.
>>>>>
>>>>
>>>> I don't make heavy usage of it so I will remove it.
>>>>
>>>>>> +
>>>>>> +#define DEVICE_MODEL_ID_REG 0x0000
>>>>>
>>>>> Please prefix macros with a driver-specific prefix. Many of them have a
>>>>> too generic name.
>>>>>
>>>>
>>>> I will change this to VGXY61_REG_* for registers, and simply VGXY61_* for others.
>>>>
>>>>>> +#define VG5661_MODEL_ID 0x5661
>>>>>> +#define VG5761_MODEL_ID 0x5761
>>>>>> +#define VGX661_WIDTH 1464
>>>>>> +#define VGX661_HEIGHT 1104
>>>>>> +#define VGX761_WIDTH 1944
>>>>>> +#define VGX761_HEIGHT 1204
>>>>>> +#define VGX661_DEFAULT_MODE 1
>>>>>> +#define VGX761_DEFAULT_MODE 1
>>>>>> +#define VGX661_SHORT_ROT_TERM 93
>>>>>> +#define VGX761_SHORT_ROT_TERM 90
>>>>>> +#define VGXY61_EXPOS_ROT_TERM 66
>>>>>> +#define VGXY61_WRITE_MULTIPLE_CHUNK_MAX 16
>>>>>> +#define VGXY61_NB_GPIOS 4
>>>>>> +#define VGXY61_NB_POLARITIES 5
>>>>>> +#define VGXY61_MIN_FRAME_LENGTH 1288
>>>>>> +#define VGXY61_MIN_EXPOSURE 10
>>>>>> +#define VGXY61_HDR_LINEAR_RATIO 10
>>>>>> +#define VGXY61_TIMEOUT_MS 500
>>>>>> +#define VGXY61_FRAME_LENGTH_DEF 1313
>>>>>> +#define DEVICE_REVISION 0x0002
>>>>>> +#define DEVICE_FWPATCH_REVISION 0x0014
>>>>>> +#define DEVICE_FWPATCH_START_ADDR 0x2000
>>>>>> +#define DEVICE_SYSTEM_FSM 0x0020
>>>>>> +#define SW_STBY 0x03
>>>>>> +#define STREAMING 0x04
>>>>>> +#define DEVICE_NVM 0x0023
>>>>>> +#define NVM_OK 0x04
>>>>>> +#define DEVICE_THSENS1_TEMPERATURE 0x0044
>>>>>> +#define DEVICE_STBY 0x0201
>>>>>> +#define STBY_NO_REQ 0
>>>>>> +#define STBY_REQ_TMP_READ BIT(2)
>>>>>> +#define DEVICE_STREAMING 0x0202
>>>>>> +#define REQ_NO_REQUEST 0
>>>>>> +#define REQ_STOP_STREAMING BIT(0)
>>>>>> +#define REQ_START_STREAMING BIT(1)
>>>>>> +#define DEVICE_EXT_CLOCK 0x0220
>>>>>> +#define DEVICE_CLK_PLL_PREDIV 0x0224
>>>>>> +#define DEVICE_CLK_SYS_PLL_MULT 0x0225
>>>>>> +#define DEVICE_GPIO_0_CTRL 0x0236
>>>>>> +#define DEVICE_GPIO_1_CTRL 0x0237
>>>>>> +#define DEVICE_GPIO_2_CTRL 0x0238
>>>>>> +#define DEVICE_GPIO_3_CTRL 0x0239
>>>>>> +#define DEVICE_SIGNALS_POLARITY_CTRL 0x023b
>>>>>> +#define DEVICE_LINE_LENGTH 0x0300
>>>>>> +#define DEVICE_ORIENTATION 0x0302
>>>>>> +#define DEVICE_VT_CTRL 0x0304
>>>>>> +#define DEVICE_FORMAT_CTRL 0x0305
>>>>>> +#define DEVICE_OIF_CTRL 0x0306
>>>>>> +#define DEVICE_OIF_ROI0_CTRL 0x030a
>>>>>> +#define DEVICE_ROI0_START_H 0x0400
>>>>>> +#define DEVICE_ROI0_START_V 0x0402
>>>>>> +#define DEVICE_ROI0_END_H 0x0404
>>>>>> +#define DEVICE_ROI0_END_V 0x0406
>>>>>> +#define DEVICE_PATGEN_CTRL 0x0440
>>>>>> +#define DEVICE_FRAME_CONTENT_CTRL 0x0478
>>>>>> +#define DEVICE_COARSE_EXPOSURE_LONG 0x0500
>>>>>> +#define DEVICE_COARSE_EXPOSURE_SHORT 0x0504
>>>>>> +#define DEVICE_ANALOG_GAIN 0x0508
>>>>>> +#define DEVICE_DIGITAL_GAIN_LONG 0x050a
>>>>>> +#define DEVICE_DIGITAL_GAIN_SHORT 0x0512
>>>>>> +#define DEVICE_FRAME_LENGTH 0x051a
>>>>>> +#define DEVICE_SIGNALS_CTRL 0x0522
>>>>>> +#define DEVICE_READOUT_CTRL 0x0530
>>>>>> +#define DEVICE_HDR_CTRL 0x0532
>>>>>> +#define DEVICE_PATGEN_LONG_DATA_GR 0x092c
>>>>>> +#define DEVICE_PATGEN_LONG_DATA_R 0x092e
>>>>>> +#define DEVICE_PATGEN_LONG_DATA_B 0x0930
>>>>>> +#define DEVICE_PATGEN_LONG_DATA_GB 0x0932
>>>>>> +#define DEVICE_PATGEN_SHORT_DATA_GR 0x0950
>>>>>> +#define DEVICE_PATGEN_SHORT_DATA_R 0x0952
>>>>>> +#define DEVICE_PATGEN_SHORT_DATA_B 0x0954
>>>>>> +#define DEVICE_PATGEN_SHORT_DATA_GB 0x0956
>>>>>> +#define DEVICE_BYPASS_CTRL 0x0a60
>>>>>> +
>>>>>> +#define V4L2_CID_HDR (V4L2_CID_USER_STVGXY61_BASE + 0)
>>>>>
>>>>> This macro should have a device-specific name, should be moved to a
>>>>> public header, and the control should be documented. I would split the
>>>>> control documentation and macro definition to a separate patch.
>>>>>
>>>>
>>>> I created two new files include/uapi/linux/st-vgxy61.h and
>>>> Documentation/userspace-api/media/drivers/st-vgxy61.rst.
>>>>
>>>> I'm not sure what you mean by separating the documentation and
>>>> definition. As there is a significant amount of code dependent on the
>>>> HDR mode I will keep the public header in this patch, and create a new
>>>> patch for the documentation. Tell me if there is any issue with this.
>>>
>>> I meant a patch that adds include/uapi/linux/st-vgxy61.h and
>>> Documentation/userspace-api/media/drivers/st-vgxy61.rst separate from
>>> the driver, but if it causes more issues than it helps review, you can
>>> combine them in a single patch.
>>
>> Ok, I initially thought about moving the relevant st-vgxy61.c hdr code
>> to a new patch, which will be quite complicated.
>> Only definition and documentation make no issue.
>>
>>>>>> +
>>>>>> +#define DEVICE_FWPATCH_REVISION_MAJOR 2
>>>>>> +#define DEVICE_FWPATCH_REVISION_MINOR 0
>>>>>> +#define DEVICE_FWPATCH_REVISION_MICRO 5
>>>>>> +
>>>>>> +static const u8 patch_array[] = {
>>>>>> + 0xbf, 0x00, 0x05, 0x20, 0x06, 0x01, 0xe0, 0xe0, 0x04, 0x80, 0xe6, 0x45,
>>>>>> + 0xed, 0x6f, 0xfe, 0xff, 0x14, 0x80, 0x1f, 0x84, 0x10, 0x42, 0x05, 0x7c,
>>>>>> + 0x01, 0xc4, 0x1e, 0x80, 0xb6, 0x42, 0x00, 0xe0, 0x1e, 0x82, 0x1e, 0xc0,
>>>>>> + 0x93, 0xdd, 0xc3, 0xc1, 0x0c, 0x04, 0x00, 0xfa, 0x86, 0x0d, 0x70, 0xe1,
>>>>>> + 0x04, 0x98, 0x15, 0x00, 0x28, 0xe0, 0x14, 0x02, 0x08, 0xfc, 0x15, 0x40,
>>>>>> + 0x28, 0xe0, 0x98, 0x58, 0xe0, 0xef, 0x04, 0x98, 0x0e, 0x04, 0x00, 0xf0,
>>>>>> + 0x15, 0x00, 0x28, 0xe0, 0x19, 0xc8, 0x15, 0x40, 0x28, 0xe0, 0xc6, 0x41,
>>>>>> + 0xfc, 0xe0, 0x14, 0x80, 0x1f, 0x84, 0x14, 0x02, 0xa0, 0xfc, 0x1e, 0x80,
>>>>>> + 0x14, 0x80, 0x14, 0x02, 0x80, 0xfb, 0x14, 0x02, 0xe0, 0xfc, 0x1e, 0x80,
>>>>>> + 0x14, 0xc0, 0x1f, 0x84, 0x14, 0x02, 0xa4, 0xfc, 0x1e, 0xc0, 0x14, 0xc0,
>>>>>> + 0x14, 0x02, 0x80, 0xfb, 0x14, 0x02, 0xe4, 0xfc, 0x1e, 0xc0, 0x0c, 0x0c,
>>>>>> + 0x00, 0xf2, 0x93, 0xdd, 0x86, 0x00, 0xf8, 0xe0, 0x04, 0x80, 0xc6, 0x03,
>>>>>> + 0x70, 0xe1, 0x0e, 0x84, 0x93, 0xdd, 0xc3, 0xc1, 0x0c, 0x04, 0x00, 0xfa,
>>>>>> + 0x6b, 0x80, 0x06, 0x40, 0x6c, 0xe1, 0x04, 0x80, 0x09, 0x00, 0xe0, 0xe0,
>>>>>> + 0x0b, 0xa1, 0x95, 0x84, 0x05, 0x0c, 0x1c, 0xe0, 0x86, 0x02, 0xf9, 0x60,
>>>>>> + 0xe0, 0xcf, 0x78, 0x6e, 0x80, 0xef, 0x25, 0x0c, 0x18, 0xe0, 0x05, 0x4c,
>>>>>> + 0x1c, 0xe0, 0x86, 0x02, 0xf9, 0x60, 0xe0, 0xcf, 0x0b, 0x84, 0xd8, 0x6d,
>>>>>> + 0x80, 0xef, 0x05, 0x4c, 0x18, 0xe0, 0x04, 0xd8, 0x0b, 0xa5, 0x95, 0x84,
>>>>>> + 0x05, 0x0c, 0x2c, 0xe0, 0x06, 0x02, 0x01, 0x60, 0xe0, 0xce, 0x18, 0x6d,
>>>>>> + 0x80, 0xef, 0x25, 0x0c, 0x30, 0xe0, 0x05, 0x4c, 0x2c, 0xe0, 0x06, 0x02,
>>>>>> + 0x01, 0x60, 0xe0, 0xce, 0x0b, 0x84, 0x78, 0x6c, 0x80, 0xef, 0x05, 0x4c,
>>>>>> + 0x30, 0xe0, 0x0c, 0x0c, 0x00, 0xf2, 0x93, 0xdd, 0x46, 0x01, 0x70, 0xe1,
>>>>>> + 0x08, 0x80, 0x0b, 0xa1, 0x08, 0x5c, 0x00, 0xda, 0x06, 0x01, 0x68, 0xe1,
>>>>>> + 0x04, 0x80, 0x4a, 0x40, 0x84, 0xe0, 0x08, 0x5c, 0x00, 0x9a, 0x06, 0x01,
>>>>>> + 0xe0, 0xe0, 0x04, 0x80, 0x15, 0x00, 0x60, 0xe0, 0x19, 0xc4, 0x15, 0x40,
>>>>>> + 0x60, 0xe0, 0x15, 0x00, 0x78, 0xe0, 0x19, 0xc4, 0x15, 0x40, 0x78, 0xe0,
>>>>>> + 0x93, 0xdd, 0xc3, 0xc1, 0x46, 0x01, 0x70, 0xe1, 0x08, 0x80, 0x0b, 0xa1,
>>>>>> + 0x08, 0x5c, 0x00, 0xda, 0x06, 0x01, 0x68, 0xe1, 0x04, 0x80, 0x4a, 0x40,
>>>>>> + 0x84, 0xe0, 0x08, 0x5c, 0x00, 0x9a, 0x06, 0x01, 0xe0, 0xe0, 0x14, 0x80,
>>>>>> + 0x25, 0x02, 0x54, 0xe0, 0x29, 0xc4, 0x25, 0x42, 0x54, 0xe0, 0x24, 0x80,
>>>>>> + 0x35, 0x04, 0x6c, 0xe0, 0x39, 0xc4, 0x35, 0x44, 0x6c, 0xe0, 0x25, 0x02,
>>>>>> + 0x64, 0xe0, 0x29, 0xc4, 0x25, 0x42, 0x64, 0xe0, 0x04, 0x80, 0x15, 0x00,
>>>>>> + 0x7c, 0xe0, 0x19, 0xc4, 0x15, 0x40, 0x7c, 0xe0, 0x93, 0xdd, 0xc3, 0xc1,
>>>>>> + 0x4c, 0x04, 0x7c, 0xfa, 0x86, 0x40, 0x98, 0xe0, 0x14, 0x80, 0x1b, 0xa1,
>>>>>> + 0x06, 0x00, 0x00, 0xc0, 0x08, 0x42, 0x38, 0xdc, 0x08, 0x64, 0xa0, 0xef,
>>>>>> + 0x86, 0x42, 0x3c, 0xe0, 0x68, 0x49, 0x80, 0xef, 0x6b, 0x80, 0x78, 0x53,
>>>>>> + 0xc8, 0xef, 0xc6, 0x54, 0x6c, 0xe1, 0x7b, 0x80, 0xb5, 0x14, 0x0c, 0xf8,
>>>>>> + 0x05, 0x14, 0x14, 0xf8, 0x1a, 0xac, 0x8a, 0x80, 0x0b, 0x90, 0x38, 0x55,
>>>>>> + 0x80, 0xef, 0x1a, 0xae, 0x17, 0xc2, 0x03, 0x82, 0x88, 0x65, 0x80, 0xef,
>>>>>> + 0x1b, 0x80, 0x0b, 0x8e, 0x68, 0x65, 0x80, 0xef, 0x9b, 0x80, 0x0b, 0x8c,
>>>>>> + 0x08, 0x65, 0x80, 0xef, 0x6b, 0x80, 0x0b, 0x92, 0x1b, 0x8c, 0x98, 0x64,
>>>>>> + 0x80, 0xef, 0x1a, 0xec, 0x9b, 0x80, 0x0b, 0x90, 0x95, 0x54, 0x10, 0xe0,
>>>>>> + 0xa8, 0x53, 0x80, 0xef, 0x1a, 0xee, 0x17, 0xc2, 0x03, 0x82, 0xf8, 0x63,
>>>>>> + 0x80, 0xef, 0x1b, 0x80, 0x0b, 0x8e, 0xd8, 0x63, 0x80, 0xef, 0x1b, 0x8c,
>>>>>> + 0x68, 0x63, 0x80, 0xef, 0x6b, 0x80, 0x0b, 0x92, 0x65, 0x54, 0x14, 0xe0,
>>>>>> + 0x08, 0x65, 0x84, 0xef, 0x68, 0x63, 0x80, 0xef, 0x7b, 0x80, 0x0b, 0x8c,
>>>>>> + 0xa8, 0x64, 0x84, 0xef, 0x08, 0x63, 0x80, 0xef, 0x14, 0xe8, 0x46, 0x44,
>>>>>> + 0x94, 0xe1, 0x24, 0x88, 0x4a, 0x4e, 0x04, 0xe0, 0x14, 0xea, 0x1a, 0x04,
>>>>>> + 0x08, 0xe0, 0x0a, 0x40, 0x84, 0xed, 0x0c, 0x04, 0x00, 0xe2, 0x4a, 0x40,
>>>>>> + 0x04, 0xe0, 0x19, 0x16, 0xc0, 0xe0, 0x0a, 0x40, 0x84, 0xed, 0x21, 0x54,
>>>>>> + 0x60, 0xe0, 0x0c, 0x04, 0x00, 0xe2, 0x1b, 0xa5, 0x0e, 0xea, 0x01, 0x89,
>>>>>> + 0x21, 0x54, 0x64, 0xe0, 0x7e, 0xe8, 0x65, 0x82, 0x1b, 0xa7, 0x26, 0x00,
>>>>>> + 0x00, 0x80, 0xa5, 0x82, 0x1b, 0xa9, 0x65, 0x82, 0x1b, 0xa3, 0x01, 0x85,
>>>>>> + 0x16, 0x00, 0x00, 0xc0, 0x01, 0x54, 0x04, 0xf8, 0x06, 0xaa, 0x01, 0x83,
>>>>>> + 0x06, 0xa8, 0x65, 0x81, 0x06, 0xa8, 0x01, 0x54, 0x04, 0xf8, 0x01, 0x83,
>>>>>> + 0x06, 0xaa, 0x09, 0x14, 0x18, 0xf8, 0x0b, 0xa1, 0x05, 0x84, 0xc6, 0x42,
>>>>>> + 0xd4, 0xe0, 0x14, 0x84, 0x01, 0x83, 0x01, 0x54, 0x60, 0xe0, 0x01, 0x54,
>>>>>> + 0x64, 0xe0, 0x0b, 0x02, 0x90, 0xe0, 0x10, 0x02, 0x90, 0xe5, 0x01, 0x54,
>>>>>> + 0x88, 0xe0, 0xb5, 0x81, 0xc6, 0x40, 0xd4, 0xe0, 0x14, 0x80, 0x0b, 0x02,
>>>>>> + 0xe0, 0xe4, 0x10, 0x02, 0x31, 0x66, 0x02, 0xc0, 0x01, 0x54, 0x88, 0xe0,
>>>>>> + 0x1a, 0x84, 0x29, 0x14, 0x10, 0xe0, 0x1c, 0xaa, 0x2b, 0xa1, 0xf5, 0x82,
>>>>>> + 0x25, 0x14, 0x10, 0xf8, 0x2b, 0x04, 0xa8, 0xe0, 0x20, 0x44, 0x0d, 0x70,
>>>>>> + 0x03, 0xc0, 0x2b, 0xa1, 0x04, 0x00, 0x80, 0x9a, 0x02, 0x40, 0x84, 0x90,
>>>>>> + 0x03, 0x54, 0x04, 0x80, 0x4c, 0x0c, 0x7c, 0xf2, 0x93, 0xdd, 0x00, 0x00,
>>>>>> + 0x02, 0xa9, 0x00, 0x00, 0x64, 0x4a, 0x40, 0x00, 0x08, 0x2d, 0x58, 0xe0,
>>>>>> + 0xa8, 0x98, 0x40, 0x00, 0x28, 0x07, 0x34, 0xe0, 0x05, 0xb9, 0x00, 0x00,
>>>>>> + 0x28, 0x00, 0x41, 0x05, 0x88, 0x00, 0x41, 0x3c, 0x98, 0x00, 0x41, 0x52,
>>>>>> + 0x04, 0x01, 0x41, 0x79, 0x3c, 0x01, 0x41, 0x6a, 0x3d, 0xfe, 0x00, 0x00,
>>>>>> +};
>>>>>> +
>>>>>> +static const char * const vgxy61_test_pattern_menu[] = {
>>>>>> + "Disabled",
>>>>>> + "Solid",
>>>>>> + "Colorbar",
>>>>>> + "Gradbar",
>>>>>> + "Hgrey",
>>>>>> + "Vgrey",
>>>>>> + "Dgrey",
>>>>>> + "PN28",
>>>>>> +};
>>>>>> +
>>>>>> +static const char * const vgxy61_hdr_modes[] = {
>>>>>> + "HDR linearize",
>>>>>> + "HDR substraction",
>>>>>> + "no HDR",
>>>>>> +};
>>>>>> +
>>>>>> +/* Regulator supplies */
>>>>>> +static const char * const vgxy61_supply_name[] = {
>>>>>> + "VCORE",
>>>>>> + "VDDIO",
>>>>>> + "VANA",
>>>>>> +};
>>>>>> +
>>>>>> +static const s64 link_freq[] = {
>>>>>> + /*
>>>>>> + * MIPI output freq is 804Mhz / 2, as it uses both rising edge and falling edges to send
>>>>>> + * data
>>>>>> + */
>>>>>> + 402000000ULL
>>>>>
>>>>> The link frequencies should come from DT.
>>>>
>>>> This is a hard requirement for this sensor. This value can not be
>>>> modified and must be 402MHz.
>>>>
>>>> I see the ov9734.c does something similar. Is adding this value to the
>>>> device tree really mandatory?
>>>
>>> I suppose it's fine then. Sakari, what do you think ?
>>>
>>>>>> +};
>>>>>> +
>>>>>> +#define VGXY61_NUM_SUPPLIES ARRAY_SIZE(vgxy61_supply_name)
>>>>>> +
>>>>>> +enum bin_mode {
>>>>>
>>>>> Same comment here, all enums and structures should use a driver-specific
>>>>> prefix.
>>>>>
>>>>>> + BIN_MODE_NORMAL,
>>>>>> + BIN_MODE_DIGITAL_X2,
>>>>>> + BIN_MODE_DIGITAL_X4,
>>>>>> +};
>>>>>> +
>>>>>> +enum hdr {
>>>>>> + HDR_LINEAR,
>>>>>> + HDR_SUB,
>>>>>> + NO_HDR,
>>>>>> +};
>>>>>> +
>>>>>> +enum strobe_modes {
>>>>>> + STROBE_DISABLED,
>>>>>> + STROBE_LONG,
>>>>>> + STROBE_ENABLED,
>>>>>> +};
>>>>>> +
>>>>>> +struct vgxy61_mode_info {
>>>>>> + u32 width;
>>>>>> + u32 height;
>>>>>> + enum bin_mode bin_mode;
>>>>>> + struct v4l2_rect crop;
>>>>>> +};
>>>>>> +
>>>>>> +static const u32 vgxy61_supported_codes[] = {
>>>>>> + MEDIA_BUS_FMT_Y8_1X8,
>>>>>> + MEDIA_BUS_FMT_Y10_1X10,
>>>>>> + MEDIA_BUS_FMT_Y12_1X12,
>>>>>> + MEDIA_BUS_FMT_Y14_1X14,
>>>>>> + MEDIA_BUS_FMT_Y16_1X16
>>>>>> +};
>>>>>> +
>>>>>> +static const struct vgxy61_mode_info vgx661_mode_data[] = {
>>>>>> + {
>>>>>> + .width = VGX661_WIDTH,
>>>>>> + .height = VGX661_HEIGHT,
>>>>>> + .bin_mode = BIN_MODE_NORMAL,
>>>>>> + .crop = {
>>>>>> + .left = 0,
>>>>>> + .top = 0,
>>>>>> + .width = VGX661_WIDTH,
>>>>>> + .height = VGX661_HEIGHT,
>>>>>> + },
>>>>>
>>>>> It's very nice to see no hardcoded register values in the modes tables
>>>>> :-) As the register values are computed by the driver, would it make
>>>>> sense to go one step further and drop the modes completely ? Analog crop
>>>>> and binning could then be selected freely from userspace.
>>>>
>>>> All credit goes to Dave for pointing this in the first review ;)
>>>> As for the modes, in theory we could drop them completely, but this
>>>> will require some extra testing as the sensor can't really handle
>>>> every single mode freely.
>>>>
>>>> Could this be done in a second step?
>>>
>>> I'm a bit concerned it could then break userspace, if some applications
>>> start depending on a fixed list of modes. How difficult do you think
>>> this would be ?
>>
>> I mean if they use a fixed list, the upgrade will still allow them to
>> use this list right? As it will implement a superset of the resolution
>> list.
>>
>> Also if I understand correctly they will get this list dynamically
>> from the get_fmt call, hence once the full support is added the
>> application will update itself right?
>
> The idea is that we could get rid of the modes list completely, and let
> the application configure everything through the formats and selection
> rectangles. Moving in that direction later may cause some regressions,
> for instance an application may set a mode that is not supported, with
> the driver picking the closest option now, and later it may get the
> exact resolution it requested. It's hard to know if this could break
> something.
>
>> For the complexity I need to check various rules to make sure the
>> sensor will not be in a bad state. On top of my head the X and Y must
>> be even, binning must match exactly the format ratio, and certainly
>> other stuff I have not been confronted to yet. And of course testing.
>
> Those rules don't seem to be too complicated. Testing will likely be the
> biggest time consumer :-)
>
I will give it go for v3 then. If I run into issues I could still remove the find_nearest_size call and return an error if the exact size is not supported, but appart from the time consumming part this should go well.
>> Tell me what you think.
>>
>>>>>> + },
>>>>>> + {
>>>>>> + .width = 1280,
>>>>>> + .height = 720,
>>>>>> + .bin_mode = BIN_MODE_NORMAL,
>>>>>> + .crop = {
>>>>>> + .left = 92,
>>>>>> + .top = 192,
>>>>>> + .width = 1280,
>>>>>> + .height = 720,
>>>>>> + },
>>>>>> + },
>>>>>> + {
>>>>>> + .width = 640,
>>>>>> + .height = 480,
>>>>>> + .bin_mode = BIN_MODE_DIGITAL_X2,
>>>>>> + .crop = {
>>>>>> + .left = 92,
>>>>>> + .top = 72,
>>>>>> + .width = 1280,
>>>>>> + .height = 960,
>>>>>> + },
>>>>>> + },
>>>>>> + {
>>>>>> + .width = 320,
>>>>>> + .height = 240,
>>>>>> + .bin_mode = BIN_MODE_DIGITAL_X4,
>>>>>
>>>>> Can the bin mode computed by dividing the crop width by the output width
>>>>> instead of being stored here ?
>>>>
>>>> The sensor also supports analog subsampling on top of digital binning.
>>>> I did not implement it in the driver but I made the choice to
>>>> explicitly set the bin_mode for further development. For example the
>>>> line above could be:
>>>>
>>>> .bin_mode = BIN_MODE_ANALOG_X4
>>>>
>>>> I can compute the factor (X2/X4), but not the analog/digital mode.
>>>> Hope this make sense.
>>>
>>> It thus indeed, but it then leads to the question of how to configure
>>> skipping and binning separately. The CCS driver registers two subdevs to
>>> make this possible. It increases the complexity, and I'm increasingly
>>> thinking that we should extend the V4L2 API to allow configure all the
>>> common parameters of camera sensors with a single subdev.
>>>
>>> Sakari, any opinion ?
>>>
>>>>>> + .crop = {
>>>>>> + .left = 92,
>>>>>> + .top = 72,
>>>>>> + .width = 1280,
>>>>>> + .height = 960,
>>>>>> + },
>>>>>> + },
>>>>>> +};
>>>>>> +
>>>>>> +static const struct vgxy61_mode_info vgx761_mode_data[] = {
>>>>>> + {
>>>>>> + .width = VGX761_WIDTH,
>>>>>> + .height = VGX761_HEIGHT,
>>>>>> + .bin_mode = BIN_MODE_NORMAL,
>>>>>> + .crop = {
>>>>>> + .left = 0,
>>>>>> + .top = 0,
>>>>>> + .width = VGX761_WIDTH,
>>>>>> + .height = VGX761_HEIGHT,
>>>>>> + },
>>>>>> + },
>>>>>> + {
>>>>>> + .width = 1920,
>>>>>> + .height = 1080,
>>>>>> + .bin_mode = BIN_MODE_NORMAL,
>>>>>> + .crop = {
>>>>>> + .left = 12,
>>>>>> + .top = 62,
>>>>>> + .width = 1920,
>>>>>> + .height = 1080,
>>>>>> + },
>>>>>> + },
>>>>>> + {
>>>>>> + .width = 1280,
>>>>>> + .height = 720,
>>>>>> + .bin_mode = BIN_MODE_NORMAL,
>>>>>> + .crop = {
>>>>>> + .left = 332,
>>>>>> + .top = 242,
>>>>>> + .width = 1280,
>>>>>> + .height = 720,
>>>>>> + },
>>>>>> + },
>>>>>> + {
>>>>>> + .width = 640,
>>>>>> + .height = 480,
>>>>>> + .bin_mode = BIN_MODE_DIGITAL_X2,
>>>>>> + .crop = {
>>>>>> + .left = 332,
>>>>>> + .top = 122,
>>>>>> + .width = 1280,
>>>>>> + .height = 960,
>>>>>> + },
>>>>>> + },
>>>>>> + {
>>>>>> + .width = 320,
>>>>>> + .height = 240,
>>>>>> + .bin_mode = BIN_MODE_DIGITAL_X4,
>>>>>> + .crop = {
>>>>>> + .left = 332,
>>>>>> + .top = 122,
>>>>>> + .width = 1280,
>>>>>> + .height = 960,
>>>>>> + },
>>>>>> + },
>>>>>> +};
>>>>>> +
>>>>>> +struct vgxy61_dev {
>>>>>> + struct i2c_client *i2c_client;
>>>>>> + struct v4l2_subdev sd;
>>>>>> + struct media_pad pad;
>>>>>> + struct regulator_bulk_data supplies[VGXY61_NUM_SUPPLIES];
>>>>>> + struct gpio_desc *reset_gpio;
>>>>>> + struct clk *xclk;
>>>>>> + u32 clk_freq;
>>>>>> + int id;
>>>>>> + int sensor_width;
>>>>>> + int sensor_height;
>>>>>> + u16 oif_ctrl;
>>>>>> + int nb_of_lane;
>>>>>> + int data_rate_in_mbps;
>>>>>> + int pclk;
>>>>>> + u16 line_length;
>>>>>> + int rot_term;
>>>>>> + bool gpios_polarity;
>>>>>> + bool slave_mode;
>>>>>> + /* Lock to protect all members below */
>>>>>> + struct mutex lock;
>>>>>> + struct v4l2_ctrl_handler ctrl_handler;
>>>>>> + struct v4l2_ctrl *pixel_rate_ctrl;
>>>>>> + struct v4l2_ctrl *expo_ctrl;
>>>>>> + struct v4l2_ctrl *vblank_ctrl;
>>>>>> + struct v4l2_ctrl *vflip_ctrl;
>>>>>> + struct v4l2_ctrl *hflip_ctrl;
>>>>>> + bool streaming;
>>>>>> + struct v4l2_mbus_framefmt fmt;
>>>>>> + const struct vgxy61_mode_info *sensor_modes;
>>>>>> + int sensor_modes_nb;
>>>>>> + const struct vgxy61_mode_info *current_mode;
>>>>>> + struct v4l2_fract frame_interval;
>>>>>
>>>>> This is not used. Please check all other fields too.
>>>>>
>>>>>> + bool hflip;
>>>>>> + bool vflip;
>>>>>> + enum hdr hdr;
>>>>>> + int expo_long;
>>>>>> + int expo_short;
>>>>>> + int expo_max;
>>>>>> + int expo_min;
>>>>>> + int vblank;
>>>>>> + int vblank_min;
>>>>>> + u16 frame_length;
>>>>>> +};
>>>>>> +
>>>>>> +static u8 get_bpp_by_code(__u32 code)
>>>>>> +{
>>>>>> + switch (code) {
>>>>>> + case MEDIA_BUS_FMT_Y8_1X8:
>>>>>> + return 8;
>>>>>> + case MEDIA_BUS_FMT_Y10_1X10:
>>>>>> + return 10;
>>>>>> + case MEDIA_BUS_FMT_Y12_1X12:
>>>>>> + return 12;
>>>>>> + case MEDIA_BUS_FMT_Y14_1X14:
>>>>>> + return 14;
>>>>>> + case MEDIA_BUS_FMT_Y16_1X16:
>>>>>> + return 16;
>>>>>> + default:
>>>>>> + /* Should never happen */
>>>>>> + WARN(1, "Unsupported code %d. default to 8 bpp", code);
>>>>>> + return 8;
>>>>>> + }
>>>>>> +}
>>>>>> +
>>>>>> +static u8 get_data_type_by_code(__u32 code)
>>>>>> +{
>>>>>> + switch (code) {
>>>>>> + case MEDIA_BUS_FMT_Y8_1X8:
>>>>>> + return MIPI_CSI2_DT_RAW8;
>>>>>> + case MEDIA_BUS_FMT_Y10_1X10:
>>>>>> + return MIPI_CSI2_DT_RAW10;
>>>>>> + case MEDIA_BUS_FMT_Y12_1X12:
>>>>>> + return MIPI_CSI2_DT_RAW12;
>>>>>> + case MEDIA_BUS_FMT_Y14_1X14:
>>>>>> + return MIPI_CSI2_DT_RAW14;
>>>>>> + case MEDIA_BUS_FMT_Y16_1X16:
>>>>>> + return MIPI_CSI2_DT_RAW16;
>>>>>> + default:
>>>>>> + /* Should never happen */
>>>>>> + WARN(1, "Unsupported code %d. default to MIPI_CSI2_DT_RAW8 data type", code);
>>>>>> + return MIPI_CSI2_DT_RAW8;
>>>>>> + }
>>>>>> +}
>>>>>
>>>>> I'd put all this data in a format description structure, and store that
>>>>> in the vgxy61_supported_codes array.
>>>>>
>>>>>> +
>>>>>> +static void compute_pll_parameters_by_freq(u32 freq, unsigned int *prediv, unsigned int *mult)
>>>>>> +{
>>>>>> + const unsigned int predivs[] = {1, 2, 4};
>>>>>> + int i;
>>>>>> +
>>>>>> + /*
>>>>>> + * Freq range is [6Mhz-27Mhz] already checked.
>>>>>> + * Output of divider should be in [6Mhz-12Mhz[.
>>>>>> + */
>>>>>> + for (i = 0; i < ARRAY_SIZE(predivs); i++) {
>>>>>> + *prediv = predivs[i];
>>>>>> + if (freq / *prediv < 12 * HZ_PER_MHZ)
>>>>>> + break;
>>>>>> + }
>>>>>> + WARN_ON(i == ARRAY_SIZE(predivs));
>>>>>> +
>>>>>> + /*
>>>>>> + * Target freq is 804Mhz. Don't change this as it will impact image quality.
>>>>>> + */
>>>>>> + *mult = ((804 * HZ_PER_MHZ) * (*prediv) + freq / 2) / freq;
>>>>>> +}
>>>>>> +
>>>>>> +static s32 get_pixel_rate(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + return div64_u64((u64)sensor->data_rate_in_mbps * sensor->nb_of_lane,
>>>>>> + get_bpp_by_code(sensor->fmt.code));
>>>>>> +}
>>>>>> +
>>>>>> +static inline struct vgxy61_dev *to_vgxy61_dev(struct v4l2_subdev *sd)
>>>>>> +{
>>>>>> + return container_of(sd, struct vgxy61_dev, sd);
>>>>>> +}
>>>>>> +
>>>>>> +static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
>>>>>> +{
>>>>>> + return &container_of(ctrl->handler, struct vgxy61_dev, ctrl_handler)->sd;
>>>>>> +}
>>>>>> +
>>>>>> +static int get_chunk_size(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + struct i2c_adapter *adapter = sensor->i2c_client->adapter;
>>>>>> + int max_write_len = VGXY61_WRITE_MULTIPLE_CHUNK_MAX;
>>>>>> +
>>>>>> + if (adapter->quirks && adapter->quirks->max_write_len)
>>>>>> + max_write_len = adapter->quirks->max_write_len - 2;
>>>>>> +
>>>>>> + max_write_len = min(max_write_len, VGXY61_WRITE_MULTIPLE_CHUNK_MAX);
>>>>>> +
>>>>>> + return max(max_write_len, 1);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_read_multiple(struct vgxy61_dev *sensor, u16 reg, u8 *val, int len)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + struct i2c_msg msg[2];
>>>>>> + u8 buf[2];
>>>>>> + int ret;
>>>>>> +
>>>>>> + buf[0] = reg >> 8;
>>>>>> + buf[1] = reg & 0xff;
>>>>>> +
>>>>>> + msg[0].addr = client->addr;
>>>>>> + msg[0].flags = client->flags;
>>>>>> + msg[0].buf = buf;
>>>>>> + msg[0].len = sizeof(buf);
>>>>>> +
>>>>>> + msg[1].addr = client->addr;
>>>>>> + msg[1].flags = client->flags | I2C_M_RD;
>>>>>> + msg[1].buf = val;
>>>>>> + msg[1].len = len;
>>>>>> +
>>>>>> + ret = i2c_transfer(client->adapter, msg, 2);
>>>>>> + if (ret < 0) {
>>>>>> + dev_dbg(&client->dev, "%s: %x i2c_transfer, reg: %x => %d\n", __func__,
>>>>>> + client->addr, reg, ret);
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static inline int vgxy61_read_reg(struct vgxy61_dev *sensor, u16 reg, u8 *val)
>>>>>> +{
>>>>>> + return vgxy61_read_multiple(sensor, reg, val, sizeof(*val));
>>>>>> +}
>>>>>> +
>>>>>> +static inline int vgxy61_read_reg16(struct vgxy61_dev *sensor, u16 reg, u16 *val)
>>>>>> +{
>>>>>> + return vgxy61_read_multiple(sensor, reg, (u8 *)val, sizeof(*val));
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_write_multiple(struct vgxy61_dev *sensor, u16 reg, const u8 *data, int len)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + struct i2c_msg msg;
>>>>>> + u8 buf[VGXY61_WRITE_MULTIPLE_CHUNK_MAX + 2];
>>>>>> + int i;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (len > VGXY61_WRITE_MULTIPLE_CHUNK_MAX)
>>>>>> + return -EINVAL;
>>>>>> + buf[0] = reg >> 8;
>>>>>> + buf[1] = reg & 0xff;
>>>>>> + for (i = 0; i < len; i++)
>>>>>> + buf[i + 2] = data[i];
>>>>>> +
>>>>>> + msg.addr = client->addr;
>>>>>> + msg.flags = client->flags;
>>>>>> + msg.buf = buf;
>>>>>> + msg.len = len + 2;
>>>>>> +
>>>>>> + ret = i2c_transfer(client->adapter, &msg, 1);
>>>>>> + if (ret < 0) {
>>>>>> + dev_dbg(&client->dev, "%s: i2c_transfer, reg: %x => %d\n", __func__, reg, ret);
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_write_array(struct vgxy61_dev *sensor, u16 reg, int nb, const u8 *array)
>>>>>> +{
>>>>>> + const int chunk_size = get_chunk_size(sensor);
>>>>>> + int ret;
>>>>>> + int sz;
>>>>>> +
>>>>>> + while (nb) {
>>>>>> + sz = min(nb, chunk_size);
>>>>>> + ret = vgxy61_write_multiple(sensor, reg, array, sz);
>>>>>> + if (ret < 0)
>>>>>> + return ret;
>>>>>> + nb -= sz;
>>>>>> + reg += sz;
>>>>>> + array += sz;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static inline int vgxy61_write_reg(struct vgxy61_dev *sensor, u16 reg, u8 val)
>>>>>> +{
>>>>>> + return vgxy61_write_multiple(sensor, reg, &val, sizeof(val));
>>>>>> +}
>>>>>> +
>>>>>> +static inline int vgxy61_write_reg16(struct vgxy61_dev *sensor, u16 reg, u16 val)
>>>>>> +{
>>>>>> + return vgxy61_write_multiple(sensor, reg, (u8 *)&val, sizeof(val));
>>>>>> +}
>>>>>> +
>>>>>> +static inline int vgxy61_write_reg32(struct vgxy61_dev *sensor, u16 reg, u32 val)
>>>>>> +{
>>>>>> + return vgxy61_write_multiple(sensor, reg, (u8 *)&val, sizeof(val));
>>>>>> +}
>>>>>
>>>>> You could possibly simplify handling of registers of different sizes by
>>>>> encoding the size in the register address macros. See
>>>>> https://gitlab.com/ideasonboard/nxp/linux/-/blob/pinchartl/v5.18/sensors/imx/drivers/media/i2c/imx296.c
>>>>> for an example.
>>>>
>>>> That sounds good, thank you. I will look into this.
>>>
>>> On the topic of register writes, you may also want to consider the *err
>>> argument to imx296_write(), it simplifies error handling (see how errors
>>> are handled in imx296_setup(), compared to checking the return value of
>>> every single write call).
>>>
>>>>>> +
>>>>>> +static int vgxy61_poll_reg(struct vgxy61_dev *sensor, u16 reg, u8 poll_val, int timeout_ms)
>>>>>> +{
>>>>>> + const int loop_delay_ms = 10;
>>>>>> + u8 val;
>>>>>> + int ret, timeout;
>>>>>> +
>>>>>> + timeout = read_poll_timeout(vgxy61_read_reg, ret, ((ret != 0) || (val == poll_val)),
>>>>>> + loop_delay_ms * US_PER_MS, timeout_ms * US_PER_MS, false,
>>>>>> + sensor, reg, &val);
>>>>>> + if (timeout)
>>>>>> + return timeout;
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_wait_state(struct vgxy61_dev *sensor, int state, int timeout_ms)
>>>>>> +{
>>>>>> + return vgxy61_poll_reg(sensor, DEVICE_SYSTEM_FSM, state, timeout_ms);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_check_bw(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + /* Correction factor for time required between 2 lines */
>>>>>> + const int mipi_margin = 1056;
>>>>>> + int binning_scale = sensor->current_mode->crop.height / sensor->current_mode->height;
>>>>>> + int bpp = get_bpp_by_code(sensor->fmt.code);
>>>>>> + int max_bit_per_line;
>>>>>> + int bit_per_line;
>>>>>> + u64 line_rate;
>>>>>> +
>>>>>> + line_rate = sensor->nb_of_lane * (u64)sensor->data_rate_in_mbps * sensor->line_length;
>>>>>> + max_bit_per_line = div64_u64(line_rate, sensor->pclk) - mipi_margin;
>>>>>> + bit_per_line = (bpp * sensor->current_mode->width) / binning_scale;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "max_bit_per_line = %d\n", max_bit_per_line);
>>>>>> + dev_dbg(&client->dev, "required bit_per_line = %d\n", bit_per_line);
>>>>>> +
>>>>>> + return bit_per_line > max_bit_per_line ? -EINVAL : 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int apply_exposure(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + int ret;
>>>>>> +
>>>>>> + /* We first set expo to zero to avoid forbidden parameters couple */
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_COARSE_EXPOSURE_SHORT, 0);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_COARSE_EXPOSURE_LONG, sensor->expo_long);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_COARSE_EXPOSURE_SHORT, sensor->expo_short);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>
>>>>> Is there a risk those writes may be applied to different frames ?
>>>>
>>>> Yes, but it does not matter for the sensor. Are you thinking of an
>>>> issue about user space applications for instance?
>>>
>>> Yes, if the writes cross frame boundaries, we could have a frame with a
>>> weird exposure time, which could confuse ISP algorithms.
>>>
>>> Does the sensor support CSI-2 embedded data ?
>>
>> It does, and embed the exposures in it. So I guess the ISP algorithm
>> could check these and react accordingly.
>
> For ISPs that can capture embedded data, that sounds good.
>
>> As a side note the short exposure is only used for HDR modes, so as
>> long as the user is running in SDR there is no impact at all.
>
> That can help.
>
> I suppose we can improve this further later if/when needed. I'm just
> worried it could lead to issues that are difficult to debug.
>
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "%s applied expo %d (short: %d)\n", __func__,
>>>>>> + sensor->expo_long, sensor->expo_short);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int set_frame_rate(struct vgxy61_dev *sensor)
>>>>>
>>>>> The function sets the frame length, not the frame rate.
>>>>>
>>>>>> +{
>>>>>> + return vgxy61_write_reg16(sensor, DEVICE_FRAME_LENGTH, sensor->frame_length);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_get_regulators(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + int i;
>>>>>> +
>>>>>> + for (i = 0; i < VGXY61_NUM_SUPPLIES; i++)
>>>>>> + sensor->supplies[i].supply = vgxy61_supply_name[i];
>>>>>> +
>>>>>> + return devm_regulator_bulk_get(&sensor->i2c_client->dev, VGXY61_NUM_SUPPLIES,
>>>>>> + sensor->supplies);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_apply_reset(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "%s applied reset\n", __func__);
>>>>>> + gpiod_set_value_cansleep(sensor->reset_gpio, 0);
>>>>>> + usleep_range(5000, 10000);
>>>>>> + gpiod_set_value_cansleep(sensor->reset_gpio, 1);
>>>>>> + usleep_range(5000, 10000);
>>>>>> + gpiod_set_value_cansleep(sensor->reset_gpio, 0);
>>>>>> + usleep_range(40000, 100000);
>>>>>> + return vgxy61_wait_state(sensor, SW_STBY, VGXY61_TIMEOUT_MS);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_try_fmt_internal(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *fmt,
>>>>>> + const struct vgxy61_mode_info **new_mode)
>>>>>> +{
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> + const struct vgxy61_mode_info *mode = sensor->sensor_modes;
>>>>>> + unsigned int index;
>>>>>> +
>>>>>> + /* Select code */
>>>>>> + for (index = 0; index < ARRAY_SIZE(vgxy61_supported_codes); index++) {
>>>>>> + if (vgxy61_supported_codes[index] == fmt->code)
>>>>>> + break;
>>>>>> + }
>>>>>> + if (index == ARRAY_SIZE(vgxy61_supported_codes))
>>>>>> + index = 0;
>>>>>> +
>>>>>> + /* Select size */
>>>>>> + mode = v4l2_find_nearest_size(sensor->sensor_modes, sensor->sensor_modes_nb,
>>>>>> + width, height, fmt->width, fmt->height);
>>>>>> + if (new_mode)
>>>>>> + *new_mode = mode;
>>>>>> +
>>>>>> + fmt->code = vgxy61_supported_codes[index];
>>>>>> + fmt->width = mode->width;
>>>>>> + fmt->height = mode->height;
>>>>>> + fmt->colorspace = V4L2_COLORSPACE_RAW;
>>>>>> + fmt->field = V4L2_FIELD_NONE;
>>>>>> + fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>>>>> + fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
>>>>>> + fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_get_selection(struct v4l2_subdev *sd,
>>>>>> + struct v4l2_subdev_state *sd_state,
>>>>>> + struct v4l2_subdev_selection *sel)
>>>>>> +{
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> +
>>>>>> + switch (sel->target) {
>>>>>> + case V4L2_SEL_TGT_CROP:
>>>>>> + sel->r = sensor->current_mode->crop;
>>>>>> + return 0;
>>>>>> + case V4L2_SEL_TGT_NATIVE_SIZE:
>>>>>> + case V4L2_SEL_TGT_CROP_DEFAULT:
>>>>>> + case V4L2_SEL_TGT_CROP_BOUNDS:
>>>>>> + sel->r.top = 0;
>>>>>> + sel->r.left = 0;
>>>>>> + sel->r.width = sensor->sensor_width;
>>>>>> + sel->r.height = sensor->sensor_height;
>>>>>> + return 0;
>>>>>> + }
>>>>>> +
>>>>>> + return -EINVAL;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_stream_enable(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>>>>>> + const struct v4l2_rect *crop = &sensor->current_mode->crop;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = vgxy61_check_bw(sensor);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = pm_runtime_get_sync(&client->dev);
>>>>>> + if (ret < 0) {
>>>>>> + pm_runtime_put_noidle(&client->dev);
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + /* Configure sensor */
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_FORMAT_CTRL, get_bpp_by_code(sensor->fmt.code));
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_OIF_ROI0_CTRL,
>>>>>> + get_data_type_by_code(sensor->fmt.code));
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> +
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_READOUT_CTRL, sensor->current_mode->bin_mode);
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_ROI0_START_H, crop->left);
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_ROI0_END_H, crop->left + crop->width - 1);
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_ROI0_START_V, crop->top);
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_ROI0_END_V, crop->top + crop->height - 1);
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> +
>>>>>> + ret = set_frame_rate(sensor);
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> +
>>>>>> + ret = apply_exposure(sensor);
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> +
>>>>>> + /* Start streaming */
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_STREAMING, REQ_START_STREAMING);
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> +
>>>>>> + ret = vgxy61_poll_reg(sensor, DEVICE_STREAMING, REQ_NO_REQUEST, VGXY61_TIMEOUT_MS);
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> +
>>>>>> + ret = vgxy61_wait_state(sensor, STREAMING, VGXY61_TIMEOUT_MS);
>>>>>> + if (ret)
>>>>>> + goto err_rpm_put;
>>>>>> +
>>>>>> + /* vflip and hflip cannot change during streaming */
>>>>>> + __v4l2_ctrl_grab(sensor->vflip_ctrl, true);
>>>>>> + __v4l2_ctrl_grab(sensor->hflip_ctrl, true);
>>>>>> +
>>>>>> + return 0;
>>>>>> +
>>>>>> +err_rpm_put:
>>>>>> + pm_runtime_put(&client->dev);
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_stream_disable(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_STREAMING, REQ_STOP_STREAMING);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = vgxy61_poll_reg(sensor, DEVICE_STREAMING, REQ_NO_REQUEST, 2000);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = vgxy61_wait_state(sensor, SW_STBY, VGXY61_TIMEOUT_MS);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + __v4l2_ctrl_grab(sensor->vflip_ctrl, true);
>>>>>> + __v4l2_ctrl_grab(sensor->hflip_ctrl, true);
>>>>>> +
>>>>>> + pm_runtime_put(&client->dev);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_s_stream(struct v4l2_subdev *sd, int enable)
>>>>>> +{
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + int ret = 0;
>>>>>> +
>>>>>> + mutex_lock(&sensor->lock);
>>>>>> + dev_dbg(&client->dev, "%s : requested %d / current = %d\n", __func__, enable,
>>>>>> + sensor->streaming);
>>>>>> + if (sensor->streaming == enable)
>>>>>> + goto out;
>>>>>
>>>>> This should never happen. If you want to be on the safe side and keep
>>>>> the check, I'd turn it into a WARN_ON().
>>>>
>>>> You are right, I will remove this.
>>>>
>>>>>> +
>>>>>> + ret = enable ? vgxy61_stream_enable(sensor) : vgxy61_stream_disable(sensor);
>>>>>> + if (!ret)
>>>>>> + sensor->streaming = enable;
>>>>>> +
>>>>>> +out:
>>>>>> + dev_dbg(&client->dev, "%s current now = %d / %d\n", __func__, sensor->streaming, ret);
>>>>>> + mutex_unlock(&sensor->lock);
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_enum_mbus_code(struct v4l2_subdev *sd,
>>>>>> + struct v4l2_subdev_state *sd_state,
>>>>>> + struct v4l2_subdev_mbus_code_enum *code)
>>>>>> +{
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "%s probe index %d\n", __func__, code->index);
>>>>>> + if (code->index >= ARRAY_SIZE(vgxy61_supported_codes))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + code->code = vgxy61_supported_codes[code->index];
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_get_fmt(struct v4l2_subdev *sd,
>>>>>> + struct v4l2_subdev_state *sd_state,
>>>>>> + struct v4l2_subdev_format *format)
>>>>>> +{
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + struct v4l2_mbus_framefmt *fmt;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "%s probe %d\n", __func__, format->pad);
>>>>>> + dev_dbg(&client->dev, "%s %dx%d\n", __func__, format->format.width, format->format.height);
>>>>>> +
>>>>>> + mutex_lock(&sensor->lock);
>>>>>> +
>>>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>>>>>> + fmt = v4l2_subdev_get_try_format(&sensor->sd, sd_state, format->pad);
>>>>>> + else
>>>>>> + fmt = &sensor->fmt;
>>>>>> +
>>>>>> + format->format = *fmt;
>>>>>> +
>>>>>> + mutex_unlock(&sensor->lock);q
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_get_vblank_min(struct vgxy61_dev *sensor, enum hdr hdr)
>>>>>> +{
>>>>>> + int min_vblank = VGXY61_MIN_FRAME_LENGTH - sensor->current_mode->crop.height;
>>>>>> + /* Ensure the first tule of thumb can't be negative */
>>>>>> + int min_vblank_hdr = VGXY61_MIN_EXPOSURE + sensor->rot_term + 1;
>>>>>> +
>>>>>> + if (hdr != NO_HDR)
>>>>>> + return max(min_vblank, min_vblank_hdr);
>>>>>> + return min_vblank;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_enum_frame_size(struct v4l2_subdev *sd, struct v4l2_subdev_state *sd_state,
>>>>>> + struct v4l2_subdev_frame_size_enum *fse)
>>>>>> +{
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "%s for index %d\n", __func__, fse->index);
>>>>>> + if (fse->index >= sensor->sensor_modes_nb)
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + fse->min_width = sensor->sensor_modes[fse->index].width;
>>>>>> + fse->max_width = fse->min_width;
>>>>>> + fse->min_height = sensor->sensor_modes[fse->index].height;
>>>>>> + fse->max_height = fse->min_height;
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_update_analog_gain(struct vgxy61_dev *sensor, u32 target)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + int ret;
>>>>>> +
>>>>>> + /* Apply gain */
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_ANALOG_GAIN, target);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "Set analog gain: 0x%04x\n", target);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_update_digital_gain(struct vgxy61_dev *sensor, u32 target)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + int ret;
>>>>>> +
>>>>>> + /*
>>>>>> + * For a monochrome version, configuring DIGITAL_GAIN_LONG_CH0 and
>>>>>> + * DIGITAL_GAIN_SHORT_CH0 is enough to configure the gain of all
>>>>>> + * four sub pixels.
>>>>>> + */
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_DIGITAL_GAIN_LONG, target);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_DIGITAL_GAIN_SHORT, target);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "Set digital gain: 0x%04x\n", target);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_update_patgen(struct vgxy61_dev *sensor, u32 index)
>>>>>> +{
>>>>>> + u32 pattern = index <= 3 ? index : index + 12;
>>>>>> + u32 reg;
>>>>>> +
>>>>>> + reg = (pattern << 18) | (pattern << 4);
>>>>>> + if (index)
>>>>>> + reg |= (1 << 16) | 1;
>>>>>
>>>>> Macros would be nice instead of magic numbers. Same where applicable
>>>>> below.
>>>>>
>>>>>> + return vgxy61_write_reg32(sensor, DEVICE_PATGEN_CTRL, reg);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_update_gpiox_strobe_mode(struct vgxy61_dev *sensor, enum strobe_modes mode,
>>>>>> + int idx)
>>>>>> +{
>>>>>> + const u8 index2val[] = {0x0, 0x1, 0x3};
>>>>>
>>>>> Make this static const.
>>>>>
>>>>>> + u16 reg;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = vgxy61_read_reg16(sensor, DEVICE_SIGNALS_CTRL, ®);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + reg &= ~(0xf << (idx * 4));
>>>>>> + reg |= index2val[mode] << (idx * 4);
>>>>>> +
>>>>>> + return vgxy61_write_reg16(sensor, DEVICE_SIGNALS_CTRL, reg);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_update_gpios_strobe_mode(struct vgxy61_dev *sensor, enum hdr hdr)
>>>>>> +{
>>>>>> + enum strobe_modes strobe_mode;
>>>>>> + int i, ret;
>>>>>> +
>>>>>> + switch (hdr) {
>>>>>> + case HDR_LINEAR:
>>>>>> + strobe_mode = STROBE_ENABLED;
>>>>>> + break;
>>>>>> + case HDR_SUB:
>>>>>> + case NO_HDR:
>>>>>> + strobe_mode = STROBE_LONG;
>>>>>> + break;
>>>>>> + default:
>>>>>> + /* Should never happen */
>>>>>> + WARN_ON(true);
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> + for (i = 0; i < VGXY61_NB_GPIOS; i++) {
>>>>>> + ret = vgxy61_update_gpiox_strobe_mode(sensor, strobe_mode, i);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_update_gpios_strobe_polarity(struct vgxy61_dev *sensor, int polarity)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (sensor->streaming)
>>>>>> + return -EBUSY;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "setting gpios polarity: %d\n", polarity);
>>>>>> +
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_GPIO_0_CTRL, polarity << 1);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_GPIO_1_CTRL, polarity << 1);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_GPIO_2_CTRL, polarity << 1);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_GPIO_3_CTRL, polarity << 1);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + return vgxy61_write_reg(sensor, DEVICE_SIGNALS_POLARITY_CTRL, polarity);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_get_expo_long_max(struct vgxy61_dev *sensor, int short_expo_ratio)
>>>>>> +{
>>>>>> + int first_rot_max_expo, second_rot_max_expo, third_rot_max_expo;
>>>>>> +
>>>>>> + /* Apply sensor rule of thumbs */
>>>>>> + /* Avoid bad pixel line at the botom of the image */
>>>>>> + first_rot_max_expo =
>>>>>> + ((sensor->frame_length - sensor->current_mode->crop.height - sensor->rot_term) *
>>>>>> + short_expo_ratio) - 1;
>>>>>> +
>>>>>> + /* Avoid sensor crash */
>>>>>
>>>>> How so ?
>>>>
>>>> This specific rule guarantees the framelength < expo_time. If this is
>>>> not the case then the sensor will crash.
>>>>
>>>> I will add a bit more comment for this, and for other rules also.
>>>>
>>>>>> + second_rot_max_expo =
>>>>>> + (((sensor->frame_length - VGXY61_EXPOS_ROT_TERM) * short_expo_ratio) /
>>>>>> + (short_expo_ratio + 1)) - 1;
>>>>>> +
>>>>>> + /* Avoid sensor crash */
>>>>>> + third_rot_max_expo = (sensor->frame_length / 71) * short_expo_ratio;
>>>>>> +
>>>>>> + /* Take the minimum from all rules */
>>>>>> + return min(min(first_rot_max_expo, second_rot_max_expo), third_rot_max_expo);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_update_exposure(struct vgxy61_dev *sensor, int new_expo_long, enum hdr hdr)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + int new_expo_short = 0;
>>>>>> + int expo_short_max = 0;
>>>>>> + int expo_long_min = VGXY61_MIN_EXPOSURE;
>>>>>> + int expo_long_max;
>>>>>> +
>>>>>> + /* Compute short exposure according to hdr mode and long exposure */
>>>>>> + switch (hdr) {
>>>>>> + case HDR_LINEAR:
>>>>>> + /* Take ratio into account for minimal exposures in HDR_LINEAR */
>>>>>> + expo_long_min = VGXY61_MIN_EXPOSURE * VGXY61_HDR_LINEAR_RATIO;
>>>>>> + new_expo_long = max(expo_long_min, new_expo_long);
>>>>>> +
>>>>>> + expo_long_max = vgxy61_get_expo_long_max(sensor, VGXY61_HDR_LINEAR_RATIO);
>>>>>> + expo_short_max =
>>>>>> + (expo_long_max + (VGXY61_HDR_LINEAR_RATIO / 2)) / VGXY61_HDR_LINEAR_RATIO;
>>>>>> + new_expo_short =
>>>>>> + (new_expo_long + (VGXY61_HDR_LINEAR_RATIO / 2)) / VGXY61_HDR_LINEAR_RATIO;
>>>>>> + break;
>>>>>> + case HDR_SUB:
>>>>>> + new_expo_long = max(expo_long_min, new_expo_long);
>>>>>> +
>>>>>> + expo_long_max = vgxy61_get_expo_long_max(sensor, 1);
>>>>>> + /* Short and long are the same in HDR_SUB */
>>>>>> + expo_short_max = expo_long_max;
>>>>>> + new_expo_short = new_expo_long;
>>>>>> + break;
>>>>>> + case NO_HDR:
>>>>>> + new_expo_long = max(expo_long_min, new_expo_long);
>>>>>> +
>>>>>> + /*
>>>>>> + * As short expo is 0 here, only the second rule of thumb applies, see
>>>>>> + * vgxy61_get_expo_long_max for more
>>>>>> + */
>>>>>> + expo_long_max = sensor->frame_length - VGXY61_EXPOS_ROT_TERM;
>>>>>> + break;
>>>>>> + default:
>>>>>> + /* Should never happen */
>>>>>> + WARN_ON(true);
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> + /* If this happens, something is wrong with formulas */
>>>>>> + WARN_ON(expo_long_min > expo_long_max);
>>>>>> +
>>>>>> + if (new_expo_long > expo_long_max) {
>>>>>> + dev_warn(&client->dev, "Exposure %d too high, clamping to %d\n",
>>>>>> + new_expo_long, expo_long_max);
>>>>>> + new_expo_long = expo_long_max;
>>>>>> + new_expo_short = expo_short_max;
>>>>>> + }
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "frame_length %d, new_expo_long %d, new_expo_short %d\n",
>>>>>> + sensor->frame_length, new_expo_long, new_expo_short);
>>>>>> + dev_dbg(&client->dev, "expo_long_min %d, expo_long_max %d\n",
>>>>>> + expo_long_min, expo_long_max);
>>>>>> +
>>>>>> + /* Apply exposure */
>>>>>> + sensor->expo_long = new_expo_long;
>>>>>> + sensor->expo_short = new_expo_short;
>>>>>> + sensor->expo_max = expo_long_max;
>>>>>> + sensor->expo_min = expo_long_min;
>>>>>> +
>>>>>> + if (sensor->streaming)
>>>>>> + return apply_exposure(sensor);
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_update_vblank(struct vgxy61_dev *sensor, int vblank, int hdr)
>>>>>> +{
>>>>>> + int ret;
>>>>>> +
>>>>>> + sensor->vblank_min = vgxy61_get_vblank_min(sensor, hdr);
>>>>>> + sensor->vblank = max(sensor->vblank_min, vblank);
>>>>>> + sensor->frame_length = sensor->current_mode->crop.height + sensor->vblank;
>>>>>> +
>>>>>> + /* Update exposure according to vblank */
>>>>>> + ret = vgxy61_update_exposure(sensor, sensor->expo_long, hdr);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + if (sensor->streaming)
>>>>>> + return set_frame_rate(sensor);
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_update_hdr(struct vgxy61_dev *sensor, u32 index)
>>>>>> +{
>>>>>> + const u8 index2val[] = {0x1, 0x4, 0xa};
>>>>>> + int ret;
>>>>>> +
>>>>>> + /*
>>>>>> + * vblank and short exposure change according to HDR mode, do it first as it can
>>>>>> + * violate sensors 'rule of thumbs' and therefore will require to change the long exposure
>>>>>> + */
>>>>>> + ret = vgxy61_update_vblank(sensor, sensor->vblank, index);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + /* Update strobe mode according to HDR */
>>>>>> + ret = vgxy61_update_gpios_strobe_mode(sensor, index);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_HDR_CTRL, index2val[index]);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + sensor->hdr = index;
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_set_fmt(struct v4l2_subdev *sd,
>>>>>> + struct v4l2_subdev_state *sd_state,
>>>>>> + struct v4l2_subdev_format *format)
>>>>>> +{
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + const struct vgxy61_mode_info *new_mode;
>>>>>> + struct v4l2_mbus_framefmt *fmt;
>>>>>> + int ret;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "%s probe %d\n", __func__, format->pad);
>>>>>> + dev_dbg(&client->dev, "%s %dx%d\n", __func__, format->format.width, format->format.height);
>>>>>> +
>>>>>> + mutex_lock(&sensor->lock);
>>>>>> +
>>>>>> + if (sensor->streaming) {
>>>>>> + ret = -EBUSY;
>>>>>> + goto out;
>>>>>> + }
>>>>>> +
>>>>>> + /* Find best format */
>>>>>> + ret = vgxy61_try_fmt_internal(sd, &format->format, &new_mode);
>>>>>> + if (ret)
>>>>>> + goto out;
>>>>>> +
>>>>>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>>>>>> + fmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
>>>>>> + *fmt = format->format;
>>>>>> + } else if (sensor->current_mode != new_mode || sensor->fmt.code != format->format.code) {
>>>>>> + fmt = &sensor->fmt;
>>>>>> + *fmt = format->format;
>>>>>> +
>>>>>> + sensor->current_mode = new_mode;
>>>>>> +
>>>>>> + /* Reset vblank and framelength to default */
>>>>>> + ret = vgxy61_update_vblank(sensor,
>>>>>> + VGXY61_FRAME_LENGTH_DEF -
>>>>>> + sensor->current_mode->crop.height,
>>>>>> + sensor->hdr);
>>>>>> +
>>>>>> + /* Update controls to reflect new mode */
>>>>>> + __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate_ctrl, get_pixel_rate(sensor));
>>>>>> + __v4l2_ctrl_modify_range(sensor->vblank_ctrl, sensor->vblank_min,
>>>>>> + 0xffff - sensor->current_mode->crop.height, 1,
>>>>>> + sensor->vblank);
>>>>>> + __v4l2_ctrl_s_ctrl(sensor->vblank_ctrl, sensor->vblank);
>>>>>> + __v4l2_ctrl_modify_range(sensor->expo_ctrl, sensor->expo_min,
>>>>>> + sensor->expo_max, 1,
>>>>>> + sensor->expo_long);
>>>>>> + }
>>>>>> +
>>>>>> +out:
>>>>>> + mutex_unlock(&sensor->lock);
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_get_temp_stream_enabled(struct vgxy61_dev *sensor, int *temp)
>>>>>> +{
>>>>>> + int ret;
>>>>>> + u16 temperature;
>>>>>> +
>>>>>> + ret = vgxy61_read_reg16(sensor, DEVICE_THSENS1_TEMPERATURE, &temperature);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + /* Temperature is expressed in Kelvin in Q10.2 fixed point format*/
>>>>>> + temperature = (temperature & 0x0fff) >> 2;
>>>>>> + temperature = kelvin_to_celsius(temperature);
>>>>>> +
>>>>>> + *temp = temperature;
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_get_temp_stream_disabled(struct vgxy61_dev *sensor, int *temp)
>>>>>> +{
>>>>>> + int ret;
>>>>>> +
>>>>>> + /* Device needs to be in standby mode if streaming is off */
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_STBY, STBY_REQ_TMP_READ);
>>>>>
>>>>> Can't runtime PM be disabled here ?
>>>>
>>>> You're absolutely right. This works as of now because I guess my
>>>> regulators are used for something else and therefore do not go down,
>>>> but should not.
>>>>
>>>> This is tricky as I don't want to power up (and therefore patch) the
>>>> sensor every time I want to see the temperature, but maybe I won't
>>>> have the choice.
>>>>
>>>> For example every time I stop streaming in qv4l2, the temperature is read. This would result in this:
>>>> power off -> get_temp -> power on (+patch) -> read temp -> power off
>>>> Maybe I can just remove the temperature reading while off? Any other idea?
>>>
>>> I think returning an error when the sensor is off is fine. The main use
>>> cases for the temperature sensor are related to imaging.
>>>
>>>> Anyway this will depend on the 2/5 patch discussion thread result.
>>>>
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_poll_reg(sensor, DEVICE_STBY, STBY_NO_REQ, VGXY61_TIMEOUT_MS);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + return vgxy61_get_temp_stream_enabled(sensor, temp);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_get_temp(struct vgxy61_dev *sensor, int *temp)
>>>>>> +{
>>>>>> + *temp = 0;
>>>>>> + if (sensor->streaming)
>>>>>> + return vgxy61_get_temp_stream_enabled(sensor, temp);
>>>>>> + else
>>>>>> + return vgxy61_get_temp_stream_disabled(sensor, temp);
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>>> +{
>>>>>> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> + int ret;
>>>>>> +
>>>>>> + switch (ctrl->id) {
>>>>>> + case V4L2_CID_EXPOSURE:
>>>>>> + ret = vgxy61_update_exposure(sensor, ctrl->val, sensor->hdr);
>>>>>> + ctrl->val = sensor->expo_long;
>>>>>> + break;
>>>>>> + case V4L2_CID_ANALOGUE_GAIN:
>>>>>> + ret = vgxy61_update_analog_gain(sensor, ctrl->val);
>>>>>> + break;
>>>>>> + case V4L2_CID_DIGITAL_GAIN:
>>>>>> + ret = vgxy61_update_digital_gain(sensor, ctrl->val);
>>>>>> + break;
>>>>>> + case V4L2_CID_VFLIP:
>>>>>> + case V4L2_CID_HFLIP:
>>>>>> + if (sensor->streaming) {
>>>>>> + ret = -EBUSY;
>>>>>> + break;
>>>>>> + }
>>>>>> + if (ctrl->id == V4L2_CID_VFLIP)
>>>>>> + sensor->vflip = ctrl->val;
>>>>>> + if (ctrl->id == V4L2_CID_HFLIP)
>>>>>> + sensor->hflip = ctrl->val;
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_ORIENTATION,
>>>>>> + sensor->hflip | (sensor->vflip << 1));
>>>>>> + break;
>>>>>> + case V4L2_CID_TEST_PATTERN:
>>>>>> + ret = vgxy61_update_patgen(sensor, ctrl->val);
>>>>>> + break;
>>>>>> + case V4L2_CID_HDR:
>>>>>> + ret = vgxy61_update_hdr(sensor, ctrl->val);
>>>>>> + /* Update vblank and exposure controls to match new hdr */
>>>>>> + __v4l2_ctrl_modify_range(sensor->vblank_ctrl, sensor->vblank_min,
>>>>>> + 0xffff - sensor->current_mode->crop.height, 1,
>>>>>> + sensor->vblank);
>>>>>> + __v4l2_ctrl_modify_range(sensor->expo_ctrl, sensor->expo_min,
>>>>>> + sensor->expo_max, 1, sensor->expo_long);
>>>>>> + break;
>>>>>> + case V4L2_CID_VBLANK:
>>>>>> + ret = vgxy61_update_vblank(sensor, ctrl->val, sensor->hdr);
>>>>>> + /* Update exposure control to match new vblank */
>>>>>> + __v4l2_ctrl_modify_range(sensor->expo_ctrl, sensor->expo_min,
>>>>>> + sensor->expo_max, 1, sensor->expo_long);
>>>>>> + break;
>>>>>> + default:
>>>>>> + ret = -EINVAL;
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
>>>>>> +{
>>>>>> + struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> + int ret;
>>>>>> +
>>>>>> + switch (ctrl->id) {
>>>>>> + case V4L2_CID_TEMPERATURE:
>>>>>> + ret = vgxy61_get_temp(sensor, &ctrl->val);
>>>>>> + break;
>>>>>> + default:
>>>>>> + ret = -EINVAL;
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct v4l2_ctrl_ops vgxy61_ctrl_ops = {
>>>>>> + .g_volatile_ctrl = vgxy61_g_volatile_ctrl,
>>>>>> + .s_ctrl = vgxy61_s_ctrl,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct v4l2_ctrl_config vgxy61_hdr_ctrl = {
>>>>>> + .ops = &vgxy61_ctrl_ops,
>>>>>> + .id = V4L2_CID_HDR,
>>>>>> + .name = "HDR mode",
>>>>>> + .type = V4L2_CTRL_TYPE_MENU,
>>>>>> + .min = 0,
>>>>>> + .max = ARRAY_SIZE(vgxy61_hdr_modes) - 1,
>>>>>> + .def = NO_HDR,
>>>>>> + .qmenu = vgxy61_hdr_modes,
>>>>>> +};
>>>>>> +
>>>>>> +static int vgxy61_init_controls(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + const struct v4l2_ctrl_ops *ops = &vgxy61_ctrl_ops;
>>>>>> + struct v4l2_ctrl_handler *hdl = &sensor->ctrl_handler;
>>>>>> + struct v4l2_ctrl *ctrl;
>>>>>> + int ret;
>>>>>> +
>>>>>> + v4l2_ctrl_handler_init(hdl, 16);
>>>>>> + mutex_init(&sensor->lock);
>>>>>
>>>>> This should be initialized in probe(), it doesn't belong here.
>>>>>
>>>>>> + /* We can use our own mutex for the ctrl lock */
>>>>>> + hdl->lock = &sensor->lock;
>>>>>> + /* Standard controls */
>>>>>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN, 0, 0x1c, 1, 0x0);
>>>>>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN, 0, 0xfff, 1, 256);
>>>>>> + v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEMPERATURE, -128, 128, 1, 0);
>>>>>> + v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
>>>>>> + ARRAY_SIZE(vgxy61_test_pattern_menu) - 1, 0, 0,
>>>>>> + vgxy61_test_pattern_menu);
>>>>>> + v4l2_ctrl_new_custom(hdl, &vgxy61_hdr_ctrl, NULL);
>>>>>> + ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, 0, sensor->line_length, 1,
>>>>>> + sensor->line_length - sensor->current_mode->width);
>>>>>> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>>>>> + /* Custom controls */
>>>>>> + ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, ARRAY_SIZE(link_freq) - 1, 0,
>>>>>> + link_freq);
>>>>>> + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>>>>> + /* Keep a pointer to these controls as we need to update them when setting the format */
>>>>>> + sensor->pixel_rate_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE, 1, INT_MAX, 1,
>>>>>> + get_pixel_rate(sensor));
>>>>>> + sensor->pixel_rate_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>>>>>> + sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, sensor->expo_min,
>>>>>> + sensor->expo_max, 1, sensor->expo_long);
>>>>>> + sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, sensor->vblank_min,
>>>>>> + 0xffff - sensor->current_mode->crop.height, 1,
>>>>>> + sensor->vblank);
>>>>>> + sensor->vflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, sensor->vflip);
>>>>>> + sensor->hflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, sensor->hflip);
>>>>>> +
>>>>>> + if (hdl->error) {
>>>>>> + ret = hdl->error;
>>>>>> + goto free_ctrls;
>>>>>> + }
>>>>>> +
>>>>>> + sensor->sd.ctrl_handler = hdl;
>>>>>> + return 0;
>>>>>> +
>>>>>> +free_ctrls:
>>>>>> + v4l2_ctrl_handler_free(hdl);
>>>>>> + mutex_destroy(&sensor->lock);
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct v4l2_subdev_core_ops vgxy61_core_ops = {
>>>>>> +};
>>>>>
>>>>> You can drop this.
>>>>>
>>>>>> +
>>>>>> +static const struct v4l2_subdev_video_ops vgxy61_video_ops = {
>>>>>> + .s_stream = vgxy61_s_stream,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct v4l2_subdev_pad_ops vgxy61_pad_ops = {
>>>>>> + .enum_mbus_code = vgxy61_enum_mbus_code,
>>>>>> + .get_fmt = vgxy61_get_fmt,
>>>>>> + .set_fmt = vgxy61_set_fmt,
>>>>>> + .get_selection = vgxy61_get_selection,
>>>>>> + .enum_frame_size = vgxy61_enum_frame_size,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct v4l2_subdev_ops vgxy61_subdev_ops = {
>>>>>> + .core = &vgxy61_core_ops,
>>>>>> + .video = &vgxy61_video_ops,
>>>>>> + .pad = &vgxy61_pad_ops,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct media_entity_operations vgxy61_subdev_entity_ops = {
>>>>>> + .link_validate = v4l2_subdev_link_validate,
>>>>>> +};
>>>>>> +
>>>>>> +/* Set phy polarities */
>>>>>> +static int vgxy61_tx_from_ep(struct vgxy61_dev *sensor, struct fwnode_handle *endpoint)
>>>>>> +{
>>>>>> + struct v4l2_fwnode_endpoint ep = { .bus_type = V4L2_MBUS_CSI2_DPHY };
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + u32 log2phy[VGXY61_NB_POLARITIES] = {~0, ~0, ~0, ~0, ~0};
>>>>>> + u32 phy2log[VGXY61_NB_POLARITIES] = {~0, ~0, ~0, ~0, ~0};
>>>>>> + int polarities[VGXY61_NB_POLARITIES] = {0, 0, 0, 0, 0};
>>>>>> + int l_nb;
>>>>>> + int p, l;
>>>>>> + int ret;
>>>>>> + int i;
>>>>>> +
>>>>>> + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
>>>>>> + if (ret)
>>>>>> + goto error_alloc;
>>>>>> +
>>>>>> + l_nb = ep.bus.mipi_csi2.num_data_lanes;
>>>>>> + if (l_nb != 1 && l_nb != 2 && l_nb != 4) {
>>>>>> + dev_err(&client->dev, "invalid data lane number %d\n", l_nb);
>>>>>> + goto error_ep;
>>>>>> + }
>>>>>> +
>>>>>> + /* Build log2phy, phy2log and polarities from ep info */
>>>>>> + log2phy[0] = ep.bus.mipi_csi2.clock_lane;
>>>>>> + phy2log[log2phy[0]] = 0;
>>>>>> + for (l = 1; l < l_nb + 1; l++) {
>>>>>> + log2phy[l] = ep.bus.mipi_csi2.data_lanes[l - 1];
>>>>>> + phy2log[log2phy[l]] = l;
>>>>>> + }
>>>>>> + /*
>>>>>> + * Then fill remaining slots for every physical slot to have something valid for hardware
>>>>>> + * stuff.
>>>>>> + */
>>>>>> + for (p = 0; p < VGXY61_NB_POLARITIES; p++) {
>>>>>> + if (phy2log[p] != ~0)
>>>>>> + continue;
>>>>>> + phy2log[p] = l;
>>>>>> + log2phy[l] = p;
>>>>>> + l++;
>>>>>> + }
>>>>>> + for (l = 0; l < l_nb + 1; l++)
>>>>>> + polarities[l] = ep.bus.mipi_csi2.lane_polarities[l];
>>>>>> +
>>>>>> + if (log2phy[0] != 0) {
>>>>>> + dev_err(&client->dev, "clk lane must be map to physical lane 0\n");
>>>>>> + goto error_ep;
>>>>>> + }
>>>>>> + sensor->oif_ctrl = (polarities[4] << 15) + ((phy2log[4] - 1) << 13) +
>>>>>> + (polarities[3] << 12) + ((phy2log[3] - 1) << 10) +
>>>>>> + (polarities[2] << 9) + ((phy2log[2] - 1) << 7) +
>>>>>> + (polarities[1] << 6) + ((phy2log[1] - 1) << 4) +
>>>>>> + (polarities[0] << 3) +
>>>>>> + l_nb;
>>>>>> + sensor->nb_of_lane = l_nb;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "tx uses %d lanes", l_nb);
>>>>>> + for (i = 0; i < 5; i++) {
>>>>>> + dev_dbg(&client->dev, "log2phy[%d] = %d\n", i, log2phy[i]);
>>>>>> + dev_dbg(&client->dev, "phy2log[%d] = %d\n", i, phy2log[i]);
>>>>>> + dev_dbg(&client->dev, "polarity[%d] = %d\n", i, polarities[i]);
>>>>>> + }
>>>>>> + dev_dbg(&client->dev, "oif_ctrl = 0x%04x\n", sensor->oif_ctrl);
>>>>>> +
>>>>>> + v4l2_fwnode_endpoint_free(&ep);
>>>>>> +
>>>>>> + return 0;
>>>>>> +
>>>>>> +error_ep:
>>>>>> + v4l2_fwnode_endpoint_free(&ep);
>>>>>> +error_alloc:
>>>>>> +
>>>>>> + return -EINVAL;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_configure(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + int sensor_freq;
>>>>>> + unsigned int prediv;
>>>>>> + unsigned int mult;
>>>>>> + int ret;
>>>>>> +
>>>>>> + compute_pll_parameters_by_freq(sensor->clk_freq, &prediv, &mult);
>>>>>> + sensor_freq = (mult * sensor->clk_freq) / prediv;
>>>>>> + /* Frequency to data rate is 1:1 ratio for MIPI */
>>>>>> + sensor->data_rate_in_mbps = sensor_freq;
>>>>>> + /* Video timing ISP path (pixel clock) requires 804/5 mhz = 160 mhz */
>>>>>> + sensor->pclk = sensor_freq / 5;
>>>>>> +
>>>>>> + /* Cache line_length value */
>>>>>> + ret = vgxy61_read_reg16(sensor, DEVICE_LINE_LENGTH, &sensor->line_length);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + /* Configure clocks */
>>>>>> + ret = vgxy61_write_reg32(sensor, DEVICE_EXT_CLOCK, sensor->clk_freq);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_CLK_PLL_PREDIV, prediv);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_CLK_SYS_PLL_MULT, mult);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + /* Configure interface */
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_OIF_CTRL, sensor->oif_ctrl);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + /* Disable pwm compression */
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_FRAME_CONTENT_CTRL, 0);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + /* Disable asil lines */
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_BYPASS_CTRL, 4);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + /* Set gpios polarity according to device tree value */
>>>>>> + ret = vgxy61_update_gpios_strobe_polarity(sensor, sensor->gpios_polarity);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + /* Slave mode */
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_VT_CTRL, sensor->slave_mode);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + /* Set pattern generator solid to middle value */
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_PATGEN_LONG_DATA_GR, 0x800);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_PATGEN_LONG_DATA_R, 0x800);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_PATGEN_LONG_DATA_B, 0x800);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_PATGEN_LONG_DATA_GB, 0x800);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_PATGEN_SHORT_DATA_GR, 0x800);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_PATGEN_SHORT_DATA_R, 0x800);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_PATGEN_SHORT_DATA_B, 0x800);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + ret = vgxy61_write_reg16(sensor, DEVICE_PATGEN_SHORT_DATA_GB, 0x800);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + dev_dbg(&client->dev, "clock prediv = %d\n", prediv);
>>>>>> + dev_dbg(&client->dev, "clock mult = %d\n", mult);
>>>>>> + dev_dbg(&client->dev, "data rate = %d mbps\n",
>>>>>> + sensor->data_rate_in_mbps);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_patch(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + u16 patch;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = vgxy61_write_array(sensor, DEVICE_FWPATCH_START_ADDR, sizeof(patch_array),
>>>>>> + patch_array);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = vgxy61_write_reg(sensor, DEVICE_STBY, 0x10);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = vgxy61_poll_reg(sensor, DEVICE_STBY, 0, VGXY61_TIMEOUT_MS);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = vgxy61_read_reg16(sensor, DEVICE_FWPATCH_REVISION, &patch);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + if (patch != (DEVICE_FWPATCH_REVISION_MAJOR << 12) +
>>>>>> + (DEVICE_FWPATCH_REVISION_MINOR << 8) +
>>>>>> + DEVICE_FWPATCH_REVISION_MICRO) {
>>>>>> + dev_err(&client->dev, "bad patch version expected %d.%d.%d got %d.%d.%d\n",
>>>>>> + DEVICE_FWPATCH_REVISION_MAJOR,
>>>>>> + DEVICE_FWPATCH_REVISION_MINOR,
>>>>>> + DEVICE_FWPATCH_REVISION_MICRO,
>>>>>> + patch >> 12, (patch >> 8) & 0x0f, patch & 0xff);
>>>>>> + return -ENODEV;
>>>>>> + }
>>>>>> + dev_dbg(&client->dev, "patch %d.%d.%d applied\n",
>>>>>> + patch >> 12, (patch >> 8) & 0x0f, patch & 0xff);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_detect_cut_version(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + u16 device_rev;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = vgxy61_read_reg16(sensor, DEVICE_REVISION, &device_rev);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + switch (device_rev >> 8) {
>>>>>> + case 0xA:
>>>>>> + dev_dbg(&client->dev, "Cut1 detected\n");
>>>>>> + dev_err(&client->dev, "Cut1 not supported by this driver\n");
>>>>>> + return -ENODEV;
>>>>>> + case 0xB:
>>>>>> + dev_dbg(&client->dev, "Cut2 detected\n");
>>>>>> + return 0;
>>>>>> + case 0xC:
>>>>>> + dev_dbg(&client->dev, "Cut3 detected\n");
>>>>>> + return 0;
>>>>>> + default:
>>>>>> + dev_err(&client->dev, "Unable to detect cut version\n");
>>>>>> + return -ENODEV;
>>>>>> + }
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_detect(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + struct i2c_client *client = sensor->i2c_client;
>>>>>> + u16 id = 0;
>>>>>> + int ret;
>>>>>> + u8 st;
>>>>>> +
>>>>>> + ret = vgxy61_read_reg16(sensor, DEVICE_MODEL_ID_REG, &id);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + if (id != VG5661_MODEL_ID && id != VG5761_MODEL_ID) {
>>>>>> + dev_warn(&client->dev, "Unsupported sensor id %x\n", id);
>>>>>> + return -ENODEV;
>>>>>> + }
>>>>>> + dev_dbg(&client->dev, "detected sensor id = 0x%04x\n", id);
>>>>>> + sensor->id = id;
>>>>>> +
>>>>>> + ret = vgxy61_wait_state(sensor, SW_STBY, VGXY61_TIMEOUT_MS);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = vgxy61_read_reg(sensor, DEVICE_NVM, &st);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> + if (st != NVM_OK)
>>>>>> + dev_warn(&client->dev, "Bad nvm state got %d\n", st);
>>>>>> +
>>>>>> + /* Detect cut version */
>>>>>> + ret = vgxy61_detect_cut_version(sensor);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +/* Power/clock management functions */
>>>>>> +static int vgxy61_power_on(struct device *dev)
>>>>>> +{
>>>>>> + struct i2c_client *client = to_i2c_client(dev);
>>>>>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = vgxy61_get_regulators(sensor);
>>>>>> + if (ret) {
>>>>>> + dev_err(&client->dev, "failed to get regulators %d\n", ret);
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + ret = regulator_bulk_enable(VGXY61_NUM_SUPPLIES, sensor->supplies);
>>>>>> + if (ret) {
>>>>>> + dev_err(&client->dev, "failed to enable regulators %d\n", ret);
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + ret = clk_prepare_enable(sensor->xclk);
>>>>>> + if (ret) {
>>>>>> + dev_err(&client->dev, "failed to enable clock %d\n", ret);
>>>>>> + goto disable_bulk;
>>>>>> + }
>>>>>> +
>>>>>> + /* Apply reset sequence */
>>>>>> + if (sensor->reset_gpio) {
>>>>>> + ret = vgxy61_apply_reset(sensor);
>>>>>> + if (ret) {
>>>>>> + dev_err(&client->dev, "sensor reset failed %d\n", ret);
>>>>>> + goto disable_clock;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + ret = vgxy61_detect(sensor);
>>>>>
>>>>> Do we really need to run detection every time the sensor is powered on ?
>>>>
>>>> Surely not, I will add a if (sensor->already_detected) guard for this
>>>> call to make it runs only once.
>>>
>>> It could also be done in probe().
>>>
>>>>>> + if (ret) {
>>>>>> + dev_err(&client->dev, "sensor detect failed %d\n", ret);
>>>>>> + goto disable_clock;
>>>>>> + }
>>>>>> +
>>>>>> + ret = vgxy61_patch(sensor);
>>>>>> + if (ret) {
>>>>>> + dev_err(&client->dev, "sensor patch failed %d\n", ret);
>>>>>> + goto disable_clock;
>>>>>> + }
>>>>>> +
>>>>>> + ret = vgxy61_configure(sensor);
>>>>>> + if (ret) {
>>>>>> + dev_err(&client->dev, "sensor configuration failed %d\n", ret);
>>>>>> + goto disable_clock;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +
>>>>>> +disable_clock:
>>>>>> + clk_disable_unprepare(sensor->xclk);
>>>>>> +disable_bulk:
>>>>>> + regulator_bulk_disable(VGXY61_NUM_SUPPLIES, sensor->supplies);
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_power_off(struct device *dev)
>>>>>> +{
>>>>>> + struct i2c_client *client = to_i2c_client(dev);
>>>>>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> +
>>>>>> + clk_disable_unprepare(sensor->xclk);
>>>>>> + regulator_bulk_disable(VGXY61_NUM_SUPPLIES, sensor->supplies);
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int __maybe_unused vgxy61_suspend(struct device *dev)
>>>>>> +{
>>>>>> + struct i2c_client *client = to_i2c_client(dev);
>>>>>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> +
>>>>>> + if (sensor->streaming)
>>>>>> + vgxy61_stream_disable(sensor);
>>>>>
>>>>> This shoudn't happen. The sensor should be stopped by the driver on the
>>>>> receiver side during system suspend. Same for resume.
>>>>
>>>> Sorry I'm still learning the pm_runtime API and I'm not sure I got your point.
>>>> If the sensor is already stopped then I could just remove the whole suspend function? Same for resume.
>>>> I'm not sure when the suspend function is called by the framework.
>>>
>>> vgxy61_suspend() is called on system suspend. Disabling streaming here
>>> is problematic, as it's done independently from the CSI-2 receiver. It's
>>> even worse on resume, as if you resume the sensor first, the CSI-2 lanes
>>> will go to HS mode before the receiver is started, and the receiver will
>>> never be able to synchronized. That's why the CSI-2 receiver resume
>>> handler needs to restart streaming by calling the sensor .s_stream()
>>> operation. Same thing at suspend time. Most sensors will thus not need
>>> system suspend/resume handlers.
>>>
>>> This means that the CSI-2 receiver needs to be suspended before the
>>> sensor, and resumed after. You can achieve this with the device link
>>> API, calling device_link_add(csi2_dev, sensor_dev, DL_FLAG_STATELESS)
>>> in the CSI-2 receiver driver.
>>
>> Let me rephrase your comment just to make sure I got everything right.
>>
>> On system suspend, the CSI-2 receiver driver should already have
>> stopped the stream for me, so this is irrelevant.
>>
>> On resume, if by any chance I try to resume streaming before the CSI2
>> receiver is resuming things will go terribly wrong.
>>
>> So this means I should trust the CSI2 driver to have performed
>> device_link_add on its side, and can delete the vgxy61 resume and
>> suspend ops.
>
> Correct.
>
>> I have trouble understanding why my use case differs from the imx219
>> for instance, that afaik does the same thing.
>
> Your use case doesn't differ, but we have lots of sensors drivers that
> have been developed over time while our understanding of power
> management evolved, and older drivers haven't been updated to the latest
> best practices. This results in lots of cargo-cult for new drivers too,
> it's not a great situation.
>
I understand, thanks a lot for your explanations.
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int __maybe_unused vgxy61_resume(struct device *dev)
>>>>>> +{
>>>>>> + struct i2c_client *client = to_i2c_client(dev);
>>>>>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (sensor->streaming) {
>>>>>> + ret = vgxy61_stream_enable(sensor);
>>>>>> + if (ret)
>>>>>> + goto error;
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +
>>>>>> +error:
>>>>>> + vgxy61_stream_disable(sensor);
>>>>>> + sensor->streaming = false;
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static void vgxy61_set_default_format(struct vgxy61_dev *sensor)
>>>>>> +{
>>>>>> + if (sensor->id == VG5761_MODEL_ID) {
>>>>>
>>>>> Would it make sense to have different compatible strings instead of
>>>>> detecting the version automatically ? Not only could it speed up probe
>>>>> if we could skip the detection, it would also allow not powering the
>>>>> sensor up at probe time, which may be required in some cases (for
>>>>> instance when a privacy LED is connected to the sensor power supply,
>>>>> flashing the LED at probe time can be very confusing for users).
>>>>
>>>> Your flashing LED example is a very good concern.
>>>>
>>>> The idea behind this is to ease the user life as he does not have to
>>>> know what model he is using.
>>>
>>> I agree with you, I've implemented the same in sensor drivers. One
>>> option may be to have a generic compatible string that would lead to
>>> model detection at probe time, and specific compatible strings that
>>> would skip it and avoid powering up the sensor. As long as we consider
>>> the requirements on the DT side and make sure to have a way forward
>>> without breaking backward compatibility, this can also be implemented
>>> later.
>>
>> This is a very good idea.
>>
>>>> For example we have several boards with either the VG5761 or VG5661
>>>> and are interchangeable without changing the device tree, which is
>>>> also better for us as it shrinks the device tree number to maintain.
>>>>
>>>> Also as a company we treat them the same way, hence the same driver.
>>>> I'm not hardly commited to keep this, but it indeed makes my life easier.
>>>>
>>>>>> + sensor->sensor_width = VGX761_WIDTH;
>>>>>> + sensor->sensor_height = VGX761_HEIGHT;
>>>>>> + sensor->sensor_modes = vgx761_mode_data;
>>>>>> + sensor->sensor_modes_nb = ARRAY_SIZE(vgx761_mode_data);
>>>>>> + sensor->current_mode = &vgx761_mode_data[VGX761_DEFAULT_MODE];
>>>>>> + sensor->rot_term = VGX761_SHORT_ROT_TERM;
>>>>>> + } else if (sensor->id == VG5661_MODEL_ID) {
>>>>>> + sensor->sensor_width = VGX661_WIDTH;
>>>>>> + sensor->sensor_height = VGX661_HEIGHT;
>>>>>> + sensor->sensor_modes = vgx661_mode_data;
>>>>>> + sensor->sensor_modes_nb = ARRAY_SIZE(vgx661_mode_data);
>>>>>> + sensor->current_mode = &vgx661_mode_data[VGX661_DEFAULT_MODE];
>>>>>> + sensor->rot_term = VGX661_SHORT_ROT_TERM;
>>>>>> + } else {
>>>>>> + /* Should never happen */
>>>>>> + WARN_ON(true);
>>>>>> + }
>>>>>> + sensor->fmt.width = sensor->current_mode->width;
>>>>>> + sensor->fmt.height = sensor->current_mode->height;
>>>>>> + sensor->vblank = sensor->frame_length - sensor->current_mode->crop.height;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_probe(struct i2c_client *client)
>>>>>> +{
>>>>>> + struct device *dev = &client->dev;
>>>>>> + struct fwnode_handle *endpoint;
>>>>>> + struct vgxy61_dev *sensor;
>>>>>> + int ret;
>>>>>> +
>>>>>> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
>>>>>> + if (!sensor)
>>>>>> + return -ENOMEM;
>>>>>> +
>>>>>> + sensor->i2c_client = client;
>>>>>> + sensor->streaming = false;
>>>>>> + sensor->fmt.code = MEDIA_BUS_FMT_Y8_1X8;
>>>>>> + sensor->fmt.colorspace = V4L2_COLORSPACE_RAW;
>>>>>> + sensor->fmt.field = V4L2_FIELD_NONE;
>>>>>> + sensor->fmt.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>>>>>> + sensor->fmt.quantization = V4L2_QUANTIZATION_DEFAULT;
>>>>>> + sensor->fmt.xfer_func = V4L2_XFER_FUNC_DEFAULT;
>>>>>
>>>>> Please implement the subdev .init_cfg() operation, and call it here.
>>>>> There are several examples in existing drivers.
>>>>>
>>>>>> + sensor->frame_interval.numerator = 1;
>>>>>> + sensor->frame_interval.denominator = 60;
>>>>>> + sensor->hdr = NO_HDR;
>>>>>> + sensor->expo_long = 200;
>>>>>> + sensor->expo_short = 0;
>>>>>> + sensor->hflip = false;
>>>>>> + sensor->vflip = false;
>>>>>> + sensor->frame_length = VGXY61_FRAME_LENGTH_DEF;
>>>>>> +
>>>>>> + endpoint = fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), NULL);
>>>>>> + if (!endpoint) {
>>>>>> + dev_err(dev, "endpoint node not found\n");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + ret = vgxy61_tx_from_ep(sensor, endpoint);
>>>>>> + fwnode_handle_put(endpoint);
>>>>>> + if (ret) {
>>>>>> + dev_err(dev, "Failed to parse endpoint %d\n", ret);
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> + sensor->xclk = devm_clk_get(dev, NULL);
>>>>>> + if (IS_ERR(sensor->xclk)) {
>>>>>> + dev_err(dev, "failed to get xclk\n");
>>>>>> + return PTR_ERR(sensor->xclk);
>>>>>> + }
>>>>>> + sensor->clk_freq = clk_get_rate(sensor->xclk);
>>>>>> + if (sensor->clk_freq < 6 * HZ_PER_MHZ || sensor->clk_freq > 27 * HZ_PER_MHZ) {
>>>>>> + dev_err(dev, "Only 6Mhz-27Mhz clock range supported. provide %lu MHz\n",
>>>>>> + sensor->clk_freq / HZ_PER_MHZ);
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> + sensor->gpios_polarity = of_property_read_bool(dev->of_node, "invert-gpios-polarity");
>>>>>> + sensor->slave_mode = of_property_read_bool(dev->of_node, "slave-mode");
>>>>>> +
>>>>>> + v4l2_i2c_subdev_init(&sensor->sd, client, &vgxy61_subdev_ops);
>>>>>> + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>>>>> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
>>>>>> + sensor->sd.entity.ops = &vgxy61_subdev_entity_ops;
>>>>>> + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>>>>>> +
>>>>>> + /* Request optional reset pin */
>>>>>> + sensor->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>>>>>> +
>>>>>> + ret = vgxy61_power_on(dev);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + vgxy61_set_default_format(sensor);
>>>>>> +
>>>>>> + /* HDR mode */
>>>>>> + ret = vgxy61_update_hdr(sensor, sensor->hdr);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = vgxy61_init_controls(sensor);
>>>>>> + if (ret) {
>>>>>> + dev_err(&client->dev, "controls initialization failed %d\n", ret);
>>>>>> + goto error_power_off;
>>>>>> + }
>>>>>> +
>>>>>> + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
>>>>>> + if (ret) {
>>>>>> + dev_err(&client->dev, "pads init failed %d\n", ret);
>>>>>> + goto error_handler_free;
>>>>>> + }
>>>>>> +
>>>>>> + ret = v4l2_async_register_subdev(&sensor->sd);
>>>>>> + if (ret) {
>>>>>> + dev_err(&client->dev, "async subdev register failed %d\n", ret);
>>>>>> + goto error_media_entity;
>>>>>> + }
>>>>>> +
>>>>>> + /* Enable runtime PM and turn off the device */
>>>>>> + pm_runtime_set_active(dev);
>>>>>> + pm_runtime_enable(dev);
>>>>>> + pm_runtime_idle(dev);
>>>>>
>>>>> You should do this before registering the subdev, as it can be accessed
>>>>> from userspace right after being registered.
>>>>>
>>>>> Please also use auto-suspend to avoid long delays when stopping and
>>>>> restarting streaming quickly.
>>>>
>>>> I was not aware of autosuspend. Thanks a lot, I will look into this.
>>>>
>>>>>> +
>>>>>> + dev_info(&client->dev, "vgxy61 probe successfully\n");
>>>>>
>>>>> I'd drop this, or turn it into a debug message.
>>>>>
>>>>>> +
>>>>>> + return 0;
>>>>>> +
>>>>>> +error_media_entity:
>>>>>> + media_entity_cleanup(&sensor->sd.entity);
>>>>>> +error_handler_free:
>>>>>> + v4l2_ctrl_handler_free(sensor->sd.ctrl_handler);
>>>>>> + mutex_destroy(&sensor->lock);
>>>>>> +error_power_off:
>>>>>> + vgxy61_power_off(dev);
>>>>>> +
>>>>>> + return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int vgxy61_remove(struct i2c_client *client)
>>>>>> +{
>>>>>> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>>>>> + struct vgxy61_dev *sensor = to_vgxy61_dev(sd);
>>>>>> +
>>>>>> + v4l2_async_unregister_subdev(&sensor->sd);
>>>>>> + mutex_destroy(&sensor->lock);
>>>>>> + media_entity_cleanup(&sensor->sd.entity);
>>>>>> +
>>>>>> + pm_runtime_disable(&client->dev);
>>>>>> + if (!pm_runtime_status_suspended(&client->dev))
>>>>>> + vgxy61_power_off(&client->dev);
>>>>>> + pm_runtime_set_suspended(&client->dev);
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct of_device_id vgxy61_dt_ids[] = {
>>>>>> + { .compatible = "st,st-vgxy61" },
>>>>>> + { /* sentinel */ }
>>>>>> +};
>>>>>> +MODULE_DEVICE_TABLE(of, vgxy61_dt_ids);
>>>>>> +
>>>>>> +static const struct dev_pm_ops vgxy61_pm_ops = {
>>>>>> + SET_SYSTEM_SLEEP_PM_OPS(vgxy61_suspend, vgxy61_resume)
>>>>>> + SET_RUNTIME_PM_OPS(vgxy61_power_off, vgxy61_power_on, NULL)
>>>>>> +};
>>>>>> +
>>>>>> +static struct i2c_driver vgxy61_i2c_driver = {
>>>>>> + .driver = {
>>>>>> + .name = "st-vgxy61",
>>>>>> + .of_match_table = vgxy61_dt_ids,
>>>>>> + .pm = &vgxy61_pm_ops,
>>>>>> + },
>>>>>> + .probe_new = vgxy61_probe,
>>>>>> + .remove = vgxy61_remove,
>>>>>> +};
>>>>>> +
>>>>>> +module_i2c_driver(vgxy61_i2c_driver);
>>>>>> +
>>>>>> +MODULE_AUTHOR("Benjamin Mugnier <benjamin.mugnier@xxxxxxxxxxx>");
>>>>>> +MODULE_AUTHOR("Mickael Guene <mickael.guene@xxxxxx>");
>>>>>> +MODULE_AUTHOR("Sylvain Petinot <sylvain.petinot@xxxxxxxxxxx>");
>>>>>> +MODULE_DESCRIPTION("VGXY61 camera subdev driver");
>>>>>> +MODULE_LICENSE("GPL v2");
>
