Message-ID: <iannlsewdjytz4xrunrsxejbzfjl3po2ikkvvefxw2pj4zkoyq@icoeytfcrq75>
Date: Thu, 30 Oct 2025 17:37:31 +0100
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Svyatoslav Ryhel <clamor95@...il.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>, 
	Tarang Raval <tarang.raval@...iconsignals.io>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	Hans Verkuil <hverkuil@...all.nl>, Hans de Goede <hansg@...nel.org>, 
	André Apitzsch <git@...tzsch.eu>, Sylvain Petinot <sylvain.petinot@...s.st.com>, 
	Benjamin Mugnier <benjamin.mugnier@...s.st.com>, Dongcheng Yan <dongcheng.yan@...el.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 v3 2/2] media: i2c: add Sony IMX111 CMOS camera sensor
 driver

Hi Svyatoslav

On Thu, Oct 30, 2025 at 06:11:42PM +0200, Svyatoslav Ryhel wrote:
> чт, 30 жовт. 2025 р. о 17:50 Jacopo Mondi <jacopo.mondi@...asonboard.com> пише:
> >
> > Hello,
> >   sorry for entering the conversation
> >
> > On Thu, Oct 30, 2025 at 05:13:31PM +0200, Svyatoslav Ryhel wrote:
> > > чт, 30 жовт. 2025 р. о 16:55 Tarang Raval <tarang.raval@...iconsignals.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@...il.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.
> > >
> >
> > Personally, whenever doing reviews, receiving a reply that ignores the
> > merit of the comment and simply refers to the existing code base as an
> > excuse for not caring, it's what put me off the most.
> >
> > Please respect the time reviewers have invested in looking at your
> > code by at least considering their comment instead of dismissing them.
> > In this specific case you could have easily said "I like it more this
> > way and it's consistent with what other drivers do". The same cannot
> > be said for other comments that you have decided to ignore.
> >
>
> I did not state that I will ignore all the comments. Contrary, I will
> be applying most/all of them. My point is that Linux kernel
> documentation, which Sakari Ailus (not an offence or blaming) pointed
> in v2 clearly pointed to imx219 and imx319 as an valid reference

I don't think imx219 is outdated, it's actually one of the most
developed one and is a very good reference. Simply, it's not perfect,
and we keep finding bugs that you could avoid from the very beginning
if you take the time to look into comments you receive.

> drivers, while they seem to be outdated. So by using those as
> reference I wasted both my time and maintainers time. You say "Please
> respect the time reviewers" but no one respects my time.
>

I'm sorry if you feel disrispected but I think you have been pointed
to the best reference we have that simply happens not to be perfect ?

And you might ask "if they got a pass for broken code, why didn't I
get one" ? This is not what's happening though, nobody got a pass,
simply changing code that is already mainline takes more effort and
motivation and if we can avoid bugs from the very beginning we should
do so (otherwise someone will have to fix it later on).

