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

[Vladimir Zapolskiy]

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 <wenmeng.liu@xxxxxxxxxxxxxxxx>
> > > ---
> > >    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@xxxxxxxxxxxxxxxx>
> > >    L:    linux-media@xxxxxxxxxxxxxxx
> > >    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@xxxxxxxxxx>
> > > 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