| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<PN3P287MB18291DD344424C8F45790FC18BFBA@PN3P287MB1829.INDP287.PROD.OUTLOOK.COM>
Date: Thu, 30 Oct 2025 06:52:42 +0000
From: Tarang Raval <tarang.raval@...iconsignals.io>
To: Svyatoslav Ryhel <clamor95@...il.com>
CC: Sakari Ailus <sakari.ailus@...ux.intel.com>, Mauro Carvalho Chehab
<mchehab@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Hans Verkuil
<hverkuil@...all.nl>, Hans de Goede <hansg@...nel.org>, Arnd Bergmann
<arnd@...db.de>, Dongcheng Yan <dongcheng.yan@...el.com>,
André Apitzsch <git@...tzsch.eu>, Sylvain Petinot
<sylvain.petinot@...s.st.com>, Benjamin Mugnier
<benjamin.mugnier@...s.st.com>, Heimir Thor Sverrisson
<heimir.sverrisson@...il.com>, "linux-media@...r.kernel.org"
<linux-media@...r.kernel.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2 RESEND] media: i2c: add Sony IMX111 CMOS camera
sensor driver
Hi Svyatoslav,
> > > > On Tue, Oct 28, 2025 at 11:22:00AM +0200, Svyatoslav Ryhel wrote:
> > > > > Add a v4l2 sub-device driver for the Sony IMX111 image sensor. This is a
> > > > > camera sensor using the i2c bus for control and the csi-2 bus for data.
> > > > >
> > > > > The following features are supported:
> > > > > - manual exposure, digital and analog gain control support
> > > > > - pixel rate/link freq control support
> > > > > - supported resolution up to 3280x2464 for single shot capture
> > > > > - supported resolution up to 1920x1080 @ 30fps for video
> > > > > - supported bayer order output SGBRG10 and SGBRG8
> > > > >
> > > > > Camera module seems to be partially compatible with Nokia SMIA but it
> > > > > lacks a few registers required for clock calculations and has different
> > > > > vendor-specific per-mode configurations which makes it incompatible with
> > > > > existing CCS driver.
> > > > >
> > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@...il.com>
> >
> > ...
> >
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/delay.h>
> > > > > +#include <linux/gpio/consumer.h>
> > > > > +#include <linux/i2c.h>
> > > > > +#include <linux/kernel.h>
> > > > > +#include <linux/media.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/pm_runtime.h>
> > > > > +#include <linux/ratelimit.h>
> > > > > +#include <linux/regmap.h>
> > > > > +#include <linux/regulator/consumer.h>
> > > > > +#include <linux/slab.h>
> > > > > +#include <linux/string.h>
> > > > > +#include <linux/types.h>
> > > > > +#include <linux/videodev2.h>
> > > > > +#include <linux/units.h>
> > > > > +
> > > > > +#include <media/media-entity.h>
> > > > > +#include <media/v4l2-async.h>
> > > > > +#include <media/v4l2-cci.h>
> > > > > +#include <media/v4l2-ctrls.h>
> > > > > +#include <media/v4l2-device.h>
> > > > > +#include <media/v4l2-fwnode.h>
> > > > > +#include <media/v4l2-event.h>
> > > > > +#include <media/v4l2-image-sizes.h>
> > > > > +#include <media/v4l2-subdev.h>
> > > > > +#include <media/v4l2-mediabus.h>
> >
> > A few of those headers seem to be unused and can be removed
> >
> > Like:
> >
> > #include <linux/ratelimit.h>
> > #include <linux/slab.h>
> > #include <linux/string.h>
> > #include <media/v4l2-event.h>
> > #include <media/v4l2-image-sizes.h>
> >
> > ...
> >
> > > > > +/* product information registers */
> > > > > +#define IMX111_PRODUCT_ID CCI_REG16(0x0000)
> > > > > +#define IMX111_CHIP_ID 0x111
> > > > > +#define IMX111_REVISION CCI_REG8(0x0002)
> > > > > +#define IMX111_MANUFACTURER_ID CCI_REG8(0x0003)
> > > > > +#define IMX111_SMIA_VER CCI_REG8(0x0004)
> > > > > +#define IMX111_FRAME_COUNTER CCI_REG8(0x0005)
> > > > > +#define IMX111_PIXEL_ORDER CCI_REG8(0x0006)
> > > > > +
> > > > > +/* general configuration registers */
> > > > > +#define IMX111_STREAMING_MODE CCI_REG8(0x0100)
> > > > > +#define IMX111_MODE_STANDBY 0
> > > > > +#define IMX111_MODE_STREAMING 1
> > > > > +#define IMX111_IMAGE_ORIENTATION CCI_REG8(0x0101)
> > > > > +#define IMX111_IMAGE_HFLIP BIT(0)
> > > > > +#define IMX111_IMAGE_VFLIP BIT(1)
> > > > > +#define IMX111_SOFTWARE_RESET CCI_REG8(0x0103)
> > > > > +#define IMX111_RESET_ON 1
> > > > > +#define IMX111_GROUP_WRITE CCI_REG8(0x0104)
> > > > > +#define IMX111_GROUP_WRITE_ON 1
> > > > > +#define IMX111_FRAME_DROP CCI_REG8(0x0105)
> > > > > +#define IMX111_FRAME_DROP_ON 1
> > > > > +#define IMX111_CHANNEL_ID CCI_REG8(0x0110)
> > > > > +#define IMX111_SIGNALLING_MODE CCI_REG8(0x0111)
> > > > > +#define IMX111_DATA_DEPTH CCI_REG16(0x0112)
> > > > > +#define IMX111_DATA_DEPTH_RAW8 0x08
> > > > > +#define IMX111_DATA_DEPTH_RAW10 0x0a
> > > > > +
> > > > > +/* integration time registers */
> > > > > +#define IMX111_INTEGRATION_TIME CCI_REG16(0x0202)
> > > > > +#define IMX111_INTEGRATION_TIME_MIN 0x1
> > > > > +#define IMX111_INTEGRATION_TIME_MAX 0xffff
> > > > > +#define IMX111_INTEGRATION_TIME_STEP 1
> > > > > +
> > > > > +/* analog gain control */
> > > > > +#define IMX111_REG_ANALOG_GAIN CCI_REG8(0x0205)
> > > > > +#define IMX111_ANA_GAIN_MIN 0
> > > > > +#define IMX111_ANA_GAIN_MAX 240
> > > > > +#define IMX111_ANA_GAIN_STEP 1
> > > > > +#define IMX111_ANA_GAIN_DEFAULT 0
> > > > > +
> > > > > +/* digital gain control */
> > > > > +#define IMX111_REG_DIG_GAIN_GREENR CCI_REG16(0x020e)
> > > > > +#define IMX111_REG_DIG_GAIN_RED CCI_REG16(0x0210)
> > > > > +#define IMX111_REG_DIG_GAIN_BLUE CCI_REG16(0x0212)
> > > > > +#define IMX111_REG_DIG_GAIN_GREENB CCI_REG16(0x0214)
> > > > > +#define IMX111_DGTL_GAIN_MIN 0x0100
> > > > > +#define IMX111_DGTL_GAIN_MAX 0x0fff
> > > > > +#define IMX111_DGTL_GAIN_DEFAULT 0x0100
> > > > > +#define IMX111_DGTL_GAIN_STEP 1
> > > > > +
> > > > > +/* clock configuration registers */
> > > > > +#define IMX111_PIXEL_CLK_DIVIDER_PLL1 CCI_REG8(0x0301) /* fixed to 10 */
> > > > > +#define IMX111_SYSTEM_CLK_DIVIDER_PLL1 CCI_REG8(0x0303) /* fixed to 1 */
> > > > > +#define IMX111_PRE_PLL_CLK_DIVIDER_PLL1 CCI_REG8(0x0305)
> > > > > +#define IMX111_PLL_MULTIPLIER_PLL1 CCI_REG8(0x0307)
> > > > > +#define IMX111_PLL_SETTLING_TIME CCI_REG8(0x303c)
> > > > > +#define IMX111_PLL_SETTLING_TIME_DEFAULT 200
> > > > > +#define IMX111_POST_DIVIDER CCI_REG8(0x30a4)
> > > > > +#define IMX111_POST_DIVIDER_DIV1 2
> > > > > +#define IMX111_POST_DIVIDER_DIV2 0
> > > > > +#define IMX111_POST_DIVIDER_DIV4 1
> > > > > +
> > > > > +/* frame timing registers */
> > > > > +#define IMX111_VERTICAL_TOTAL_LENGTH CCI_REG16(0x0340)
> > > > > +#define IMX111_HORIZONTAL_TOTAL_LENGTH CCI_REG16(0x0342)
> > > > > +
> > > > > +/* image size registers */
> > > > > +#define IMX111_HORIZONTAL_START CCI_REG16(0x0344)
> > > > > +#define IMX111_VERTICAL_START CCI_REG16(0x0346)
> > > > > +#define IMX111_HORIZONTAL_END CCI_REG16(0x0348)
> > > > > +#define IMX111_VERTICAL_END CCI_REG16(0x034a)
> > > > > +#define IMX111_IMAGE_WIDTH CCI_REG16(0x034c)
> > > > > +#define IMX111_IMAGE_HEIGHT CCI_REG16(0x034e)
> >
> > In the mode register settings, you can use the above macros.
> >
> > > > > +static const struct cci_reg_sequence mode_820x614[] = {
> > > > > + { CCI_REG8(0x0340), 0x04 }, { CCI_REG8(0x0341), 0xec },
> > > > > + { CCI_REG8(0x0342), 0x0d }, { CCI_REG8(0x0343), 0xd0 },
> > > > > + { CCI_REG8(0x0344), 0x00 }, { CCI_REG8(0x0345), 0x08 },
> > > > > + { CCI_REG8(0x0346), 0x00 }, { CCI_REG8(0x0347), 0x34 },
> > > > > + { CCI_REG8(0x0348), 0x0c }, { CCI_REG8(0x0349), 0xd7 },
> > > > > + { CCI_REG8(0x034a), 0x09 }, { CCI_REG8(0x034b), 0xcb },
> > > > > + { CCI_REG8(0x034c), 0x03 }, { CCI_REG8(0x034d), 0x34 },
> > > > > + { CCI_REG8(0x034e), 0x02 }, { CCI_REG8(0x034f), 0x66 },
> >
> > Here, you can use those macros.
> >
> > Likewise, in every mode.
> >
>
> This is not as simple as a substitution, imx111 requires group write
> in order to write 16bit values to registers. I will see what I can do
> about this.
It is straightforward, even if group writes are present.
static const struct cci_reg_sequence mode_820x614[] = {
// group hold ON Register Write
{ IMX111_VERTICAL_TOTAL_LENGTH, 0x04ec },
.
.
.
{ IMX111_IMAGE_HEIGHT, 0x0266 },
// group hold OFF Register write
};
However, in your register settings, I do not find any group hold ON or OFF
register before or after these image size–related registers. This appears
to be a simple substitution.
If I have misunderstood something, please let me know.
One more suggestion: you can create a helper function for group writes
It is up to you, if you want, you can make this change.
static int imx111_group_write(struct imx111 *sensor, bool enable)
{
int ret;
if (enable)
cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE,
IMX111_GROUP_WRITE_ON, IMX111_GROUP_WRITE_ON,
&ret);
else
cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE,
IMX111_GROUP_WRITE_ON, 0, &ret);
return ret;
}
It removes the repeated cci_update_bits(... GROUP_WRITE ...) boilerplate.
Best Regards,
Tarang
Powered by blists - more mailing lists