Re: [PATCH v3 5/5] media: i2c: Add driver for ST VGXY61 camera sensor

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

Hi Benjamin,

On Fri, Aug 26, 2022 at 04:12:22PM +0200, Benjamin MUGNIER wrote:
> > > +	int rot_term;
> > 
> > rot_term value is always the same. No need for a field here for that.
> > 
> 
> It changes according to the sensor's model id and could be either
> VGX761_SHORT_ROT_TERM or VGX661_SHORT_ROT_TERM. See the
> vgxy61_fill_sensor_param function.

Ah, thanks for the explanation. I somehow had missed that earlier.

> > > +	struct i2c_client *client = sensor->i2c_client;
> > > +	/* Correction factor for time required between 2 lines */
> > > +	const int mipi_margin = 1056;
> > 
> > What does this value signify? Is it really constant or somehow dependent by
> > the configuration set by the driver?
> 
> It is a simplification of time needed to send short packets and for the MIPI
> to add transition times (EoT, LPS, and SoT packet delimiters) needed by the
> protocol to go in low power between 2 packets of data. This is a mipi IP
> constant for the sensor.

Ok. A comment elaborating this as above would be nice.

> > > +	ctrl = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ, ARRAY_SIZE(link_freq) - 1, 0,
> > > +				      link_freq);
> > > +	ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +	/* Custom controls */
> > > +	v4l2_ctrl_new_custom(hdl, &vgxy61_hdr_ctrl, NULL);
> > > +	/* Keep a pointer to these controls as we need to update them when setting the format */
> > > +	sensor->pixel_rate_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE, 1, INT_MAX, 1,
> > > +						    get_pixel_rate(sensor));
> > > +	sensor->pixel_rate_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +	sensor->expo_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, sensor->expo_min,
> > > +					      sensor->expo_max, 1, sensor->expo_long);
> > > +	sensor->vblank_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK, sensor->vblank_min,
> > > +						0xffff - sensor->current_mode->crop.height, 1,
> > > +						sensor->vblank);
> > > +	sensor->vflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, sensor->vflip);
> > > +	sensor->hflip_ctrl = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, sensor->hflip);
> > 
> > You don't seem to have the link frequency control. I suppose you should, as
> > well as have it in DT.
> > 
> > <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks>
> > 
> 
> Are you referring to V4L2_CID_LINK_FREQ? I defined it above as a read only
> control as it is fixed for this sensor.

Ah, it was of course the first control.

Anyway, in general case, the frequencies that can be used on the board
should come from DT, as using other frequencies may interfere with other
devices in the same system.

...

> > > +	sensor->slave_mode = of_property_read_bool(dev->of_node, "slave-mode");
> > 
> > What does this actually do?
> > 
> > On parallel bus the slave mode tells the synchronisation signals originate
> > from the receiver. On CSI-2 there are no specific synchronisation signals.
> > 
> 
> We discussed about this on v1 a while ago:
> 
> https://lore.kernel.org/all/c610a2c9-31b1-1950-00fa-a6b3fd3517a1@foss.st.com/
> 
> Tell me if there is any changes to be made, or maybe some documentation to
> clarify?

Ah, yes, I hadn't had time to read that reply yet. :-(

I think we should have a generic solution for this. You may not have two
similar kinds of sensors you're synchronising this way. One option could be
to drop this functionality from the driver now to get it in sooner.

Something to consider:

- how to convey where the signal comes from (phandle),

- capture start signal polarity,

- whether it's input/output and

- pull-down / pull-up configuration?

These are similar to what GPIOs have. Hardware support may vary of course.

-- 
Kind regards,

Sakari Ailus