Re: [RFC PATCH 01/24] V4L fixes

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Ivaylo,

On Mon, Apr 25, 2016 at 07:32:50PM +0300, Ivaylo Dimitrov wrote:
> Hi,
> 
> On 25.04.2016 16:25, Sakari Ailus wrote:
> >Hi Ivaylo,
> >
> >Thanks for the set!
> >
> >On Mon, Apr 25, 2016 at 12:08:01AM +0300, Ivaylo Dimitrov wrote:
> >>From: "Tuukka.O Toivonen" <tuukka.o.toivonen@nokia.com>
> >>
> >>Squashed from the following upstream commits:
> >>
> >>V4L: Create control class for sensor mode
> >>V4L: add ad5820 focus specific custom controls
> >>V4L: add V4L2_CID_TEST_PATTERN
> >>V4L: Add V4L2_CID_MODE_OPSYSCLOCK for reading output system clock
> >>
> >>Signed-off-by: Tuukka Toivonen <tuukka.o.toivonen@nokia.com>
> >>Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> >>---
> >>  include/uapi/linux/v4l2-controls.h | 17 +++++++++++++++++
> >>  1 file changed, 17 insertions(+)
> >>
> >>diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>index b6a357a..23011cc 100644
> >>--- a/include/uapi/linux/v4l2-controls.h
> >>+++ b/include/uapi/linux/v4l2-controls.h
> >>@@ -62,6 +62,7 @@
> >>  #define V4L2_CTRL_CLASS_FM_RX		0x00a10000	/* FM Receiver controls */
> >>  #define V4L2_CTRL_CLASS_RF_TUNER	0x00a20000	/* RF tuner controls */
> >>  #define V4L2_CTRL_CLASS_DETECT		0x00a30000	/* Detection controls */
> >>+#define V4L2_CTRL_CLASS_MODE		0x00a40000	/* Sensor mode information */
> >>
> >>  /* User-class control IDs */
> >>
> >>@@ -974,4 +975,20 @@ enum v4l2_detect_md_mode {
> >>  #define V4L2_CID_DETECT_MD_THRESHOLD_GRID	(V4L2_CID_DETECT_CLASS_BASE + 3)
> >>  #define V4L2_CID_DETECT_MD_REGION_GRID		(V4L2_CID_DETECT_CLASS_BASE + 4)
> >>
> >>+/* SMIA-type sensor information */
> >>+#define V4L2_CID_MODE_CLASS_BASE		(V4L2_CTRL_CLASS_MODE | 0x900)
> >>+#define V4L2_CID_MODE_CLASS			(V4L2_CTRL_CLASS_MODE | 1)
> >>+#define V4L2_CID_MODE_FRAME_WIDTH		(V4L2_CID_MODE_CLASS_BASE+1)
> >>+#define V4L2_CID_MODE_FRAME_HEIGHT		(V4L2_CID_MODE_CLASS_BASE+2)
> >>+#define V4L2_CID_MODE_VISIBLE_WIDTH		(V4L2_CID_MODE_CLASS_BASE+3)
> >>+#define V4L2_CID_MODE_VISIBLE_HEIGHT		(V4L2_CID_MODE_CLASS_BASE+4)
> >
> >The interface here pre-dates the selection API. The frame width and height
> >is today conveyed to the bridge driver by the smiapp driver in the scaler
> >(or binner in case of the lack of the scaler) sub-device's source pad
> >format.
> >
> >(While that's the current interface, I'm not entirely happy with it; it
> >requires more sub-devices to be created for the same I2C device). I think in
> >this case you'd need one more to properly control the sensor.
> >
> 
> By looking at the code, it seems those are read-only, so I don't think it
> make sense to add a binner sub-device with read-only controls.

Well, I can't disagree with that. However, I think a number of other drivers
would benefit from doing this properly --- the V4L2 selection API which
defines a strict order for the processing steps does not suit to this use
case very well. You could use the crop selection rectangle for now, albeit
that doesn't fully express what the sensor does, or use private controls. We
could then later replace this with something better.

> 
> >>+#define V4L2_CID_MODE_PIXELCLOCK		(V4L2_CID_MODE_CLASS_BASE+5)
> >>+#define V4L2_CID_MODE_SENSITIVITY		(V4L2_CID_MODE_CLASS_BASE+6)
> >>+#define V4L2_CID_MODE_OPSYSCLOCK		(V4L2_CID_MODE_CLASS_BASE+7)
> >
> >While I don't remember quite what the sensitivity value was about (it could
> >be e.g. binning summing / averaging), the other two values are passed to
> >(and also controlled by) the user space using controls. There are
> >V4L2_CID_PIXEL_RATE and V4L2_CID_LINK_FREQ or such.
> >
> 
> I've already made a change so V4L2_CID_PIXEL_RATE is used in et8ek8 driver (https://github.com/freemangordon/linux-n900/commit/54433e50451b4ed6cc6e3b25d149c5cacd97e438),
> but V4L2_CID_MODE_PIXELCLOCK is used in smiapp driver, which seems to expose
> its own controls. Not sure those are needed though. And if, what for. I hope
> you know better than me.

It's needed to tell the sensor timings to the user space. The camera control
algorithms need that information.

> 
> I guess V4L2_CID_MODE_OPSYSCLOCK can be easily transformed to
> V4L2_CID_LINK_FREQ in the same way as V4L2_CID_MODE_PIXELCLOCK.

Yes.

> 
> >>+
> >>+/* Control IDs specific to the AD5820 driver as defined by V4L2 */
> >>+#define V4L2_CID_FOCUS_AD5820_BASE 		(V4L2_CTRL_CLASS_CAMERA | 0x10af)
> >>+#define V4L2_CID_FOCUS_AD5820_RAMP_TIME		(V4L2_CID_FOCUS_AD5820_BASE+0)
> >>+#define V4L2_CID_FOCUS_AD5820_RAMP_MODE		(V4L2_CID_FOCUS_AD5820_BASE+1)
> >
> >The ad5820 driver isn't in upstream at the moment. It should be investigated
> >whether there is a possibility to have standard V4L2 controls for lens
> >devices, or whether device specific controls should be implemented instead.
> >Device specific controls are a safe choice in this case, but they should be
> >in a separate patch, possibly one that would also include the lens driver
> >itself.
> >
> 
> Yeah, I sent the whole patch for the sake of not losing the history too
> much.

There's still work to be done but I'm very happy to see that you and a few
others are contributing. :-)

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk