vpif_display.c bug

vpif_display.c bug

[Hans Verkuil]

Hi Chaithrika,

This code in vpif_display.c is not correct:

        for (i = 0; i < subdev_count; i++) {
                vpif_obj.sd[i] = v4l2_i2c_new_probed_subdev(&vpif_obj.v4l2_dev,
                                                i2c_adap, subdevdata[i].name,
                                                subdevdata[i].name,
                                                &subdevdata[i].addr);
                if (!vpif_obj.sd[i]) {
                        vpif_err("Error registering v4l2 subdevice\n");
                        goto probe_subdev_out;
                }

                if (vpif_obj.sd[i])
                        vpif_obj.sd[i]->grp_id = 1 << i;
        }

This: '&subdevdata[i].addr' should be: I2C_ADDRS(subdevdata[i].addr).

The list of probe addresses must be terminated by I2C_CLIENT_END (= -1) and
that isn't the case here.

An alternative solution is to use v4l2_i2c_new_subdev, but then no probing
will take place. But I think that you don't want probing at all since this
address information comes from the platform data, so one can assume that
that data is correct.

Even better is to copy the implementation from vpfe_capture.c and to use
v4l2_i2c_new_subdev_board().

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

RE: vpif_display.c bug

[chaithrika]

On Mon, Aug 10, 2009 at 11:37:23, Hans Verkuil wrote:
> Hi Chaithrika,
> 
> This code in vpif_display.c is not correct:
> 
>         for (i = 0; i < subdev_count; i++) {
>                 vpif_obj.sd[i] = v4l2_i2c_new_probed_subdev(&vpif_obj.v4l2_dev,
>                                                 i2c_adap, subdevdata[i].name,
>                                                 subdevdata[i].name,
>                                                 &subdevdata[i].addr);
>                 if (!vpif_obj.sd[i]) {
>                         vpif_err("Error registering v4l2 subdevice\n");
>                         goto probe_subdev_out;
>                 }
> 
>                 if (vpif_obj.sd[i])
>                         vpif_obj.sd[i]->grp_id = 1 << i;
>         }
> 
> This: '&subdevdata[i].addr' should be: I2C_ADDRS(subdevdata[i].addr).
> 
> The list of probe addresses must be terminated by I2C_CLIENT_END (= -1) and
> that isn't the case here.
> 
> An alternative solution is to use v4l2_i2c_new_subdev, but then no probing
> will take place. But I think that you don't want probing at all since this
> address information comes from the platform data, so one can assume that
> that data is correct.
> 
> Even better is to copy the implementation from vpfe_capture.c and to use
> v4l2_i2c_new_subdev_board().
> 

Hans,
Thank you for the suggestions.
I will look into this and submit a patch to correct this bug.

Regards, 
Chaithrika

> Regards,
> 
> 	Hans
> 
> -- 
> Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
> 

RE: vpif_display.c bug

[Karicheri, Muralidharan]

Chaithrika,

No need to change this since this is already corrected as part of my vpif capture patch set that I had submitted for review. I had mentioned this to Hans as well.

Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
new phone: 301-407-9583
Old Phone : 301-515-3736 (will be deprecated)
email: m-karicheri2@ti.com

>-----Original Message-----
>From: linux-media-owner@vger.kernel.org [mailto:linux-media-
>owner@vger.kernel.org] On Behalf Of Subrahmanya, Chaithrika
>Sent: Monday, August 10, 2009 8:51 AM
>To: 'Hans Verkuil'
>Cc: linux-media@vger.kernel.org
>Subject: RE: vpif_display.c bug
>
>On Mon, Aug 10, 2009 at 11:37:23, Hans Verkuil wrote:
>> Hi Chaithrika,
>>
>> This code in vpif_display.c is not correct:
>>
>>         for (i = 0; i < subdev_count; i++) {
>>                 vpif_obj.sd[i] =
>v4l2_i2c_new_probed_subdev(&vpif_obj.v4l2_dev,
>>                                                 i2c_adap,
>subdevdata[i].name,
>>                                                 subdevdata[i].name,
>>                                                 &subdevdata[i].addr);
>>                 if (!vpif_obj.sd[i]) {
>>                         vpif_err("Error registering v4l2 subdevice\n");
>>                         goto probe_subdev_out;
>>                 }
>>
>>                 if (vpif_obj.sd[i])
>>                         vpif_obj.sd[i]->grp_id = 1 << i;
>>         }
>>
>> This: '&subdevdata[i].addr' should be: I2C_ADDRS(subdevdata[i].addr).
>>
>> The list of probe addresses must be terminated by I2C_CLIENT_END (= -1)
>and
>> that isn't the case here.
>>
>> An alternative solution is to use v4l2_i2c_new_subdev, but then no
>probing
>> will take place. But I think that you don't want probing at all since
>this
>> address information comes from the platform data, so one can assume that
>> that data is correct.
>>
>> Even better is to copy the implementation from vpfe_capture.c and to use
>> v4l2_i2c_new_subdev_board().
>>
>
>Hans,
>Thank you for the suggestions.
>I will look into this and submit a patch to correct this bug.
>
>Regards,
>Chaithrika
>
>> Regards,
>>
>> 	Hans
>>
>> --
>> Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
>>
>
>
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: vpif_display.c bug