> >
> > > > > +   case V4L2_CID_VBLANK:
> > > > > +         /* Update max exposure while meeting expected vblanking */
> > > > > +         max = sensor->cur_mode->height + ctrl->val - 5;
> > > >
> > > > You can define a macro for the value 5 to improve readability.
> > > > Also, make this change in the init_control function.
> > > >
> > >
> > > imx219 does not specifies this as a define
> > >
> >
> > It doesn't but it should, like 90% of other drivers in mainline do
> >
> >
> > > > > +         __v4l2_ctrl_modify_range(sensor->exposure,
> > > > > +                            sensor->exposure->minimum,
> > > > > +                            max, sensor->exposure->step, max);
> > > >
> > > > This may fail; consider adding an error check.
> > > >
> > >
> > > imx219 does not return error here too
> > >
> >
> > so ?
> >
> > > > > +         break;
> > > > > +   }
> > > > > +
> > > > > +   /*
> > > > > +    * Applying V4L2 control value only happens
> > > > > +    * when power is up for streaming
> > > > > +    */
> > > > > +   if (!pm_runtime_get_if_in_use(dev))
> > > > > +         return 0;
> > > > > +
> > > > > +   switch (ctrl->id) {
> > > > > +   case V4L2_CID_ANALOGUE_GAIN:
> > > > > +         cci_write(sensor->regmap, IMX111_REG_ANALOG_GAIN, ctrl->val, &ret);
> > > > > +         break;
> > > > > +   case V4L2_CID_DIGITAL_GAIN:
> > > > > +         ret = imx111_update_digital_gain(sensor, ctrl->val);
> > > > > +         break;
> > > > > +   case V4L2_CID_EXPOSURE:
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     IMX111_GROUP_WRITE_ON, &ret);
> > > > > +         cci_write(sensor->regmap, IMX111_INTEGRATION_TIME, ctrl->val, &ret);
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     0, &ret);
> > > > > +         break;
> > > > > +   case V4L2_CID_HBLANK:
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     IMX111_GROUP_WRITE_ON, &ret);
> > > > > +         dev_err(dev, "writing 0x%x to HTL\n", sensor->cur_mode->width + ctrl->val);
> > > > > +         cci_write(sensor->regmap, IMX111_HORIZONTAL_TOTAL_LENGTH,
> > > > > +                 sensor->cur_mode->width + ctrl->val, &ret);
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     0, &ret);
> > > > > +         break;
> > > > > +   case V4L2_CID_VBLANK:
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     IMX111_GROUP_WRITE_ON, &ret);
> > > > > +         dev_err(dev, "writing 0x%x to VTL\n", sensor->cur_mode->height + ctrl->val);
> > > > > +         cci_write(sensor->regmap, IMX111_VERTICAL_TOTAL_LENGTH,
> > > > > +                 sensor->cur_mode->height + ctrl->val, &ret);
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     0, &ret);
> > > > > +         break;
> > > > > +   case V4L2_CID_HFLIP:
> > > > > +   case V4L2_CID_VFLIP:
> > > > > +         cci_write(sensor->regmap, IMX111_IMAGE_ORIENTATION,
> > > > > +                 sensor->hflip->val | sensor->vflip->val << 1, &ret);
> > > > > +         break;
> > > > > +   case V4L2_CID_TEST_PATTERN:
> > > > > +         cci_write(sensor->regmap, IMX111_TEST_PATTERN, ctrl->val, &ret);
> > > > > +         break;
> > > > > +   case V4L2_CID_TEST_PATTERN_RED:
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     IMX111_GROUP_WRITE_ON, &ret);
> > > > > +         cci_write(sensor->regmap, IMX111_SOLID_COLOR_RED, ctrl->val, &ret);
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     0, &ret);
> > > > > +         break;
> > > > > +   case V4L2_CID_TEST_PATTERN_GREENR:
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     IMX111_GROUP_WRITE_ON, &ret);
> > > > > +         cci_write(sensor->regmap, IMX111_SOLID_COLOR_GR, ctrl->val, &ret);
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     0, &ret);
> > > > > +         break;
> > > > > +   case V4L2_CID_TEST_PATTERN_BLUE:
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     IMX111_GROUP_WRITE_ON, &ret);
> > > > > +         cci_write(sensor->regmap, IMX111_SOLID_COLOR_BLUE, ctrl->val, &ret);
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     0, &ret);
> > > > > +         break;
> > > > > +   case V4L2_CID_TEST_PATTERN_GREENB:
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     IMX111_GROUP_WRITE_ON, &ret);
> > > > > +         cci_write(sensor->regmap, IMX111_SOLID_COLOR_GB, ctrl->val, &ret);
> > > > > +         cci_update_bits(sensor->regmap, IMX111_GROUP_WRITE, IMX111_GROUP_WRITE_ON,
> > > > > +                     0, &ret);
> > > > > +         break;
> > > > > +   default:
> > > > > +         ret = -EINVAL;
> > > > > +   }
> > > > > +
> > > > > +   pm_runtime_put(dev);
> > > > > +
> > > > > +   return ret;
> > > > > +}
> > > >
> > > > ---
> > > >
> > > > > +static int imx111_init_controls(struct imx111 *sensor)
> > > > > +{
> > > > > +   const struct v4l2_ctrl_ops *ops = &imx111_ctrl_ops;
> > > > > +   struct device *dev = regmap_get_device(sensor->regmap);
> > > > > +   const struct imx111_mode *mode = sensor->cur_mode;
> > > > > +   struct v4l2_fwnode_device_properties props;
> > > > > +   struct v4l2_subdev *sd = &sensor->sd;
> > > >
> > > > No need for a new variable; there is only one user in the function.
> > > >
> > >
> > > This make code reading cleaner, no?
> > >
> > > > > +   struct v4l2_ctrl_handler *hdl = &sensor->hdl;
> > > > > +   s64 pixel_rate_min, pixel_rate_max;
> > > > > +   int i, ret;
> > > > > +
> > > > > +   ret = v4l2_fwnode_device_parse(dev, &props);
> > > > > +   if (ret < 0)
> > > > > +         return ret;
> > > > > +
> > > > > +   ret = v4l2_ctrl_handler_init(hdl, 13);
> > > >
> > > > Now there are 15 controls.
> > > >
> > > > No need for explicit error checking; you can omit the error check if you'd like.
> > > >
> > > > > +   if (ret)
> > > > > +         return ret;
> > > > > +
> > > > > +   pixel_rate_min = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW10);
> > > > > +   pixel_rate_max = div_u64(sensor->pixel_clk_raw, 2 * IMX111_DATA_DEPTH_RAW8);
> > > > > +   sensor->pixel_rate = v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE,
> > > > > +                                  pixel_rate_min, pixel_rate_max,
> > > > > +                                  1, div_u64(sensor->pixel_clk_raw,
> > > > > +                                  2 * sensor->data_depth));
> > > > > +
> > > > > +   sensor->link_freq = v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
> > > > > +                                    0, 0, &sensor->default_link_freq);
> > > > > +   if (sensor->link_freq)
> > > > > +         sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > > +
> > > > > +   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_ANALOGUE_GAIN,
> > > > > +                 IMX111_ANA_GAIN_MIN, IMX111_ANA_GAIN_MAX,
> > > > > +                 IMX111_ANA_GAIN_STEP, IMX111_ANA_GAIN_DEFAULT);
> > > > > +
> > > > > +   v4l2_ctrl_new_std(hdl, ops, V4L2_CID_DIGITAL_GAIN,
> > > > > +                 IMX111_DGTL_GAIN_MIN, IMX111_DGTL_GAIN_MAX,
> > > > > +                 IMX111_DGTL_GAIN_STEP, IMX111_DGTL_GAIN_DEFAULT);
> > > > > +
> > > > > +   sensor->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
> > > > > +   if (sensor->hflip)
> > > > > +         sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > > > +
> > > > > +   sensor->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
> > > > > +   if (sensor->vflip)
> > > > > +         sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > > > > +
> > > > > +   sensor->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, IMX111_VBLANK_MIN,
> > > > > +                              IMX111_VTL_MAX - mode->height, 1,
> > > > > +                              mode->vtl_def - mode->height);
> > > > > +   sensor->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK, IMX111_HBLANK_MIN,
> > > > > +                              IMX111_HTL_MAX - mode->width, 1,
> > > > > +                              mode->htl_def - mode->width);
> > > > > +
> > > > > +   /*
> > > > > +    * The maximum coarse integration time is the frame length in lines
> > > > > +    * minus five.
> > > > > +    */
> > > > > +   sensor->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE,
> > > > > +                                IMX111_INTEGRATION_TIME_MIN,
> > > > > +                                IMX111_PIXEL_ARRAY_HEIGHT - 5,
> > > > > +                                IMX111_INTEGRATION_TIME_STEP,
> > > > > +                                IMX111_PIXEL_ARRAY_HEIGHT - 5);
> > > > > +
> > > > > +   v4l2_ctrl_new_fwnode_properties(hdl, ops, &props);
> > > > > +
> > > > > +   v4l2_ctrl_new_std_menu_items(hdl, ops, V4L2_CID_TEST_PATTERN,
> > > > > +                          ARRAY_SIZE(test_pattern_menu) - 1, 0, 0,
> > > > > +                          test_pattern_menu);
> > > > > +   for (i = 0; i < 4; i++) {
> > > > > +         /*
> > > > > +          * The assumption is that
> > > > > +          * V4L2_CID_TEST_PATTERN_GREENR == V4L2_CID_TEST_PATTERN_RED + 1
> > > > > +          * V4L2_CID_TEST_PATTERN_BLUE   == V4L2_CID_TEST_PATTERN_RED + 2
> > > > > +          * V4L2_CID_TEST_PATTERN_GREENB == V4L2_CID_TEST_PATTERN_RED + 3
> > > > > +          */
> > > > > +         v4l2_ctrl_new_std(hdl, ops, V4L2_CID_TEST_PATTERN_RED + i,
> > > > > +                       IMX111_TESTP_COLOUR_MIN, IMX111_TESTP_COLOUR_MAX,
> > > > > +                       IMX111_TESTP_COLOUR_STEP, IMX111_TESTP_COLOUR_MAX);
> > > > > +         /* The "Solid color" pattern is white by default */
> > > > > +   }
> > > > > +
> > > > > +   if (hdl->error)
> > > > > +         return hdl->error;
> > > > > +
> > > > > +   sd->ctrl_handler = hdl;
> > > > > +
> > > > > +   return 0;
> > > > > +};
> > > >
> > > > ---
> > > >
> > > > > +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.
> >
> > Could you at least take the time to read the code ?
> >

