Hi Javier,
On Tuesday 24 May 2011 10:31:46 javier Martin wrote:
> On 23 May 2011 11:03, Laurent Pinchart wrote:
> > On Saturday 21 May 2011 17:29:18 Guennadi Liakhovetski wrote:
> >> On Fri, 20 May 2011, Javier Martin wrote:
> > [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 ?
> >> > +
> >> > + mt9p031->reg_2v8 = regulator_get(NULL, "cam_2v8");
> >> > + if (IS_ERR(mt9p031->reg_2v8)) {
> >> > + ret = PTR_ERR(mt9p031->reg_2v8);
> >> > + pr_err("Failed 2.8v regulator: %d\n", ret);
> >>
> >> ditto
> >>
> >> > + goto e2v8;
> >> > + }
> >> > + /* turn on core */
> >> > + ret = regulator_enable(mt9p031->reg_1v8);
> >> > + if (ret) {
> >> > + pr_err("Failed to enable 1.8v regulator: %d\n", ret);
> >>
> >> ditto
> >>
> >> > + goto e1v8en;
> >> > + }
> >> > + return 0;
> >
> > Why do you leave core power on at the end of probe() ? You should only
> > turn it on when needed.
>
> Just as I said, because restarting registers does not work yet.
--
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
