Hi Sakari,
Thanks for the review.
On Wednesday 31 August 2011 20:23:33 Sakari Ailus wrote:
> On Wed, Aug 31, 2011 at 02:24:12PM +0200, Laurent Pinchart wrote:
> > The MT9T001 is a parallel 3MP sensor from Aptina (formerly Micron)
> > controlled through I2C.
> >
> > The driver creates a V4L2 subdevice. It currently supports binning and
> > cropping, and the gain, exposure, test pattern and black level controls.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
[snip]
> > diff --git a/drivers/media/video/mt9t001.c
> > b/drivers/media/video/mt9t001.c new file mode 100644
> > index 0000000..32ab217
> > --- /dev/null
> > +++ b/drivers/media/video/mt9t001.c
> > @@ -0,0 +1,788 @@
[snip]
> > +/*
> > + * mt9m001 i2c address 0x5d
>
> Is it also that for mt8t001?
Do you mean mt9t001 ? :-) The comment isn't really useful, I've removed it.
> > + */
[snip]
> > +static struct mt9t001 *to_mt9t001(struct v4l2_subdev *sd)
> > +{
> > + return container_of(sd, struct mt9t001, subdev);
> > +}
>
> I guess you could add inline here, or define it as a macro.
I've added inline.
> > +static int mt9t001_read(struct i2c_client *client, const u8 reg)
> > +{
> > + s32 data = i2c_smbus_read_word_data(client, reg);
> > + return data < 0 ? data : swab16(data);
>
> Is the swab16 correct here? What about be16_to_cpu()?
Fixed.
> > +}
> > +
> > +static int mt9t001_write(struct i2c_client *client, const u8 reg,
> > + const u16 data)
> > +{
> > + return i2c_smbus_write_word_data(client, reg, swab16(data));
>
> Ditto.
>
> > +}
[snip]
> > +static int mt9t001_s_stream(struct v4l2_subdev *subdev, int enable)
> > +{
> > + struct mt9t001 *mt9t001 = to_mt9t001(subdev);
> > +
> > + /* Switch to master "normal" mode or stop sensor readout */
> > + return mt9t001_set_output_control(mt9t001,
> > + enable ? 0 : MT9T001_OUTPUT_CONTROL_CHIP_ENABLE,
> > + enable ? MT9T001_OUTPUT_CONTROL_CHIP_ENABLE : 0);
>
> I wonder if an if would be better here. You also could change the
> mt9t001_set_output_control() to take in the number of the bit and whether
> you enable or disable it.
I've added more code to s_stream, the code looks cleaner now.
> > +}
[snip]
> > +#define V4L2_CID_TEST_PATTERN (V4L2_CID_USER_BASE | 0x1001)
>
> Thest pattern is something that almost every sensor have.
>
> > +#define V4L2_CID_GAIN_RED (V4L2_CTRL_CLASS_CAMERA | 0x1001)
> > +#define V4L2_CID_GAIN_GREEN1 (V4L2_CTRL_CLASS_CAMERA | 0x1002)
> > +#define V4L2_CID_GAIN_GREEN2 (V4L2_CTRL_CLASS_CAMERA | 0x1003)
>
> The greens are usually not numbered but have either blue/red subscript
> based on the colour of the adjacent pixel as far as I understand. What
> about calling them GREEN_RED or GREEN_R (and same for blue)?
Good point. I've fixed that.
> Also these are quite low level controls as opposed to the other higher
> level controls in this class. I wonder if creating a separate class for
> them would make sense. We'll need a new class for the hblank/vblank
> controls anyway. I might call it "sensor".
>
> These controls could be also standardised.
I agree.
A "sensor" control class might make sense for these 5 controls, but they can
also be useful for non-sensor hardware (for instance with an analog pixel
decoder).
> > +#define V4L2_CID_GAIN_BLUE (V4L2_CTRL_CLASS_CAMERA | 0x1004)
> > +
> > +static int mt9t001_gain_data(s32 *gain)
> > +{
> > + /* Gain is controlled by 2 analog stages and a digital stage. Valid
> > + * values for the 3 stages are
> > + *
> > + * Stage Min Max Step
> > + * ------------------------------------------
> > + * First analog stage x1 x2 1
> > + * Second analog stage x1 x4 0.125
> > + * Digital stage x1 x16 0.125
> > + *
> > + * To minimize noise, the gain stages should be used in the second
> > + * analog stage, first analog stage, digital stage order. Gain from a
> > + * previous stage should be pushed to its maximum value before the next
> > + * stage is used.
> > + */
> > + if (*gain <= 32)
> > + return *gain;
> > +
> > + if (*gain <= 64) {
> > + *gain &= ~1;
> > + return (1 << 6) | (*gain >> 1);
> > + }
> > +
> > + *gain &= ~7;
> > + return ((*gain - 64) << 5) | (1 << 6) | 32;
> > +}
>
> This one looks very similar to another Aptina sensor driver. My comment
> back then was that the analog and digital gain should be separate controls
> as the user typically would e.g. want to know (s)he's using digital gain
> instead of analog one.
>
> What about implementing this?
>
> It's a good question whether we need one or two new controls. If the answer
> is two, then how do they relate to the existing control?
I'm not too sure about this. If an application needs that much control over
the hardware, wouldn't it be hardware-specific anyway, and know about control
ranges ? The mt9t001 actually has 3 gain stages, so one might even argue that
we should expose 3 gain controls :-)
> > +static int mt9t001_ctrl_freeze(struct mt9t001 *mt9t001, bool freeze)
> > +{
> > + return mt9t001_set_output_control(mt9t001,
> > + freeze ? 0 : MT9T001_OUTPUT_CONTROL_SYNC,
> > + freeze ? MT9T001_OUTPUT_CONTROL_SYNC : 0);
> > +}
> > +
> > +static int mt9t001_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + static const u8 gains[4] = {
> > + MT9T001_RED_GAIN, MT9T001_GREEN1_GAIN,
> > + MT9T001_GREEN2_GAIN, MT9T001_BLUE_GAIN
> > + };
> > +
> > + struct mt9t001 *mt9t001 =
> > + container_of(ctrl->handler, struct mt9t001, ctrls);
> > + struct i2c_client *client = v4l2_get_subdevdata(&mt9t001->subdev);
> > + unsigned int count;
> > + unsigned int i;
> > + int data;
> > + int ret;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_GAIN_RED:
> > + case V4L2_CID_GAIN_GREEN1:
> > + case V4L2_CID_GAIN_GREEN2:
> > + case V4L2_CID_GAIN_BLUE:
> > +
> > + /* Disable control updates if more than one control has changed
> > + * in the cluster.
> > + */
> > + for (i = 0, count = 0; i < 4; ++i) {
> > + struct v4l2_ctrl *gain = mt9t001->gains[i];
> > +
> > + if (gain->val != gain->cur.val)
> > + count++;
> > + }
> > +
> > + if (count > 1) {
> > + ret = mt9t001_ctrl_freeze(mt9t001, true);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + /* Update the gain controls. */
> > + for (i = 0; i < 4; ++i) {
> > + struct v4l2_ctrl *gain = mt9t001->gains[i];
> > +
> > + if (gain->val == gain->cur.val)
> > + continue;
> > +
> > + data = mt9t001_gain_data(&gain->val);
> > + ret = mt9t001_write(client, gains[i], data);
> > + if (ret < 0) {
> > + mt9t001_ctrl_freeze(mt9t001, false);
> > + return ret;
> > + }
> > + }
> > +
> > + /* Enable control updates. */
> > + if (count > 1) {
> > + ret = mt9t001_ctrl_freeze(mt9t001, false);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + break;
> > +
> > + case V4L2_CID_EXPOSURE:
> > + ret = mt9t001_write(client, MT9T001_SHUTTER_WIDTH_LOW,
> > + ctrl->val & 0xffff);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return mt9t001_write(client, MT9T001_SHUTTER_WIDTH_HIGH,
> > + ctrl->val >> 16);
> > + case V4L2_CID_TEST_PATTERN:
> > + ret = mt9t001_set_output_control(mt9t001,
> > + ctrl->val ? 0 : MT9T001_OUTPUT_CONTROL_TEST_DATA,
> > + ctrl->val ? MT9T001_OUTPUT_CONTROL_TEST_DATA : 0);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return mt9t001_write(client, MT9T001_TEST_DATA, ctrl->val << 2);
> > +
>
> > + case V4L2_CID_BLACK_LEVEL:
> Does this do automatic black level calibration? If so, I'd call this
> BLACK_LEVEL_CALIBRATE instead.
V4L2_CID_BLACK_LEVEL is marked as deprecated, so a new control indeed makes
sense. It should probably belong to the sensor class.
> > + return mt9t001_write(client, MT9T001_BLACK_LEVEL_CALIBRATION,
> > + MT9T001_BLACK_LEVEL_RECALCULATE);
> > + }
> > +
> > + return 0;
> > +}
[snip]
> Just a general comment. How much does this sensor share with other Aptina
> sensors? Could one of the existing Aptina sensor drivers such as the mt9v032
> used to drive this sensor?
Not all features are available on both sensors, but that shouldn't be too much
of an issue.
The biggest problem is that there are many subtle differences. For instance
#define MT9T001_ROW_START 0x01
#define MT9T001_COLUMN_START 0x02
#define MT9V032_COLUMN_START 0x01
#define MT9V032_ROW_START 0x02
is quite easy to miss. The MT9T001 needs to be programmed with window
width/height minus one, while the MT9V032 needs to be programmed with window
width/height. Horizontal and vertical binning are configured in two separate
registers in the MT9T001 and in a single register in the MT9V032. The list
goes on.
It might be possible to support several chips with a single driver, but I
think we need a couple more before we can identify the proper recurring
patterns.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
