Hi Javier,
> [snip]
>
>> > diff --git a/drivers/media/video/mt9p031.c
>> > b/drivers/media/video/mt9p031.c new file mode 100644
>> > index 0000000..e406b64
>> > --- /dev/null
>> > +++ b/drivers/media/video/mt9p031.c
>
> [snip]
>
>> > +}
>> > +
>> > +static int mt9p031_power_on(struct mt9p031 *mt9p031)
>> > +{
>> > + int ret;
>> > +
>> > + /* turn on VDD_IO */
>> > + ret = regulator_enable(mt9p031->reg_2v8);
>> > + if (ret) {
>> > + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
>>
>> dev_err()
>>
>> > + return ret;
>> > + }
>> > + if (mt9p031->pdata->set_xclk)
>> > + mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
>
> Can you make 54000000 a #define at the beginning of the file ?
>
> You should soft-reset the chip here by calling mt9p031_reset().If I do this, I would be force to cache some registers and restart
them. I've tried to do this but I don't know what is failing that
there are some artifacts consisting on horizontal black lines in the
image.
You need to cache registers anyway, as the chip will be reset to default
values by the core power cycling. And as I'm writing those lines I realize
that you don't power cycle reg_1v8. This needs to be done to save power.
Please, let me push this to mainline without this feature as a first
step, since I'll have to spend some assigned to another project.
Power handling is an important feature. I don't think the driver is ready
without it.
[snip]
>> > + */
>> > +static int mt9p031_video_probe(struct i2c_client *client)
>> > +{
>> > + s32 data;
>> > + int ret;
>> > +
>> > + /* Read out the chip version register */
>> > + data = reg_read(client, MT9P031_CHIP_VERSION);
>> > + if (data != MT9P031_CHIP_VERSION_VALUE) {
>> > + dev_err(&client->dev,
>> > + "No MT9P031 chip detected, register read %x\n",
>> > data); + return -ENODEV;
>> > + }
>> > +
>> > + dev_info(&client->dev, "Detected a MT9P031 chip ID %x\n", data);
>> > +
>> > + ret = mt9p031_reset(client);
>> > + if (ret < 0)
>> > + dev_err(&client->dev, "Failed to initialise the
>> > camera\n");
>
> If you move the soft-reset operation to mt9p031_power_on(), you don't
> need to call it here.The reason for this is the same as before. I haven't still been able
to success on restarting registers and getting everything to work
fine.
It would be great if you allowed me to push this as it is as an
intermediate step.
Sorry, but I'd like to see power management properly implemented before the
driver hits mainline. Other less important features (such as exposure/gain
controls for instance) can be missing, but proper power management is
important.
[snip]
>> > + mt9p031->rect.width = MT9P031_MAX_WIDTH;
>> > + mt9p031->rect.height = MT9P031_MAX_HEIGHT;
>> > +
>> > + mt9p031->format.code = V4L2_MBUS_FMT_SGRBG12_1X12;
>> > +
>> > + mt9p031->format.width = MT9P031_MAX_WIDTH;
>> > + mt9p031->format.height = MT9P031_MAX_HEIGHT;
>> > + mt9p031->format.field = V4L2_FIELD_NONE;
>> > + mt9p031->format.colorspace = V4L2_COLORSPACE_SRGB;
>> > +
>> > + mt9p031->xskip = 1;
>> > + mt9p031->yskip = 1;
>> > +
>> > + mt9p031->reg_1v8 = regulator_get(NULL, "cam_1v8");
>> > + if (IS_ERR(mt9p031->reg_1v8)) {
>> > + ret = PTR_ERR(mt9p031->reg_1v8);
>> > + pr_err("Failed 1.8v regulator: %d\n", ret);
>>
>> dev_err()
>>
>> > + goto e1v8;
>> > + }
>
> The driver can be used with boards where either or both of the 1.8V and
> 2.8V supplies are always on, thus not connected to any regulator. I'm
> not sure how that's usually handled, if board code should define an
> "always-on" power supply, or if the driver shouldn't fail when no
> regulator is present. In any case, this must be handled.I think board code should define an "always-on" power supply.
Fine with me. How is that done BTW ?