Re: [PATCH v2 2/2] media: i2c: Add Samsung S5KJN5 image sensor driver

Hi Wenmeng,

On 7/31/26 11:24, Wenmeng Liu wrote:
> 
> Hi Vladimir,
> On 7/25/2026 3:22 PM, Vladimir Zapolskiy wrote:
>> Hi Wenmeng.
>>
>> On 7/24/26 12:50, Wenmeng Liu wrote:
>>> Add a V4L2 sub-device driver for the Samsung S5KJN5, a 50 MP GBRG 10-bit
>>
>> I believe "GBRG" detalization here will be incorrect, if you consider image
>> flips or odd crops.
>>
>>> RAW MIPI CSI-2 image sensor. The driver supports the 4096x3072 mode and
>>> exposes the standard controls (exposure, gain, vblank/hblank, test
>>> pattern).
>>>
>>> Signed-off-by: Wenmeng Liu <[email protected]>
>>> ---
>>>    MAINTAINERS                |    1 +
>>>    drivers/media/i2c/Kconfig  |   11 +
>>>    drivers/media/i2c/Makefile |    1 +
>>>    drivers/media/i2c/s5kjn5.c | 2835 ++++++++++++++++++++++++++++++++++
>>> ++++++++++
>>>    4 files changed, 2848 insertions(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index
>>> 244c7ed5872aa0972a7b055d7e0b19d530820a08..6f6edae5de13efc0cda6c91ab8c9a40bfe2cbff9 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -24195,6 +24195,7 @@ M:    Wenmeng Liu <[email protected]>
>>>    L:    [email protected]
>>>    S:    Supported
>>>    F:    Documentation/devicetree/bindings/media/i2c/samsung,s5kjn5.yaml
>>> +F:    drivers/media/i2c/s5kjn5.c
>>>    SAMSUNG S5P Security SubSystem (SSS) DRIVER
>>>    M:    Krzysztof Kozlowski <[email protected]>
>>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>>> index
>>> 7ffccba897ae5637bd0cb913e1e04f9c7d0c89f5..06b5dbc32cab328df8d10da04d91a708bff6fdda 100644
>>> --- a/drivers/media/i2c/Kconfig
>>> +++ b/drivers/media/i2c/Kconfig
>>> @@ -824,6 +824,17 @@ config VIDEO_S5KJN1
>>>          To compile this driver as a module, choose M here: the
>>>          module will be called s5kjn1.
>>> +config VIDEO_S5KJN5
>>> +    tristate "Samsung S5KJN5 sensor support"
>>> +    depends on GPIOLIB
>>> +    select V4L2_CCI_I2C
>>> +    help
>>> +      This is a V4L2 sensor driver for Samsung S5KJN5 50MP GBRG
>>
>> Same comment as above, I believe "GBRG" should be either removed or
>> generalized to "Bayer" or "raw".
>>
>>> +      10-bit RAW MIPI CSI-2 camera sensor.
>>> +
>>> +      To compile this driver as a module, choose M here: the
>>> +      module will be called s5kjn5.
>>> +
>>>    config VIDEO_T4KA3
>>>        tristate "Toshiba T4KA3 sensor support"
>>>        depends on ACPI || COMPILE_TEST
>>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>>> index
>>> d04bd5724552e88fc2ad39f9bbaf9b4ce1763939..45019ce20a6c34c8139b8247b6904c3abb1a8fe7 100644
>>> --- a/drivers/media/i2c/Makefile
>>> +++ b/drivers/media/i2c/Makefile
>>> @@ -133,6 +133,7 @@ obj-$(CONFIG_VIDEO_S5K3M5) += s5k3m5.o
>>>    obj-$(CONFIG_VIDEO_S5K5BAF) += s5k5baf.o
>>>    obj-$(CONFIG_VIDEO_S5K6A3) += s5k6a3.o
>>>    obj-$(CONFIG_VIDEO_S5KJN1) += s5kjn1.o
>>> +obj-$(CONFIG_VIDEO_S5KJN5) += s5kjn5.o
>>>    obj-$(CONFIG_VIDEO_SAA6588) += saa6588.o
>>>    obj-$(CONFIG_VIDEO_SAA6752HS) += saa6752hs.o
>>>    obj-$(CONFIG_VIDEO_SAA7110) += saa7110.o
>>> diff --git a/drivers/media/i2c/s5kjn5.c b/drivers/media/i2c/s5kjn5.c
>>> new file mode 100644
>>> index
>>> 0000000000000000000000000000000000000000..865db808aabcf7d910acbdab8126b9fa308ef875
>>> --- /dev/null
>>> +++ b/drivers/media/i2c/s5kjn5.c
>>> @@ -0,0 +1,2835 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Samsung S5KJN5 50 MP GBRG 10-bit RAW MIPI CSI-2 sensor driver
>>
>> Same comment as above.
>>
>>> + *
>>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/gpio/consumer.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <media/v4l2-cci.h>
>>> +#include <media/v4l2-ctrls.h>
>>> +#include <media/v4l2-device.h>
>>
>> Let me ask, which interface or declaration from media/v4l2-ctrls.h
>> is used in the driver? I have a feeling that it's unused.
>>
> 
> It is required. The driver call many interfaces form it, like
> v4l2_ctrl_new_int_menu/__v4l2_ctrl_handler_setup...

my bad, I stopped at media/v4l2-device.h above, and my question is about
this media/v4l2-device.h header actually, can you please double check,
if the driver uses anything from that header or not?

I wrongly copied a definitely used media/v4l2-ctrls.h to my question,
it was not my intention, sorry.

> And thanks a lot for the review. ACK for the rest, and I'll address them
> in the next revision.
> 

Thank you! I will try my best to review the driver without a great delay.

-- 
Best wishes,
Vladimir