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

On 8/1/2026 1:39 AM, Vladimir Zapolskiy wrote:
> 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.
> 

Yes, you're right. This header file is unnecessary. Thanks for catching 
that. I'll fix it in the next version.

Thanks,
wenmeng