Re: [PATCH v2 2/2] media: i2c: Add driver for ST VD56G3 camera sensor

[Sakari Ailus]

Hi Sylwain,

Apologies for the delay...

On Mon, Jun 03, 2024 at 11:59:29AM +0200, Sylvain Petinot wrote:
> >> +/*
> >> + * The VD56G3 pixel array is organized as follows:
> >> + *
> >> + * +--------------------------------+
> >> + * |                                | \
> >> + * |   +------------------------+   |  |
> >> + * |   |                        |   |  |
> >> + * |   |                        |   |  |
> >> + * |   |                        |   |  |
> >> + * |   |                        |   |  |
> >> + * |   |                        |   |  |
> >> + * |   |   Default resolution   |   |  | Native height (1364)
> > 
> > What's outside the default resolution? It doesn't appear the driver would
> > allow capturing pixels out side this area.
> 
> Well both native and default resolutions are supported in the
> 'vd56g3_supported_modes' below.
> However this quite exotic resolution (1364 x 1124) isn't well supported
> by csi receivers, ISPs. That's why the default resolution of the driver
> is 1120 x 1360 (multiple of 16).

Ack.

I'd still keep the native resolution as the default and allow configuring
something else if the user space wants to.

The desired resolution really depends on the use case (as well as the ISP).

> >> +		break;
> >> +	case V4L2_CID_EXPOSURE_AUTO:
> >> +		is_auto = (ctrl->val == V4L2_EXPOSURE_AUTO);
> >> +		__v4l2_ctrl_grab(sensor->ae_lock_ctrl, !is_auto);
> >> +		__v4l2_ctrl_grab(sensor->ae_bias_ctrl, !is_auto);
> >> +		break;
> >> +	default:
> >> +		break;
> > 
> > You could omit default here.
> 
> I don't really like switch case without default. For sure I can omit,
> but I prefer making it explicit.

I'm ok with that.

...

> >> +static int vd56g3_power_off(struct vd56g3 *sensor)
> >> +{
> >> +	clk_disable_unprepare(sensor->xclk);
> >> +	gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> >> +	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
> >> +	return 0;
> > 
> > You can make the return type void.
> > 
> > Do you need two pairs of functions doing the same, or could you call
> > vd56g3_runtime_resume and vd56g3_runtime_suspend from driver's probe and
> > remove functions, too?
> 
> "Well, in fact, I tested both options before submitting V2 (I mean the
> unification of vd56g3_runtime_resume/suspend functions with
> vd56g3_power_on/off).
> 
> The unification option has the advantage of simplifying the code and
> removing two "useless" functions. The only drawback is that I had to
> call v4l2_i2c_subdev_init() earlier in the probe() function, whereas
> it's currently called in vd56g3_subdev_init() (currently at the end of
> the probe()). OK, it's not a big deal, but I find that the resulting
> code is not as well structured/divided (thus readable).
> 
> I'm interested to get your feedback to decide wich option to push for V3.

I'd prefer calling v4l2_i2c_subdev_init() earlier, in order to set the
device context. These are usually among those things that should be done as
early as possible, in order to avoid invalid pointers where much of the
driver code expects something else.

Btw. if you're not in too much hurry (I guess so?), we're just about to
rework the sensor API, to include internal pads and embedded data, for
better sensor configurability. It'll take a while before we're there
though, but when this driver is merged, the existing API must continue to
work.

-- 
Kind regards,

Sakari Ailus