Re: [PATCH v2 2/2] media: i2c: add Samsung S5KJN1 image sensor device driver

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

Hi Vladimir,

On Thu, Oct 23, 2025 at 03:13:15AM +0300, Vladimir Zapolskiy wrote:
> Hi Sakari,
> 
> thank you so much for review!
> 
> On 10/22/25 11:45, Sakari Ailus wrote:
> > Hi Vladimir,
> > 
> > On Thu, Oct 16, 2025 at 05:04:19AM +0300, Vladimir Zapolskiy wrote:
> > > Samsung S5KJN1 is a 50MP image sensor, it produces Bayer GRBG (2x2)
> > > frames in RAW10 output format, the maximum supported output resolution
> > > is 8160x6144 at 10 frames per second rate.
> > > 
> > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> 
> <snip>
> 
> > > +
> > > +#define S5KJN1_NUM_SUPPLIES	ARRAY_SIZE(s5kjn1_supply_names)
> > 
> > Please use ARRAY_SIZE() directly where you need this.
> > 
> 
> There are 6 places of the macro usage in the driver, but will do it.
> 
> <snip>
> 
> > > +
> > > +static u64 s5kjn1_mode_to_pixel_rate(const struct s5kjn1_mode *mode)
> > > +{
> > > +	u64 pixel_rate;
> > > +
> > > +	pixel_rate = s5kjn1_link_freq_menu[0] * 2 * S5KJN1_DATA_LANES;
> > > +	do_div(pixel_rate, 10);			/* bits per pixel */
> > 
> > You could also use div_u64().
> > 
> 
> Right, also it would make sense to change the argument from mode to freq,
> that's what I notice.

Ack.

...

> > > +static int s5kjn1_set_pad_format(struct v4l2_subdev *sd,
> > > +				 struct v4l2_subdev_state *state,
> > > +				 struct v4l2_subdev_format *fmt)
> > > +{
> > > +	struct s5kjn1 *s5kjn1 = to_s5kjn1(sd);
> > > +	s64 hblank, vblank, exposure_max;
> > > +	const struct s5kjn1_mode *mode;
> > > +
> > > +	mode = v4l2_find_nearest_size(s5kjn1_supported_modes,
> > > +				      ARRAY_SIZE(s5kjn1_supported_modes),
> > > +				      width, height,
> > > +				      fmt->format.width, fmt->format.height);
> > > +
> > > +	s5kjn1_update_pad_format(s5kjn1, mode, &fmt->format);
> > > +
> > > +	/* Format code can be updated with respect to flip controls */
> > > +	*v4l2_subdev_state_get_format(state, 0) = fmt->format;
> > > +
> > > +	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY)
> > > +		return 0;
> > > +
> > > +	if (s5kjn1->mode == mode)
> > > +		return 0;
> > > +
> > > +	s5kjn1->mode = mode;
> > > +
> > > +	/* Update limits and set FPS and exposure to default values */
> > > +	hblank = mode->hts - mode->width;
> > > +	__v4l2_ctrl_modify_range(s5kjn1->hblank, hblank, hblank, 1, hblank);
> > > +
> > > +	vblank = mode->vts - mode->height;
> > > +	__v4l2_ctrl_modify_range(s5kjn1->vblank, vblank,
> > > +				 S5KJN1_VTS_MAX - mode->height, 1, vblank);
> > > +	__v4l2_ctrl_s_ctrl(s5kjn1->vblank, vblank);
> > > +
> > > +	exposure_max = mode->vts - mode->exposure_margin;
> > > +	__v4l2_ctrl_modify_range(s5kjn1->exposure, S5KJN1_EXPOSURE_MIN,
> > > +				 exposure_max, S5KJN1_EXPOSURE_STEP,
> > > +				 mode->exposure);
> > > +	__v4l2_ctrl_s_ctrl(s5kjn1->exposure, mode->exposure);
> > 
> > Note that these can also fail. Assigning the format to the state should
> > thus be done as last.
> 
> Likely it could happen due to some obscure reasons, but it is not expected
> to happen due to the new applied mode settings, because the settings are
> the default ones for the selected mode. Anyway, I agree that in general
> an error could appear, I'll add the next check before changing the state:
> 
>         if (s5kjn1->sd.ctrl_handler->error)
>                 return s5kjn1->sd.ctrl_handler->error;

The control handler's error state is set when setting up the control
handler itself somehow fails, not when applying a value to a control fails.
I.e. you should check the return values from the above functions only.

E.g. setting the vertical blanking value typically results in an I²C write.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> 
> <snip>
> 
> > > +
> > > +static const struct v4l2_subdev_pad_ops s5kjn1_pad_ops = {
> > > +	.set_fmt = s5kjn1_set_pad_format,
> > > +	.get_fmt = v4l2_subdev_get_fmt,
> > > +	.enum_mbus_code = s5kjn1_enum_mbus_code,
> > > +	.enum_frame_size = s5kjn1_enum_frame_size,
> > > +	.enable_streams = s5kjn1_enable_streams,
> > > +	.disable_streams = s5kjn1_disable_streams,
> > 
> > Could you also add selections support, even if they're all read-only?
> > 
> 
> Will it be sufficient to set
> 
>         sel->r.top = 0;
>         sel->r.left = 0;
>         sel->r.width = fmt->width;
>         sel->r.height = fmt->height;
> 
> for the crop selection targets like it's done in ov2640 case for instance?

Preferrably like e.g. imx219.

-- 
Kind regards,

Sakari Ailus