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

[Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>]

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 <wenmeng.liu@oss.qualcomm.com>
>> ---
>>   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 <wenmeng.liu@oss.qualcomm.com>
>>   L:    linux-media@vger.kernel.org
>>   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 <krzk@kernel.org>
>> 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...

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

Thanks,
Wenmeng