On Wed, Dec 14, 2011 at 02:49:05PM +0100, Laurent Pinchart wrote:
> Hi Martin,
>
> On Wednesday 14 December 2011 08:14:01 martin@xxxxxxxxxxxxxxxxxxxxxx wrote:
> > On Wed, Dec 14, 2011 at 02:55:31AM +0100, Marek Vasut wrote:
> > > > The MT9M032 is a parallel 1.6MP sensor from Micron controlled through
> > > > I2C.
> > > >
> > > > The driver creates a V4L2 subdevice. It currently supports cropping,
> > > > gain, exposure and v/h flipping controls in monochrome mode with an
> > > > external pixel clock.
> > > >
> > > > Signed-off-by: Martin Hostettler <martin@xxxxxxxxxxxxxxxxxxxxxx>
> > > > ---
>
> [snip]
>
> > > > diff --git a/drivers/media/video/mt9m032.c
> > > > b/drivers/media/video/mt9m032.c new file mode 100644
> > > > index 0000000..b4159c7
> > > > --- /dev/null
> > > > +++ b/drivers/media/video/mt9m032.c
> > > > @@ -0,0 +1,819 @@
>
> [snip]
>
> > > > +static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
> > > > +{
> > > > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > > > + s32 data = i2c_smbus_read_word_data(client, reg);
> > > > +
> > > > + return data < 0 ? data : swab16(data);
> > >
> > > Uhm ... why ?
> >
> > error codes are not swab16-ed, data values are. Hardware needs swapping of
> > data values.
>
> You can use i2c_smbus_read_word_swapped() and i2c_smbus_write_word_swapped().
Yes, i didn't notice that they have been added. Nicer to use them.
>
> > > > +}
> > > > +
> > > > +static int mt9m032_write_reg(struct mt9m032 *sensor, u8 reg,
> > > > + const u16 data)
> > > > +{
> > > > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > > > +
> > > > + return i2c_smbus_write_word_data(client, reg, swab16(data));
> > > > +}
>
> [snip]
>
> > > > +static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool
> > > > hflip) +{
> > > > + int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
> > >
> > > !!(bool) ? hmm ...
> >
> > a true bool can be any value except 0. !!bool is guaranteed to be either
> > 0 or 1 and this suitable for use in bitshifting to set a specific bit.
>
> I'd use
>
> int reg_val = (vflip ? MT9M032_READ_MODE2_VFLIP : 0)
> | (hflip ? MT9M032_READ_MODE2_HFLIP : 0)
> ...
>
> but that might just be me.
I like shifts more, easy to check in the datasheet.
>
> BTW, what about using fixed-size types for register values ? u16 would make
> sense here.
I don't think i would make a difference here.
>
> > > > + | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
> > > > + | MT9M032_READ_MODE2_ROW_BLC
> > > > + | 0x0007;
> > > > +
> > > > + return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
> > > > +}
>
> [snip]
>
> > > > +static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
> > > > +{
> > > > + int digital_gain_val; /* in 1/8th (0..127) */
> > > > + int analog_mul; /* 0 or 1 */
> > > > + int analog_gain_val; /* in 1/16th. (0..63) */
> > > > + u16 reg_val;
> > > > +
> > > > + digital_gain_val = 51; /* from setup example */
> > > > +
> > > > + if (val < 63) {
> > > > + analog_mul = 0;
> > > > + analog_gain_val = val;
> > > > + } else {
> > > > + analog_mul = 1;
> > > > + analog_gain_val = val / 2;
> > > > + }
> > > > +
> > > > + /* a_gain = (1+analog_mul) + (analog_gain_val+1)/16 */
> > > > + /* overall_gain = a_gain * (1 + digital_gain_val / 8) */
> > > > +
> > > > + reg_val = (digital_gain_val & MT9M032_GAIN_DIGITAL_MASK) <<
> > > > MT9M032_GAIN_DIGITAL_SHIFT + | (analog_mul & 1) <<
> > > > MT9M032_GAIN_AMUL_SHIFT
> > > > + | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
> > > > +
> > > > + return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
> > > > +}
>
> Lot's of Aptina sensors have a similar gain setup with two or three stages. Do
> you think it would be worth it creating a function that could be shared
> between them ?
I don't have time to work on such an extension in a meaningful way.
I'm not sure what's the best way to use this kind of staged gain hardware
best anyway.
>
> > > > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > > > +{
> > > > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > > > + u16 reg_pll1;
> > > > + unsigned int pre_div;
> > > > + int res, ret;
> > > > +
> > > > + /* TODO: also support other pre-div values */
>
> I might already have mentioned this, but wouldn't it be time to work a on real
> PLL setup code that compute the pre-divisor, multiplier and output divisor
> dynamically from the input and output clock frequencies ?
I'm not sure what the implications for quality and stability of such a
generic setup would be. My gut feeling is most users go with known working
hardcoded values.
Also in the datasheet i have access to, this is totally underdocumented.
>
> > > > + if (pdata->pll_pre_div != 6) {
> > > > + dev_warn(to_dev(sensor),
> > > > + "Unsupported PLL pre-divisor value %u, using default
> > >
> > > 6\n",
> > >
> > > > + pdata->pll_pre_div);
> > > > + }
> > > > + pre_div = 6;
> > > > +
> > > > + sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> > > > + (pre_div * pdata->pll_out_div);
> > > > +
> > > > + reg_pll1 = ((pdata->pll_out_div - 1) &
> > > > MT9M032_PLL_CONFIG1_OUTDIV_MASK) + | pdata->pll_mul <<
> > > > MT9M032_PLL_CONFIG1_MUL_SHIFT;
> > > > +
> > > > + ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
> > > > + if (!ret)
> > > > + ret = mt9m032_write_reg(sensor, 0x10, 0x53); /* Select PLL as
> > >
> > > clock
> > >
> > > > source */ +
> > >
> > > urm ... magic ... black magic ...
> >
> > yes, indeed. But i don't have any more information.
> > The datasheet nicely states "reserved" and in the bringup section
> > "poke in these magical values, please". :/ Not even a register name.
>
> What version of the datasheet do you have ? According to the MT9P031
Rev. B 10/07 EN
> datasheet, register 0x10 is called PLL Control and bits 0 and 1 are described
> as
>
> 1 Use PLL
> When set, use the PLL output as the system clock. When clear, use EXTCLK as
> the system clock.
> 0 Power PLL
> When set, the PLL is powered. When clear, it is not powered.
>
> The other bits are not documented. The mt9p031 driver has
>
> #define MT9P031_PLL_CONTROL 0x10
> #define MT9P031_PLL_CONTROL_PWROFF 0x0050
> #define MT9P031_PLL_CONTROL_PWRON 0x0051
> #define MT9P031_PLL_CONTROL_USEPLL 0x0052
>
> which you could reuse.
Yes, it seems close enough. But i'm a bit uneasy about mixing from
different datasheets. Would i keep the original names so everybody can see
that it's from a data sheet of a similar but different hardware?
It would be
MT9P031_PLL_CONTROL_PWRON | MT9P031_PLL_CONTROL_USEPLL
then witch resolves to a slightly odd 0x0051 | 0x0052.
>
> > > > + if (!ret)
> > > > + ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
> > > > + /* more reserved, Continuous */
> > > > + /* Master Mode */
> > > > + if (!ret)
> > > > + res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
> > > > +
> > > > + if (!ret)
> > > > + ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
> > > > + /* Set 14-bit mode, select 7 divider */
> > > > +
> > > > + return ret;
> > > > +}
>
>
> [snip]
>
> > > > + /*
> > > > +* This driver was developed with a camera module with seperate external
> > > > +* pix clock. For setups which use the clock from the camera interface
> > > > +* the code will need to be extended with the appropriate platform
> > > > +* callback to setup the clock.
> > > > + */
> > > > + chip_version = mt9m032_read_reg(sensor, MT9M032_CHIP_VERSION);
> > > > + if (0x1402 == chip_version) {
> > >
> > > So I suspect this can also cast fireballs ? Also, why do you use Yoda
> > > conditions here ? Please run checkpatch.pl on this too just to be sure.
> >
> > No fireball was visible when looking at the hardware.
>
> #define MT9M032_CHIP_VERSION_VALUE 0x1402
>
> is an option (not sure if it prevents fireballs though).
I don't think it will prevent any burns or fires.
But i'll add it anyway, it's clear enough.
>
> > > > + dev_info(&client->dev, "mt9m032: detected sensor.\n");
> > > > + } else {
> > > > + dev_warn(&client->dev, "mt9m032: error: detected unsupported
> > > chip version 0x%x\n",
> > > > + chip_version);
> > > > + ret = -ENODEV;
> > > > + goto free_sensor;
> > > > + }
>
> [snip]
>
> > > > +static int __init mt9m032_init(void)
> > > > +{
> > > > + int rval;
> > > > +
> > > > + rval = i2c_add_driver(&mt9m032_i2c_driver);
> > > > + if (rval)
> > > > + pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
> > > > +
> > > > + return rval;
> > > > +}
> > > > +
> > > > +static void mt9m032_exit(void)
> > > > +{
> > > > + i2c_del_driver(&mt9m032_i2c_driver);
> > > > +}
> > > > +
> > > > +module_init(mt9m032_init);
> > > > +module_exit(mt9m032_exit);
>
> v3.3 will have a very handy module_i2c_driver() macro. See
> https://lkml.org/lkml/2011/11/17/36
If you think i should do that i can. Looses a bit verboseness in case of
errors, but i don't think it matters.
For me both ways are ok.
>
> > > > +MODULE_AUTHOR("Martin Hostettler");
> > > > +MODULE_DESCRIPTION("MT9M032 camera sensor driver");
> > > > +MODULE_LICENSE("GPL v2");
>
> --
> 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
