Re: [PATCH v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Svyatoslav,

On Thu, Oct 30, 2025 at 05:13:31PM +0200, Svyatoslav Ryhel wrote:
> чт, 30 жовт. 2025 р. о 16:55 Tarang Raval <tarang.raval@siliconsignals.io> пише:
> >
> > Hi Svyatoslav,
> >
> > > 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@gmail.com>
> >
> > ---
> >
> > > +static int imx111_set_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +   struct imx111 *sensor = ctrl_to_imx111(ctrl);
> > > +   struct device *dev = regmap_get_device(sensor->regmap);
> > > +   s64 max;
> > > +   int ret = 0;
> > > +
> > > +   /* Propagate change of current control to all related controls */
> > > +   switch (ctrl->id) {
> >
> > Do we need the switch statement, since only one case is present?
> > You can use an 'if' instead.
> >
> 
> imx219 and imx319 which are recommended references use switch, and it
> seems that media maintainters are particularly picky to code style, I
> have copied it from there.

The documentation lists areas where the mentioned drivers serve as good
examples, it does not say everything in those drivers is done the way it is
best. Common sense indeed should prevail.

The reason why we mention these these drivers as examples is that there are
things that have been notoriously hard to get right (such as runtime PM
usage in sensor drivers).

...

> > > +static int imx111_initialize(struct imx111 *sensor)
> > > +{
> > > +   struct device *dev = regmap_get_device(sensor->regmap);
> > > +   int ret;
> >
> > ret = 0;
> >
> 
> cci_write does not state that ret must be initiated.

It does not explicitly use that wording but the documentation is clear
enough IMO.

-- 
Kind regards,

Sakari Ailus