Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Please, do not remove anyone from the CC list. On 25 May 2011 05:45, Chris Rodley wrote: > Hi, > > Have upgraded the driver to Javier's latest RFC driver. > Still having problems viewing output. > > Setting up with: > # media-ctl -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1], "OMAP3 ISP > CCDC":1->"OMAP3 ISP CCDC output":0[1]' > Resetting all links to inactive > Setting up link 16:0 -> 5:0 [1] > Setting up link 5:1 -> 6:0 [1] > > # media-ctl -f '"mt9p031 2-0048":0[SGRBG12 320x240], "OMAP3 ISP > CCDC":0[SGRBG8 320x240], "OMAP3 ISP CCDC":1[SGRBG8 320x240]' > Setting up format SGRBG12 320x240 on pad mt9p031 2-0048/0 > Format set: SGRB > Setting up format SGRBG12 320x240 on pad OMAP3 ISP CCDC/0 > Format set: SGRBG12 320x240 > Setting up format SGRBG8 320x240 on pad OMAP3 ISP CCDC/0 > Format set: SGRBG8 320x240 > Setting up format SGRBG8 320x240 on pad OMAP3 ISP CCDC/1 > Format set: SGRBG8 320x240 > > Then: > # yavta -f SGRBG8 -s 320x240 -n 4 --capture=100 -F /dev/video2 > Device /dev/video2 opened. > Device `OMAP3 ISP CCDC output' on `media' is a video capture device. > Video format set: width: 320 height: 240 buffer size: 76800 > Video format: GRBG (47425247) 320x240 > 4 buffers requested. > length: 76800 offset: 0 > Buffer 0 mapped at address 0x4006c000. > length: 76800 offset: 77824 > Buffer 1 mapped at address 0x40222000. > length: 76800 offset: 155648 > Buffer 2 mapped at address 0x4025e000. > length: 76800 offset: 233472 > Buffer 3 mapped at address 0x402f. > > After this it hangs and will exit on 'ctrl c': > omap3isp omap3isp: CCDC stop timeout! > > Any ideas what is causing this problem? > No idea, it works for me. Note you have to apply RFC + PATCH v2 2/2. Please, double check. Also, if you have problems with last RFC patch you should answer RFC mail. Not this one. Thank you. -- Javier Martin Vista Silicon S.L. CDTUC - FASE C - Oficina S-345 Avda de los Castros s/n 39005- Santander. Cantabria. Spain +34 942 25 32 60 www.vista-silicon.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Hi, Have upgraded the driver to Javier's latest RFC driver. Still having problems viewing output. Setting up with: # media-ctl -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1], "OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]' Resetting all links to inactive Setting up link 16:0 -> 5:0 [1] Setting up link 5:1 -> 6:0 [1] # media-ctl -f '"mt9p031 2-0048":0[SGRBG12 320x240], "OMAP3 ISP CCDC":0[SGRBG8 320x240], "OMAP3 ISP CCDC":1[SGRBG8 320x240]' Setting up format SGRBG12 320x240 on pad mt9p031 2-0048/0 Format set: SGRB Setting up format SGRBG12 320x240 on pad OMAP3 ISP CCDC/0 Format set: SGRBG12 320x240 Setting up format SGRBG8 320x240 on pad OMAP3 ISP CCDC/0 Format set: SGRBG8 320x240 Setting up format SGRBG8 320x240 on pad OMAP3 ISP CCDC/1 Format set: SGRBG8 320x240 Then: # yavta -f SGRBG8 -s 320x240 -n 4 --capture=100 -F /dev/video2 Device /dev/video2 opened. Device `OMAP3 ISP CCDC output' on `media' is a video capture device. Video format set: width: 320 height: 240 buffer size: 76800 Video format: GRBG (47425247) 320x240 4 buffers requested. length: 76800 offset: 0 Buffer 0 mapped at address 0x4006c000. length: 76800 offset: 77824 Buffer 1 mapped at address 0x40222000. length: 76800 offset: 155648 Buffer 2 mapped at address 0x4025e000. length: 76800 offset: 233472 Buffer 3 mapped at address 0x402f. After this it hangs and will exit on 'ctrl c': omap3isp omap3isp: CCDC stop timeout! Any ideas what is causing this problem? This may be useful also: # media-ctl -p Opening media device /dev/media0 Enumerating entities Found 16 entities Enumerating pads and links Device topology - entity 1: OMAP3 ISP CCP2 (2 pads, 2 links) type V4L2 subdev subtype Unknown device node name /dev/v4l-subdev0 pad0: Input [SGRBG10 4096x4096] <- 'OMAP3 ISP CCP2 input':pad0 [] pad1: Output [SGRBG10 4096x4096] -> 'OMAP3 ISP CCDC':pad0 [] - entity 2: OMAP3 ISP CCP2 input (1 pad, 1 link) type Node subtype V4L device node name /dev/video0 pad0: Output -> 'OMAP3 ISP CCP2':pad0 [] - entity 3: OMAP3 ISP CSI2a (2 pads, 2 links) type V4L2 subdev subtype Unknown device node name /dev/v4l-subdev1 pad0: Input [SGRBG10 4096x4096] pad1: Output [SGRBG10 4096x4096] -> 'OMAP3 ISP CSI2a output':pad0 [] -> 'OMAP3 ISP CCDC':pad0 [] - entity 4: OMAP3 ISP CSI2a output (1 pad, 1 link) type Node subtype V4L device node name /dev/video1 pad0: Input <- 'OMAP3 ISP CSI2a':pad1 [] - entity 5: OMAP3 ISP CCDC (3 pads, 9 links) type V4L2 subdev subtype Unknown device node name /dev/v4l-subdev2 pad0: Input [SGRBG8 320x240] <- 'OMAP3 ISP CCP2':pad1 [] <- 'OMAP3 ISP CSI2a':pad1 [] <- 'mt9p031 2-0048':pad0 [ACTIVE] pad1: Output [SGRBG8 320x240] -> 'OMAP3 ISP CCDC output':pad0 [ACTIVE] -> 'OMAP3 ISP resizer':pad0 [] pad2: Output [SGRBG8 320x239] -> 'OMAP3 ISP preview':pad0 [] -> 'OMAP3 ISP AEWB':pad0 [IMMUTABLE,ACTIVE] -> 'OMAP3 ISP AF':pad0 [IMMUTABLE,ACTIVE] -> 'OMAP3 ISP histogram':pad0 [IMMUTABLE,ACTIVE] - entity 6: OMAP3 ISP CCDC output (1 pad, 1 link) type Node subtype V4L device node name /dev/video2 pad0: Input <- 'OMAP3 ISP CCDC':pad1 [ACTIVE] - entity 7: OMAP3 ISP preview (2 pads, 4 links) type V4L2 subdev subtype Unknown device node name /dev/v4l-subdev3 pad0: Input [SGRBG10 4096x4096] <- 'OMAP3 ISP CCDC':pad2 [] <- 'OMAP3 ISP preview input':pad0 [] pad1: Output [YUYV 2034x4088] -> 'OMAP3 ISP preview output':pad0 [] -> 'OMAP3 ISP resizer':pad0 [] - entity 8: OMAP3 ISP preview input (1 pad, 1 link) type Node subtype V4L device node name /dev/video3 pad0: Output -> 'OMAP3 ISP preview':pad0 [] - entity 9: OMAP3 ISP preview output (1 pad, 1 link) type Node subtype V4L device node name /dev/video4 pad0: Input <- 'OMAP3 ISP preview':pad1 [] - entity 10: OMAP3 ISP resizer (2 pads, 4 links) type V4L2 subdev subtype Unknown device node name /dev/v4l-subdev4 pad0: Input [YUYV 4095x4095 (0,6)/4094x4082] <- 'OMAP3 ISP CCDC':pad1 [] <- 'OMAP3 ISP preview':pad1 [] <- 'OMAP3 ISP resizer input':pad0 [] pad1: Output [YUYV 3312x4095] -> 'OMAP3 ISP resizer output':pad0 [] - entity 11: OMAP3 ISP resizer input (1 pad, 1 link) type Node subtype V4L device node name /dev/video5 pad0: Output -> 'OMAP3 ISP resizer':pad0 [] - entity
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Hi Javier,
On Tuesday 24 May 2011 10:56:22 javier Martin wrote:
> On 24 May 2011 10:39, Laurent Pinchart wrote:
> > 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 000..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, 5400);
> >> >
> >> > Can you make 5400 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.
>
> OK, I'll focus on this feature from now on. However, I can't guarantee
> that I won't be removed from the project in the process. If that
> happens I will send my current patches to the community and someone
> else will have to complete the job.
I understand. I could take over but I don't have an MT9P031 hardware :-S
> >> [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,
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On 24 May 2011 10:39, Laurent Pinchart
wrote:
> 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 000..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, 5400);
>> >
>> > Can you make 5400 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.
>
OK, I'll focus on this feature from now on. However, I can't guarantee
that I won't be removed from the project in the process. If that
happens I will send my current patches to the community and someone
else will have to complete the job.
>> [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 ?
>
You can use a fixed regulator for that purpose:
http://lxr.linux.no/#linux+v2.6.37.2/include/linux/regulator/fixed.h
--
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
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 000..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, 5400);
> >
> > Can you make 5400 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 whe
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Hi, Laurent, Guennadi,
thank you for your review. I've already fixed most of the issues.
On 23 May 2011 11:03, Laurent Pinchart
wrote:
> Hi Guennadi and Javier,
>
> 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 000..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, 5400);
>
> Can you make 5400 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.
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.
[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.
[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.
>> > +
>> > + 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.
--
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On 23/05/11 18:54, javier Martin wrote: > On 23 May 2011 05:01, Chris Rodley wrote: >> Error when using media-ctl as below with v2 mt9p031 driver from Javier and >> latest media-ctl version. >> Is there a patch I missed to add different formats - or maybe my command is >> wrong? > Please, try the following: > > ./media-ctl -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1], "OMAP3 > ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]' > ./media-ctl -f '"mt9p031 2-0048":0[SGRBG12 320x240], "OMAP3 ISP > CCDC":0[SGRBG8 320x240], "OMAP3 ISP CCDC":1[SGRBG8 320x240]' > > Thanks. Thanks Javier! That worked. The command below used to work but no output to stdout any more: ./yavta --stdout -f SGRBG8 -s 320x240 -n 4 --capture=100 -F `./media-ctl -e "OMAP3 ISP CCDC output"` | nc 10.1.1.99 3000 I have played around with this but have been unable to get a result. Something to do with the media-ctl part of the command? Thanks again! Chris -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On Mon, 23 May 2011, Laurent Pinchart wrote:
> > > +{
> > > + struct mt9p031 *mt9p031 = to_mt9p031(client);
> > > + int ret;
> > > +
> > > + /* Disable chip output, synchronous option update */
> > > + ret = reg_write(client, MT9P031_RST, MT9P031_RST_ENABLE);
> > > + if (ret < 0)
> > > + return -EIO;
> > > + ret = reg_write(client, MT9P031_RST, MT9P031_RST_DISABLE);
> > > + if (ret < 0)
> > > + return -EIO;
> > > + ret = mt9p031_set_output_control(mt9p031, MT9P031_OUTPUT_CONTROL_CEN,
> > > 0); + if (ret < 0)
> > > + return -EIO;
> > > + return 0;
> >
> > I think, a sequence like
> >
> > ret = fn();
> > if (!ret)
> > ret = fn();
> > if (!ret)
> > ret = fn();
> > return ret;
> >
> > is a better way to achieve the same.
>
> I disagree with you on that :-) I find code sequences that return as soon as
> an error occurs, using the main code path for the error-free case, easier to
> read. It can be a matter of personal taste though.
Whichever way, but it should be consistent, IMHO.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Hi Guennadi,
On Monday 23 May 2011 10:48:36 Guennadi Liakhovetski wrote:
> On Mon, 23 May 2011, javier Martin wrote:
> > On 21 May 2011 17:29, Guennadi Liakhovetski wrote:
> > > On Fri, 20 May 2011, Javier Martin wrote:
> > >> This driver adds basic support for Aptina mt9p031 sensor.
> > >>
> > >> Signed-off-by: Javier Martin
> > >> ---
> > >> drivers/media/video/Kconfig |8 +
> > >> drivers/media/video/Makefile |1 +
> > >> drivers/media/video/mt9p031.c | 751
> > >> + include/media/mt9p031.h
> > >> | 11 +
> > >> 4 files changed, 771 insertions(+), 0 deletions(-)
> > >> create mode 100644 drivers/media/video/mt9p031.c
> > >> create mode 100644 include/media/mt9p031.h
> > >>
> > >> diff --git a/drivers/media/video/mt9p031.c
> > >> b/drivers/media/video/mt9p031.c new file mode 100644
> > >> index 000..e406b64
> > >> --- /dev/null
> > >> +++ b/drivers/media/video/mt9p031.c
> > >> @@ -0,0 +1,751 @@
> > >> +/*
> > >> + * Driver for MT9P031 CMOS Image Sensor from Aptina
> > >> + *
> > >> + * Copyright (C) 2011, Javier Martin
> > >> + *
> > >> + * Copyright (C) 2011, Guennadi Liakhovetski
> > >> + *
> > >> + * Based on the MT9V032 driver and Bastian Hecht's code.
> > >> + *
> > >> + * This program is free software; you can redistribute it and/or
> > >> modify + * it under the terms of the GNU General Public License
> > >> version 2 as + * published by the Free Software Foundation.
> > >> + */
> > >> +
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +#include
> > >> +
> > >> +/* mt9p031 selected register addresses */
> > >> +#define MT9P031_CHIP_VERSION 0x00
> > >> +#define MT9P031_CHIP_VERSION_VALUE 0x1801
> > >> +#define MT9P031_ROW_START0x01
> > >
> > > Don't mix spaces and TABs between "#define" and the macro - just use
> > > one space everywhere.
> >
> > I've done this in order to follow Laurent's directions. He does the
> > same in mt9v032 driver.
> > So, unless Laurent and you agree I think I won't change it.
>
> Ah, so, you use a space for registers and TABs for their values, ok then.
>
> > >> +struct mt9p031 {
> > >> + struct v4l2_subdev subdev;
> > >> + struct media_pad pad;
> > >> + struct v4l2_rect rect; /* Sensor window */
> > >> + struct v4l2_mbus_framefmt format;
> > >> + struct mt9p031_platform_data *pdata;
> > >> + struct mutex power_lock;
> > >
> > > Don't locks _always_ have to be documented? And this one: you only
> > > protect set_power() with it, Laurent, is this correct?
> >
> > Just following the model Laurent applies in mt9v032. Let's see what he
> > has to say about this.
>
> Try running scripts/checkpatch.pl on your patch. I think, it will complain
> about this. And in general it's a good idea to run it before submission;)
>
> > >> +static int mt9p031_reset(struct i2c_client *client)
> > >> +{
> > >> + struct mt9p031 *mt9p031 = to_mt9p031(client);
> > >> + int ret;
> > >> +
> > >> + /* Disable chip output, synchronous option update */
> > >> + ret = reg_write(client, MT9P031_RST, MT9P031_RST_ENABLE);
> > >> + if (ret < 0)
> > >> + return -EIO;
> > >> + ret = reg_write(client, MT9P031_RST, MT9P031_RST_DISABLE);
> > >> + if (ret < 0)
> > >> + return -EIO;
> > >> + ret = mt9p031_set_output_control(mt9p031,
> > >> MT9P031_OUTPUT_CONTROL_CEN, 0); + if (ret < 0)
> > >> + return -EIO;
> > >> + return 0;
> > >
> > > I think, a sequence like
> > >
> > >ret = fn();
> > >if (!ret)
> > >ret = fn();
> > >if (!ret)
> > >ret = fn();
> > >return ret;
> > >
> > > is a better way to achieve the same.
> >
> > Sorry, but I have to disagree. I understand what you want to achieve
> > but this seems quite tricky to me.
> > I explicitly changed parts of the code that were written using that
> > style because I think It was better understandable.
>
> Well, that was my opinion. Since Laurent will be taking this patch via his
> tree, his decision will be final, of course. But I think, he'll agree,
> that at least you have to be consistent across the driver. And at least
> you'd want to propagate your error code up to the caller instead of
> replacing it with "-EIO."
To follow up on my previous answer on this, I agree that the return value
should be propagated instead of replacing it with -EIO.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Hi Guennadi and Javier,
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 000..e406b64
> > --- /dev/null
> > +++ b/drivers/media/video/mt9p031.c
[snip]
> > +#define MT9P031_ROW_START 0x01
>
> Don't mix spaces and TABs between "#define" and the macro - just use one
> space everywhere.
>
> > +#defineMT9P031_ROW_START_SKIP 54
That should be MT9P031_ROW_START_DEF. You should define MT9P031_ROW_START_MIN
and MT9P031_ROW_START_MAX as well, you will need them. Same for column start,
window height and window width.
> > +#define MT9P031_COLUMN_START 0x02
> > +#defineMT9P031_COLUMN_START_SKIP 16
> > +#define MT9P031_WINDOW_HEIGHT 0x03
> > +#define MT9P031_WINDOW_WIDTH 0x04
> > +#define MT9P031_H_BLANKING 0x05
> > +#defineMT9P031_H_BLANKING_VALUE0
> > +#define MT9P031_V_BLANKING 0x06
> > +#defineMT9P031_V_BLANKING_VALUE25
> > +#define MT9P031_OUTPUT_CONTROL 0x07
> > +#defineMT9P031_OUTPUT_CONTROL_CEN 2
> > +#defineMT9P031_OUTPUT_CONTROL_SYN 1
> > +#define MT9P031_SHUTTER_WIDTH_UPPER0x08
> > +#define MT9P031_SHUTTER_WIDTH 0x09
> > +#define MT9P031_PIXEL_CLOCK_CONTROL0x0a
> > +#define MT9P031_FRAME_RESTART 0x0b
> > +#define MT9P031_SHUTTER_DELAY 0x0c
> > +#define MT9P031_RST0x0d
> > +#defineMT9P031_RST_ENABLE 1
> > +#defineMT9P031_RST_DISABLE 0
> > +#define MT9P031_READ_MODE_10x1e
> > +#define MT9P031_READ_MODE_20x20
> > +#defineMT9P031_READ_MODE_2_ROW_MIR 0x8000
> > +#defineMT9P031_READ_MODE_2_COL_MIR 0x4000
> > +#define MT9P031_ROW_ADDRESS_MODE 0x22
> > +#define MT9P031_COLUMN_ADDRESS_MODE0x23
> > +#define MT9P031_GLOBAL_GAIN0x35
> > +
> > +#define MT9P031_MAX_HEIGHT 1944
> > +#define MT9P031_MAX_WIDTH 2592
> > +#define MT9P031_MIN_HEIGHT 2
> > +#define MT9P031_MIN_WIDTH 18
You can get rid of those 4 #define's and use MT9P031_WINDOW_(HEIGHT|
WIDTH)_(MIN|MAX) instead.
> > +struct mt9p031 {
> > + struct v4l2_subdev subdev;
> > + struct media_pad pad;
> > + struct v4l2_rect rect; /* Sensor window */
> > + struct v4l2_mbus_framefmt format;
> > + struct mt9p031_platform_data *pdata;
> > + struct mutex power_lock;
>
> Don't locks _always_ have to be documented? And this one: you only protect
> set_power() with it, Laurent, is this correct?
You're right, locks have to always be documented, either inline or in a
comment block above the structure. A small comment such as /* Protects
power_count */ is enough.
> > + int power_count;
> > + u16 xskip;
> > + u16 yskip;
> > + u16 output_control;
> > + struct regulator *reg_1v8;
> > + struct regulator *reg_2v8;
> > +};
[snip]
> > +static int mt9p031_reset(struct i2c_client *client)
> > +{
> > + struct mt9p031 *mt9p031 = to_mt9p031(client);
> > + int ret;
> > +
> > + /* Disable chip output, synchronous option update */
> > + ret = reg_write(client, MT9P031_RST, MT9P031_RST_ENABLE);
> > + if (ret < 0)
> > + return -EIO;
> > + ret = reg_write(client, MT9P031_RST, MT9P031_RST_DISABLE);
> > + if (ret < 0)
> > + return -EIO;
> > + ret = mt9p031_set_output_control(mt9p031, MT9P031_OUTPUT_CONTROL_CEN,
> > 0); + if (ret < 0)
> > + return -EIO;
> > + return 0;
>
> I think, a sequence like
>
> ret = fn();
> if (!ret)
> ret = fn();
> if (!ret)
> ret = fn();
> return ret;
>
> is a better way to achieve the same.
I disagree with you on that :-) I find code sequences that return as soon as
an error occurs, using the main code path for the error-free case, easier to
read. It can be a matter of personal taste though.
This being said, the function can end with
return mt9p031_set_output_control(mt9p031, MT9P031_OUTPUT_CONTROL_CEN,
0);
instead of
ret = mt9p031_set_output_control(mt9p031, MT9P031_OUTPUT_CONTROL_CEN,
0);
if (ret < 0)
return -EIO;
return 0;
> > +}
> > +
> > +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
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On Mon, 23 May 2011, javier Martin wrote:
> On 21 May 2011 17:29, Guennadi Liakhovetski wrote:
> > On Fri, 20 May 2011, Javier Martin wrote:
> >
> >> This driver adds basic support for Aptina mt9p031 sensor.
> >>
> >> Signed-off-by: Javier Martin
> >> ---
> >> drivers/media/video/Kconfig | 8 +
> >> drivers/media/video/Makefile | 1 +
> >> drivers/media/video/mt9p031.c | 751
> >> +
> >> include/media/mt9p031.h | 11 +
> >> 4 files changed, 771 insertions(+), 0 deletions(-)
> >> create mode 100644 drivers/media/video/mt9p031.c
> >> create mode 100644 include/media/mt9p031.h
> >>
> >> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
> >> new file mode 100644
> >> index 000..e406b64
> >> --- /dev/null
> >> +++ b/drivers/media/video/mt9p031.c
> >> @@ -0,0 +1,751 @@
> >> +/*
> >> + * Driver for MT9P031 CMOS Image Sensor from Aptina
> >> + *
> >> + * Copyright (C) 2011, Javier Martin
> >> + *
> >> + * Copyright (C) 2011, Guennadi Liakhovetski
> >> + *
> >> + * Based on the MT9V032 driver and Bastian Hecht's code.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + */
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +#include
> >> +#include
> >> +#include
> >> +#include
> >> +
> >> +/* mt9p031 selected register addresses */
> >> +#define MT9P031_CHIP_VERSION 0x00
> >> +#define MT9P031_CHIP_VERSION_VALUE 0x1801
> >> +#define MT9P031_ROW_START 0x01
> >
> > Don't mix spaces and TABs between "#define" and the macro - just use one
> > space everywhere.
> >
>
> I've done this in order to follow Laurent's directions. He does the
> same in mt9v032 driver.
> So, unless Laurent and you agree I think I won't change it.
Ah, so, you use a space for registers and TABs for their values, ok then.
> >> +struct mt9p031 {
> >> + struct v4l2_subdev subdev;
> >> + struct media_pad pad;
> >> + struct v4l2_rect rect; /* Sensor window */
> >> + struct v4l2_mbus_framefmt format;
> >> + struct mt9p031_platform_data *pdata;
> >> + struct mutex power_lock;
> >
> > Don't locks _always_ have to be documented? And this one: you only protect
> > set_power() with it, Laurent, is this correct?
> >
>
> Just following the model Laurent applies in mt9v032. Let's see what he
> has to say about this.
Try running scripts/checkpatch.pl on your patch. I think, it will complain
about this. And in general it's a good idea to run it before submission;)
> >> +static int mt9p031_reset(struct i2c_client *client)
> >> +{
> >> + struct mt9p031 *mt9p031 = to_mt9p031(client);
> >> + int ret;
> >> +
> >> + /* Disable chip output, synchronous option update */
> >> + ret = reg_write(client, MT9P031_RST, MT9P031_RST_ENABLE);
> >> + if (ret < 0)
> >> + return -EIO;
> >> + ret = reg_write(client, MT9P031_RST, MT9P031_RST_DISABLE);
> >> + if (ret < 0)
> >> + return -EIO;
> >> + ret = mt9p031_set_output_control(mt9p031,
> >> MT9P031_OUTPUT_CONTROL_CEN, 0);
> >> + if (ret < 0)
> >> + return -EIO;
> >> + return 0;
> >
> >
> > I think, a sequence like
> >
> > ret = fn();
> > if (!ret)
> > ret = fn();
> > if (!ret)
> > ret = fn();
> > return ret;
> >
> > is a better way to achieve the same.
> >
>
> Sorry, but I have to disagree. I understand what you want to achieve
> but this seems quite tricky to me.
> I explicitly changed parts of the code that were written using that
> style because I think It was better understandable.
Well, that was my opinion. Since Laurent will be taking this patch via his
tree, his decision will be final, of course. But I think, he'll agree,
that at least you have to be consistent across the driver. And at least
you'd want to propagate your error code up to the caller instead of
replacing it with "-EIO."
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On 21 May 2011 17:29, Guennadi Liakhovetski wrote: > On Fri, 20 May 2011, Javier Martin wrote: > >> This driver adds basic support for Aptina mt9p031 sensor. >> >> Signed-off-by: Javier Martin >> --- >> drivers/media/video/Kconfig | 8 + >> drivers/media/video/Makefile | 1 + >> drivers/media/video/mt9p031.c | 751 >> + >> include/media/mt9p031.h | 11 + >> 4 files changed, 771 insertions(+), 0 deletions(-) >> create mode 100644 drivers/media/video/mt9p031.c >> create mode 100644 include/media/mt9p031.h >> >> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig >> index 00f51dd..5c96b89 100644 >> --- a/drivers/media/video/Kconfig >> +++ b/drivers/media/video/Kconfig >> @@ -329,6 +329,14 @@ config VIDEO_OV7670 >> OV7670 VGA camera. It currently only works with the M88ALP01 >> controller. >> >> +config VIDEO_MT9P031 >> + tristate "Aptina MT9P031 support" >> + depends on I2C && VIDEO_V4L2 >> + ---help--- >> + This driver supports MT9P031 cameras from Micron >> + This is a Video4Linux2 sensor-level driver for the Micron >> + mt0p031 5 Mpixel camera. > > Two sentences seem to repeat the same with other words, and it's better to > stay consistent: just use Aptina everywhere, maybe put Micron in brackets > at one location. > OK, I will fix it. >> + >> config VIDEO_MT9V011 >> tristate "Micron mt9v011 sensor support" >> depends on I2C && VIDEO_V4L2 >> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile >> index ace5d8b..912b29b 100644 >> --- a/drivers/media/video/Makefile >> +++ b/drivers/media/video/Makefile >> @@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o >> obj-$(CONFIG_VIDEO_OV7670) += ov7670.o >> obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o >> obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o >> +obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o >> obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o >> obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o >> obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o >> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c >> new file mode 100644 >> index 000..e406b64 >> --- /dev/null >> +++ b/drivers/media/video/mt9p031.c >> @@ -0,0 +1,751 @@ >> +/* >> + * Driver for MT9P031 CMOS Image Sensor from Aptina >> + * >> + * Copyright (C) 2011, Javier Martin >> + * >> + * Copyright (C) 2011, Guennadi Liakhovetski >> + * >> + * Based on the MT9V032 driver and Bastian Hecht's code. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> + >> +/* mt9p031 selected register addresses */ >> +#define MT9P031_CHIP_VERSION 0x00 >> +#define MT9P031_CHIP_VERSION_VALUE 0x1801 >> +#define MT9P031_ROW_START 0x01 > > Don't mix spaces and TABs between "#define" and the macro - just use one > space everywhere. > I've done this in order to follow Laurent's directions. He does the same in mt9v032 driver. So, unless Laurent and you agree I think I won't change it. > >> +#define MT9P031_ROW_START_SKIP 54 >> +#define MT9P031_COLUMN_START 0x02 >> +#define MT9P031_COLUMN_START_SKIP 16 >> +#define MT9P031_WINDOW_HEIGHT 0x03 >> +#define MT9P031_WINDOW_WIDTH 0x04 >> +#define MT9P031_H_BLANKING 0x05 >> +#define MT9P031_H_BLANKING_VALUE 0 >> +#define MT9P031_V_BLANKING 0x06 >> +#define MT9P031_V_BLANKING_VALUE 25 >> +#define MT9P031_OUTPUT_CONTROL 0x07 >> +#define MT9P031_OUTPUT_CONTROL_CEN 2 >> +#define MT9P031_OUTPUT_CONTROL_SYN 1 >> +#define MT9P031_SHUTTER_WIDTH_UPPER 0x08 >> +#define MT9P031_SHUTTER_WIDTH 0x09 >> +#define MT9P031_PIXEL_CLOCK_CONTROL 0x0a >> +#define MT9P031_FRAME_RESTART 0x0b >> +#define MT9P031_SHUTTER_DELAY 0x0c >> +#define MT9P031_RST 0x0d >> +#define MT9P031_RST_ENABLE 1 >> +#define MT9P031_RST_DISABLE 0 >> +#define MT9P031_READ_MODE_1 0x1e >> +#define MT9P031_READ_MODE_2 0x20 >> +#define MT9P031_READ_MODE_2_ROW_MIR 0x8000 >> +#define MT9P031_READ_MODE_2_COL_MIR 0x4000 >> +#define MT9P031_ROW_ADDRESS_MODE 0x22 >> +#define MT9P031_COLUMN_ADDRESS_MODE 0x23 >> +#define MT9P031_GLOBAL_GAIN 0x35 >> + >> +#define
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On 23 May 2011 05:01, Chris Rodley wrote: > Error when using media-ctl as below with v2 mt9p031 driver from Javier and > latest media-ctl version. > Is there a patch I missed to add different formats - or maybe my command is > wrong? > > # ./media-ctl -v -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1], "OMAP3 ISP > CCDC":1->"OMAP3 ISP CCDC output":0[1]' > Opening media device /dev/media0 > Enumerating entities > Found 16 entities > Enumerating pads and links > Resetting all links to inactive > Setting up link 16:0 -> 5:0 [1] > Setting up link 5:1 -> 6:0 [1] > > # ./media-ctl -v -f '"mt9p031 2-0048":0[SGRBG8 320x240], "OMAP3 ISP > CCDC":1[SGRBG8 320x240]' > Opening media device /dev/media0 > Enumerating entities > Found 16 entities > Enumerating pads and links > Setting up format SGRBG8 320x240 on pad mt9p031 2-0048/0 > Unable to set format: Invalid argument (-22) > > I also tried: > ./media-ctl -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1], "OMAP3 ISP > CCDC":1->"OMAP3 ISP CCDC output":0[1]' > ./media-ctl -f '"mt9p031 2-0048":0[SGRBG10 752x480 (1,5)/752x480], "OMAP3 ISP > CCDC":0[SGRBG8 752x480], "OMAP3 ISP CCDC":1[SGRBG8 752x480]' > > With the same result. > > Cheers, > Chris > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please, try the following: ./media-ctl -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1], "OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]' ./media-ctl -f '"mt9p031 2-0048":0[SGRBG12 320x240], "OMAP3 ISP CCDC":0[SGRBG8 320x240], "OMAP3 ISP CCDC":1[SGRBG8 320x240]' Thanks. -- Javier Martin Vista Silicon S.L. CDTUC - FASE C - Oficina S-345 Avda de los Castros s/n 39005- Santander. Cantabria. Spain +34 942 25 32 60 www.vista-silicon.com -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Error when using media-ctl as below with v2 mt9p031 driver from Javier and latest media-ctl version. Is there a patch I missed to add different formats - or maybe my command is wrong? # ./media-ctl -v -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1], "OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]' Opening media device /dev/media0 Enumerating entities Found 16 entities Enumerating pads and links Resetting all links to inactive Setting up link 16:0 -> 5:0 [1] Setting up link 5:1 -> 6:0 [1] # ./media-ctl -v -f '"mt9p031 2-0048":0[SGRBG8 320x240], "OMAP3 ISP CCDC":1[SGRBG8 320x240]' Opening media device /dev/media0 Enumerating entities Found 16 entities Enumerating pads and links Setting up format SGRBG8 320x240 on pad mt9p031 2-0048/0 Unable to set format: Invalid argument (-22) I also tried: ./media-ctl -r -l '"mt9p031 2-0048":0->"OMAP3 ISP CCDC":0[1], "OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]' ./media-ctl -f '"mt9p031 2-0048":0[SGRBG10 752x480 (1,5)/752x480], "OMAP3 ISP CCDC":0[SGRBG8 752x480], "OMAP3 ISP CCDC":1[SGRBG8 752x480]' With the same result. Cheers, Chris -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Hi Mauro, On Saturday 21 May 2011 14:55:12 Mauro Carvalho Chehab wrote: > Hi Laurent, > > Despite all those changes at Nokia side, I'm still assuming that you're > handling the omap3 patches. That's correct. I maintain (with Sakari) the OMAP3 ISP driver. For practical reason I'm the one who sends the pull requests. > So, I'm just marking those two patches as RFC until I receive a pull request > from you. Note that this patch is not strictly tied to the OMAP3 ISP, as it adds support for a sensor that can be used with any media controller-enabled ISP. In practice we don't have many of such ISPs :-) I'll thus take the patch in my tree. > Anyway, in this specific case, Koen made some comments, so we should wait > for Javier answer before moving ahead. Guennadi also sent comments. I'll wait for the next version of the patch. Please note that I will probably apply patch 1/2 only at first, as we still have no proper solution to support sensors on pluggable expansion modules in a modular way. More work is needed in that area before patch 2/2 (or rather a patch set that will provide the same functionality) can be applied. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
On Fri, 20 May 2011, Javier Martin wrote:
> This driver adds basic support for Aptina mt9p031 sensor.
>
> Signed-off-by: Javier Martin
> ---
> drivers/media/video/Kconfig |8 +
> drivers/media/video/Makefile |1 +
> drivers/media/video/mt9p031.c | 751
> +
> include/media/mt9p031.h | 11 +
> 4 files changed, 771 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/mt9p031.c
> create mode 100644 include/media/mt9p031.h
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index 00f51dd..5c96b89 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -329,6 +329,14 @@ config VIDEO_OV7670
> OV7670 VGA camera. It currently only works with the M88ALP01
> controller.
>
> +config VIDEO_MT9P031
> + tristate "Aptina MT9P031 support"
> + depends on I2C && VIDEO_V4L2
> + ---help---
> + This driver supports MT9P031 cameras from Micron
> + This is a Video4Linux2 sensor-level driver for the Micron
> + mt0p031 5 Mpixel camera.
Two sentences seem to repeat the same with other words, and it's better to
stay consistent: just use Aptina everywhere, maybe put Micron in brackets
at one location.
> +
> config VIDEO_MT9V011
> tristate "Micron mt9v011 sensor support"
> depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index ace5d8b..912b29b 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
> obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
> obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
> obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o
> +obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o
> obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o
> obj-$(CONFIG_VIDEO_SR030PC30)+= sr030pc30.o
> obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o
> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
> new file mode 100644
> index 000..e406b64
> --- /dev/null
> +++ b/drivers/media/video/mt9p031.c
> @@ -0,0 +1,751 @@
> +/*
> + * Driver for MT9P031 CMOS Image Sensor from Aptina
> + *
> + * Copyright (C) 2011, Javier Martin
> + *
> + * Copyright (C) 2011, Guennadi Liakhovetski
> + *
> + * Based on the MT9V032 driver and Bastian Hecht's code.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +
> +/* mt9p031 selected register addresses */
> +#define MT9P031_CHIP_VERSION 0x00
> +#define MT9P031_CHIP_VERSION_VALUE 0x1801
> +#define MT9P031_ROW_START0x01
Don't mix spaces and TABs between "#define" and the macro - just use one
space everywhere.
> +#define MT9P031_ROW_START_SKIP 54
> +#define MT9P031_COLUMN_START 0x02
> +#define MT9P031_COLUMN_START_SKIP 16
> +#define MT9P031_WINDOW_HEIGHT0x03
> +#define MT9P031_WINDOW_WIDTH 0x04
> +#define MT9P031_H_BLANKING 0x05
> +#define MT9P031_H_BLANKING_VALUE0
> +#define MT9P031_V_BLANKING 0x06
> +#define MT9P031_V_BLANKING_VALUE25
> +#define MT9P031_OUTPUT_CONTROL 0x07
> +#define MT9P031_OUTPUT_CONTROL_CEN 2
> +#define MT9P031_OUTPUT_CONTROL_SYN 1
> +#define MT9P031_SHUTTER_WIDTH_UPPER 0x08
> +#define MT9P031_SHUTTER_WIDTH0x09
> +#define MT9P031_PIXEL_CLOCK_CONTROL 0x0a
> +#define MT9P031_FRAME_RESTART0x0b
> +#define MT9P031_SHUTTER_DELAY0x0c
> +#define MT9P031_RST 0x0d
> +#define MT9P031_RST_ENABLE 1
> +#define MT9P031_RST_DISABLE 0
> +#define MT9P031_READ_MODE_1 0x1e
> +#define MT9P031_READ_MODE_2 0x20
> +#define MT9P031_READ_MODE_2_ROW_MIR 0x8000
> +#define MT9P031_READ_MODE_2_COL_MIR 0x4000
> +#define MT9P031_ROW_ADDRESS_MODE 0x22
> +#define MT9P031_COLUMN_ADDRESS_MODE 0x23
> +#define MT9P031_GLOBAL_GAIN 0x35
> +
> +#define MT9P031_MAX_HEIGHT 1944
> +#define MT9P031_MAX_WIDTH2592
> +#define MT9P031_MIN_HEIGHT 2
> +#define MT9P031_MIN_WIDTH18
> +
> +struct mt9p031 {
> + struct v4l2_subdev subdev;
> + struct media_pad pad;
> + struct v4l2_rect rect; /* Sensor window */
> + struc
Re: [PATCH v2 1/2] MT9P031: Add support for Aptina mt9p031 sensor.
Hi Laurent, Despite all those changes at Nokia side, I'm still assuming that you're handling the omap3 patches. So, I'm just marking those two patches as RFC until I receive a pull request from you. Anyway, in this specific case, Koen made some comments, so we should wait for Javier answer before moving ahead. Thanks, Mauro PS. If you're not keep doing it anymore for omap3, please point me to another maintainer that would be taking it. I just need to know whom is responsible to send me patches for each driver(s). Em 20-05-2011 10:47, Javier Martin escreveu: > This driver adds basic support for Aptina mt9p031 sensor. > > Signed-off-by: Javier Martin > --- > drivers/media/video/Kconfig |8 + > drivers/media/video/Makefile |1 + > drivers/media/video/mt9p031.c | 751 > + > include/media/mt9p031.h | 11 + > 4 files changed, 771 insertions(+), 0 deletions(-) > create mode 100644 drivers/media/video/mt9p031.c > create mode 100644 include/media/mt9p031.h > > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig > index 00f51dd..5c96b89 100644 > --- a/drivers/media/video/Kconfig > +++ b/drivers/media/video/Kconfig > @@ -329,6 +329,14 @@ config VIDEO_OV7670 > OV7670 VGA camera. It currently only works with the M88ALP01 > controller. > > +config VIDEO_MT9P031 > + tristate "Aptina MT9P031 support" > + depends on I2C && VIDEO_V4L2 > + ---help--- > + This driver supports MT9P031 cameras from Micron > + This is a Video4Linux2 sensor-level driver for the Micron > + mt0p031 5 Mpixel camera. > + > config VIDEO_MT9V011 > tristate "Micron mt9v011 sensor support" > depends on I2C && VIDEO_V4L2 > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile > index ace5d8b..912b29b 100644 > --- a/drivers/media/video/Makefile > +++ b/drivers/media/video/Makefile > @@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o > obj-$(CONFIG_VIDEO_OV7670) += ov7670.o > obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o > obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o > +obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o > obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o > obj-$(CONFIG_VIDEO_SR030PC30)+= sr030pc30.o > obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o > diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c > new file mode 100644 > index 000..e406b64 > --- /dev/null > +++ b/drivers/media/video/mt9p031.c > @@ -0,0 +1,751 @@ > +/* > + * Driver for MT9P031 CMOS Image Sensor from Aptina > + * > + * Copyright (C) 2011, Javier Martin > + * > + * Copyright (C) 2011, Guennadi Liakhovetski > + * > + * Based on the MT9V032 driver and Bastian Hecht's code. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +/* mt9p031 selected register addresses */ > +#define MT9P031_CHIP_VERSION 0x00 > +#define MT9P031_CHIP_VERSION_VALUE 0x1801 > +#define MT9P031_ROW_START0x01 > +#define MT9P031_ROW_START_SKIP 54 > +#define MT9P031_COLUMN_START 0x02 > +#define MT9P031_COLUMN_START_SKIP 16 > +#define MT9P031_WINDOW_HEIGHT0x03 > +#define MT9P031_WINDOW_WIDTH 0x04 > +#define MT9P031_H_BLANKING 0x05 > +#define MT9P031_H_BLANKING_VALUE0 > +#define MT9P031_V_BLANKING 0x06 > +#define MT9P031_V_BLANKING_VALUE25 > +#define MT9P031_OUTPUT_CONTROL 0x07 > +#define MT9P031_OUTPUT_CONTROL_CEN 2 > +#define MT9P031_OUTPUT_CONTROL_SYN 1 > +#define MT9P031_SHUTTER_WIDTH_UPPER 0x08 > +#define MT9P031_SHUTTER_WIDTH0x09 > +#define MT9P031_PIXEL_CLOCK_CONTROL 0x0a > +#define MT9P031_FRAME_RESTART0x0b > +#define MT9P031_SHUTTER_DELAY0x0c > +#define MT9P031_RST 0x0d > +#define MT9P031_RST_ENABLE 1 > +#define MT9P031_RST_DISABLE 0 > +#define MT9P031_READ_MODE_1 0x1e > +#define MT9P031_READ_MODE_2 0x20 > +#define MT9P031_READ_MODE_2_ROW_MIR 0x8000 > +#define MT9P031_READ_MODE_2_COL_MIR 0x4000 > +#define MT9P031_ROW_ADDRESS_MODE 0x22 > +#define MT9P031_COLUMN_ADDRESS_MODE 0x23 > +#define MT9P031_GLOBAL_GAIN 0x35 > + > +#define MT9P031_MAX_HEIGHT 1944 > +#define MT9P031_MAX_WIDTH