To be honest, this is what had me jump into this conversation as an
indication you had not really put attention to the comments you
received.


> > int cci_write(struct regmap *map, u32 reg, u64 val, int *err)
> > {
> >         bool little_endian;
> >         unsigned int len;
> >         u8 buf[8];
> >         int ret;
> >
> >         if (err && *err)
> >                 return *err;
> >
> > And by the way, the documentation says:
> >
> > /**
> >  * cci_write() - Write a value to a single CCI register
> >  *
> >  * @map: Register map to write to
> >  * @reg: Register address to write, use CCI_REG#() macros to encode reg width
> >  * @val: Value to be written
> >  * @err: Optional pointer to store errors, if a previous error is set
> >  *       then the write will be skipped
> >  *
> >  * Return: %0 on success or a negative error code on failure.
> >  */
> > int cci_write(struct regmap *map, u32 reg, u64 val, int *err);
> >
> >
> > >
> > > > > +
> > > > > +   /* Configure the PLL. */
> > > > > +   cci_write(sensor->regmap, IMX111_PRE_PLL_CLK_DIVIDER_PLL1,
> > > > > +           sensor->pll->pre_div, &ret);
> >
> > I'm very surprised this doesn't sometimes fail as ret is not
> > initialized
> >
> > > > > +   cci_write(sensor->regmap, IMX111_PLL_MULTIPLIER_PLL1, sensor->pll->mult, &ret);
> > > > > +   cci_write(sensor->regmap, IMX111_POST_DIVIDER, IMX111_POST_DIVIDER_DIV1, &ret);
> > > > > +   cci_write(sensor->regmap, IMX111_PLL_SETTLING_TIME,
> > > > > +           to_settle_delay(sensor->pll->extclk_rate), &ret);
> > > > > +
> > > > > +   ret = cci_multi_reg_write(sensor->regmap, imx111_global_init,
> > > > > +                       ARRAY_SIZE(imx111_global_init), NULL);
> > > >
> > > > You are overwriting the previous errors.
> > > >
> > > > please use ret |=
> >
> > or you can pass ret to cci_multi_reg_write() as well
> >
> > Maybe that's why you don't see errors causes by uninitialized ret ?
> >
> > > >
> > > > > +   if (ret < 0) {
> > > > > +         dev_err(dev, "Failed to initialize the sensor\n");
> > > > > +         return ret;
> > > > > +   }
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > >
> > > > ---
> > > >
> > > > > +static int imx111_set_format(struct v4l2_subdev *sd,
> > > > > +                    struct v4l2_subdev_state *state,
> > > > > +                    struct v4l2_subdev_format *format)
> > > > > +{
> > > > > +   struct imx111 *sensor = sd_to_imx111(sd);
> > > > > +   struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
> > > > > +   struct v4l2_mbus_framefmt *fmt;
> > > > > +   const struct imx111_mode *mode;
> > > > > +
> > > > > +   mode = v4l2_find_nearest_size(imx111_modes, ARRAY_SIZE(imx111_modes),
> > > > > +                           width, height,
> > > > > +                           mbus_fmt->width, mbus_fmt->height);
> > > > > +
> > > > > +   fmt = v4l2_subdev_state_get_format(state, format->pad);
> > > > > +
> > > > > +   fmt->code = imx111_get_format_code(sensor, mbus_fmt->code, false);
> > > > > +   fmt->width = mode->width;
> > > > > +   fmt->height = mode->height;
> > > > > +   fmt->colorspace = V4L2_COLORSPACE_RAW;
> > > > > +
> > > > > +   *mbus_fmt = *fmt;
> > > > > +
> > > > > +   if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > > > +         sensor->cur_mode = mode;
> > > > > +         sensor->data_depth = imx111_get_format_bpp(fmt);
> > > > > +         __v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate,
> > > > > +                            div_u64(sensor->pixel_clk_raw, 2 * sensor->data_depth));
> > > > > +
> > > > > +         __v4l2_ctrl_modify_range(sensor->vblank, IMX111_VBLANK_MIN,
> > > > > +                            IMX111_VTL_MAX - mode->height, 1,
> > > > > +                            mode->vtl_def - mode->height);
> > > > > +         __v4l2_ctrl_s_ctrl(sensor->vblank, mode->vtl_def - mode->height);
> > > > > +
> > > > > +         __v4l2_ctrl_modify_range(sensor->hblank, IMX111_HBLANK_MIN,
> > > > > +                            IMX111_HTL_MAX - mode->width, 1,
> > > > > +                            mode->htl_def - mode->width);
> > > > > +         __v4l2_ctrl_s_ctrl(sensor->hblank, mode->htl_def - mode->width);
> > > >
> > > > All the above V4L2 operations need to check for errors.
> > > >
> > >
> > > yet again imx219 and imx319 do not check any of those
> > >
> >
> > And we recently got an error on imx219 controls update that went
> > unnoticed because of this and I'm now fixing 40+ drivers because this
> > has been copied over and over. Want to make them 41 ?
> >
>
> There will pop 41 - 42 - 43 if documentation continues pointing to
> outdated driver as a reference.
>
> > > > > +   }
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > >
> > > > ---
> > > >
> > > > > +static int imx111_identify_module(struct imx111 *sensor)
> > > > > +{
> > > > > +   struct device *dev = regmap_get_device(sensor->regmap);
> > > > > +   u64 value, revision, manufacturer;
> > > > > +   int ret;
> > > > > +
> > > > > +   ret = cci_read(sensor->regmap, IMX111_PRODUCT_ID, &value, NULL);
> > > > > +   if (ret)
> > > > > +         return ret;
> > > > > +
> > > > > +   if (value != IMX111_CHIP_ID) {
> > > > > +         dev_err(dev, "chip id mismatch: %x!=%04llx", IMX111_CHIP_ID, value);
> > > > > +         return -ENXIO;
> > > > > +   }
> > > > > +
> > > > > +   cci_read(sensor->regmap, IMX111_REVISION, &revision, NULL);
> > > > > +   cci_read(sensor->regmap, IMX111_MANUFACTURER_ID, &manufacturer, NULL);
> > > >
> > > > Instead of NULL, pass ret for the error code, and return ret at the end.
> > > >
> > > > > +
> > > > > +   dev_dbg(dev, "module IMX%03llx rev. %llu manufacturer %llu\n",
> > > > > +         value, revision, manufacturer);
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > > > +
> > > > > +static int imx111_clk_init(struct imx111 *sensor)
> > > > > +{
> > > > > +   struct device *dev = regmap_get_device(sensor->regmap);
> > > > > +   u32 ndata_lanes = sensor->bus_cfg.bus.mipi_csi2.num_data_lanes;
> > > > > +   u64 extclk_rate, system_clk;
> > > > > +   unsigned int i;
> > > > > +
> > > > > +   extclk_rate = clk_get_rate(sensor->extclk);
> > > > > +   if (!extclk_rate)
> > > > > +         return dev_err_probe(dev, -EINVAL, "EXTCLK rate unknown\n");
> > > > > +
> > > > > +   for (i = 0; i < ARRAY_SIZE(imx111_pll); i++) {
> > > > > +         if (clk_get_rate(sensor->extclk) == imx111_pll[i].extclk_rate) {
> > > > > +               sensor->pll = &imx111_pll[i];
> > > > > +               break;
> > > > > +         }
> > > > > +   }
> > > > > +   if (!sensor->pll)
> > > > > +         return dev_err_probe(dev, -EINVAL, "Unsupported EXTCLK rate %llu\n", extclk_rate);
> > > >
> > > > Max line length should be 80 columns. This applies everywhere the line
> > > > length exceeds 80 characters.
> >
> > In response to your reply in a separate email:
> >
> > https://www.kernel.org/doc/html/latest/driver-api/media/maintainer-entry-profile.html#coding-style-addendum
> > Media development uses checkpatch.pl on strict mode to verify the code style, e.g.:
> > $ ./scripts/checkpatch.pl --strict --max-line-length=80
> >
> > I don't like being that strict too. Feel free to send a patch, you'll
> > have my ack
> >
>
> Oh God, I would like to but I don't want to touch this more than I
> need to. I have not faced such restrictiveness in any other kernel
> subsystem so far.
>

I agree we should move over 80 cols, it's a discussion we already had
many times but if rules have to be changed they have to be changed at
the subsystem level, not because you and me dislike them.

I'll ignore the following as it doesn't add anything to the
conversation.

> "People with restrictive hardware shouldn't make it more inconvenient
> for people who have better resources. Yes, we'll accommodate things to
> within reasonable limits. But no, 80-column terminals in 2020 isn't
> "reasonable" any more as far as I'm concerned. People commonly used
> 132-column terminals even back in the 80's, for chrissake, don't try
> to make 80 columns some immovable standard."
>
> > > >
> > > > > +
> > > > > +   system_clk = div_u64(extclk_rate, sensor->pll->pre_div) * sensor->pll->mult;
> > > > > +
> > > > > +   /*
> > > > > +    * Pixel clock or Logic clock is used for internal image processing is
> > > > > +    * generated by dividing into 1/10 or 1/8 frequency according to the
> > > > > +    * word length of the CSI2 interface. This clock is designating the pixel
> > > > > +    * rate and used as the base of integration time, frame rate etc.
> > > > > +    */
> > > > > +   sensor->pixel_clk_raw = system_clk * ndata_lanes;
> > > > > +
> > > > > +   /*
> > > > > +    * The CSI-2 bus is clocked for 16-bit per pixel, transmitted in DDR over n lanes
> > > > > +    * for RAW10 default format.
> > > > > +    */
> > > > > +   sensor->default_link_freq = div_u64(sensor->pixel_clk_raw * 8,
> > > > > +                               2 * IMX111_DATA_DEPTH_RAW10);
> > > > > +
> > > > > +   if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> > > > > +       sensor->bus_cfg.link_frequencies[0] != sensor->default_link_freq)
> > > > > +         return dev_err_probe(dev, -EINVAL,
> > > > > +                          "Unsupported DT link-frequencies, expected %llu\n",
> > > > > +                          sensor->default_link_freq);
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > >
> > > > ---
> > > >
> > > > > +static const struct of_device_id imx111_of_match[] = {
> > > > > +   { .compatible = "sony,imx111" },
> > > > > +   { /* sentinel */ }
> > > > > +};
> > > > > +MODULE_DEVICE_TABLE(of, imx111_of_match);
> > > > > +
> > > > > +static struct i2c_driver imx111_i2c_driver = {
> > > > > +   .driver = {
> > > > > +         .name = "imx111",
> > > > > +         .of_match_table = imx111_of_match,
> > > > > +         .pm = &imx111_pm_ops,
> > > > > +   },
> > > > > +   .probe = imx111_probe,
> > > > > +   .remove = imx111_remove,
> > > > > +};
> > > > > +module_i2c_driver(imx111_i2c_driver);
> > > > > +
> > > > > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@...il.com>");
> > > > > +MODULE_DESCRIPTION("Sony IMX111 CMOS Image Sensor driver");
> > > > > +MODULE_LICENSE("GPL");
> > > >
> > > > Best Regards,
> > > > Tarang

Powered by blists - more mailing lists