[chaithrika]

On Mon, Aug 10, 2009 at 20:39:05, Karicheri, Muralidharan wrote:
> Chaithrika,
> 
> No need to change this since this is already corrected as part of my vpif capture patch set that I had submitted for review. I had mentioned this to Hans as well.
> 
Murali,

Thank you for correcting this bug in your patch set. 

Regards, 
Chaithrika

> Murali Karicheri
> Software Design Engineer
> Texas Instruments Inc.
> Germantown, MD 20874
> new phone: 301-407-9583
> Old Phone : 301-515-3736 (will be deprecated)
> email: m-karicheri2@ti.com
> 
> >-----Original Message-----
> >From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> >owner@vger.kernel.org] On Behalf Of Subrahmanya, Chaithrika
> >Sent: Monday, August 10, 2009 8:51 AM
> >To: 'Hans Verkuil'
> >Cc: linux-media@vger.kernel.org
> >Subject: RE: vpif_display.c bug
> >
> >On Mon, Aug 10, 2009 at 11:37:23, Hans Verkuil wrote:
> >> Hi Chaithrika,
> >>
> >> This code in vpif_display.c is not correct:
> >>
> >>         for (i = 0; i < subdev_count; i++) {
> >>                 vpif_obj.sd[i] =
> >v4l2_i2c_new_probed_subdev(&vpif_obj.v4l2_dev,
> >>                                                 i2c_adap,
> >subdevdata[i].name,
> >>                                                 subdevdata[i].name,
> >>                                                 &subdevdata[i].addr);
> >>                 if (!vpif_obj.sd[i]) {
> >>                         vpif_err("Error registering v4l2 subdevice\n");
> >>                         goto probe_subdev_out;
> >>                 }
> >>
> >>                 if (vpif_obj.sd[i])
> >>                         vpif_obj.sd[i]->grp_id = 1 << i;
> >>         }
> >>
> >> This: '&subdevdata[i].addr' should be: I2C_ADDRS(subdevdata[i].addr).
> >>
> >> The list of probe addresses must be terminated by I2C_CLIENT_END (= -1)
> >and
> >> that isn't the case here.
> >>
> >> An alternative solution is to use v4l2_i2c_new_subdev, but then no
> >probing
> >> will take place. But I think that you don't want probing at all since
> >this
> >> address information comes from the platform data, so one can assume that
> >> that data is correct.
> >>
> >> Even better is to copy the implementation from vpfe_capture.c and to use
> >> v4l2_i2c_new_subdev_board().
> >>
> >
> >Hans,
> >Thank you for the suggestions.
> >I will look into this and submit a patch to correct this bug.
> >
> >Regards,
> >Chaithrika
> >
> >> Regards,
> >>
> >> 	Hans
> >>
> >> --
> >> Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
> >>
> >
> >
> >
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-media" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

RE: vpif_display.c bug

[Karicheri, Muralidharan]

Hans,

I have already changed v4l2_i2c_new_probed_subdev() to v4l2_i2c_new_subdev_board() in my latest patch set for adding vpif capture driver for DM6467 that you had reviewed. I think this change is not needed
once that patch is applied.

Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
new phone: 301-407-9583
Old Phone : 301-515-3736 (will be deprecated)
email: m-karicheri2@ti.com

>-----Original Message-----
>From: linux-media-owner@vger.kernel.org [mailto:linux-media-
>owner@vger.kernel.org] On Behalf Of Hans Verkuil
>Sent: Monday, August 10, 2009 2:07 AM
>To: Subrahmanya, Chaithrika
>Cc: linux-media@vger.kernel.org
>Subject: vpif_display.c bug
>
>Hi Chaithrika,
>
>This code in vpif_display.c is not correct:
>
>        for (i = 0; i < subdev_count; i++) {
>                vpif_obj.sd[i] =
>v4l2_i2c_new_probed_subdev(&vpif_obj.v4l2_dev,
>                                                i2c_adap,
>subdevdata[i].name,
>                                                subdevdata[i].name,
>                                                &subdevdata[i].addr);
>                if (!vpif_obj.sd[i]) {
>                        vpif_err("Error registering v4l2 subdevice\n");
>                        goto probe_subdev_out;
>                }
>
>                if (vpif_obj.sd[i])
>                        vpif_obj.sd[i]->grp_id = 1 << i;
>        }
>
>This: '&subdevdata[i].addr' should be: I2C_ADDRS(subdevdata[i].addr).
>
>The list of probe addresses must be terminated by I2C_CLIENT_END (= -1) and
>that isn't the case here.
>
>An alternative solution is to use v4l2_i2c_new_subdev, but then no probing
>will take place. But I think that you don't want probing at all since this
>address information comes from the platform data, so one can assume that
>that data is correct.
>
>Even better is to copy the implementation from vpfe_capture.c and to use
>v4l2_i2c_new_subdev_board().
>
>Regards,
>
>	Hans
>
>--
>Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
>--
>To unsubscribe from this list: send the line "unsubscribe linux-media" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: vpif_display.c bug

[Hans Verkuil]

On Monday 10 August 2009 17:07:51 Karicheri, Muralidharan wrote:
> Hans,
> 
> I have already changed v4l2_i2c_new_probed_subdev() to v4l2_i2c_new_subdev_board() in my latest patch set for adding vpif capture driver for DM6467 that you had reviewed. I think this change is not needed
> once that patch is applied.

You are right. Hmm, I really have a bad memory for these things, I reviewed
it only last Friday :-(

Regards,

	Hans

> 
> Murali Karicheri
> Software Design Engineer
> Texas Instruments Inc.
> Germantown, MD 20874
> new phone: 301-407-9583
> Old Phone : 301-515-3736 (will be deprecated)
> email: m-karicheri2@ti.com
> 
> >-----Original Message-----
> >From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> >owner@vger.kernel.org] On Behalf Of Hans Verkuil
> >Sent: Monday, August 10, 2009 2:07 AM
> >To: Subrahmanya, Chaithrika
> >Cc: linux-media@vger.kernel.org
> >Subject: vpif_display.c bug
> >
> >Hi Chaithrika,
> >
> >This code in vpif_display.c is not correct:
> >
> >        for (i = 0; i < subdev_count; i++) {
> >                vpif_obj.sd[i] =
> >v4l2_i2c_new_probed_subdev(&vpif_obj.v4l2_dev,
> >                                                i2c_adap,
> >subdevdata[i].name,
> >                                                subdevdata[i].name,
> >                                                &subdevdata[i].addr);
> >                if (!vpif_obj.sd[i]) {
> >                        vpif_err("Error registering v4l2 subdevice\n");
> >                        goto probe_subdev_out;
> >                }
> >
> >                if (vpif_obj.sd[i])
> >                        vpif_obj.sd[i]->grp_id = 1 << i;
> >        }
> >
> >This: '&subdevdata[i].addr' should be: I2C_ADDRS(subdevdata[i].addr).
> >
> >The list of probe addresses must be terminated by I2C_CLIENT_END (= -1) and
> >that isn't the case here.
> >
> >An alternative solution is to use v4l2_i2c_new_subdev, but then no probing
> >will take place. But I think that you don't want probing at all since this
> >address information comes from the platform data, so one can assume that
> >that data is correct.
> >
> >Even better is to copy the implementation from vpfe_capture.c and to use
> >v4l2_i2c_new_subdev_board().
> >
> >Regards,
> >
> >	Hans
> >
> >--
> >Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-media" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> 



-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom