On 09/14/2011 09:10 AM, Scott Jiang wrote:
>>> +static int bcap_qbuf(struct file *file, void *priv,
>>> + struct v4l2_buffer *buf)
>>> +{
>>> + struct bcap_device *bcap_dev = video_drvdata(file);
>>> + struct v4l2_fh *fh = file->private_data;
>>> + struct bcap_fh *bcap_fh = container_of(fh, struct bcap_fh, fh);
>>> +
>>> + if (!bcap_fh->io_allowed)
>>> + return -EACCES;
>>
>> I suppose -EBUSY would be more appropriate here.
>>
> no, io_allowed is to control which file instance has the right to do I/O.
Looks like you are doing here what the v4l2 priority mechanism is meant for.
Have you considered the access priority (VIDIOC_G_PRIORITY/VIDIOC_S_PRIORITY
and friends)? Does it have any shortcomings?
>
>>> + fmt =&bcap_formats[i];
>>> + if (mbus_code)
>>> + *mbus_code = fmt->mbus_code;
>>> + if (bpp)
>>> + *bpp = fmt->bpp;
>>> + v4l2_fill_mbus_format(&mbus_fmt, pixfmt,
>>> + fmt->mbus_code);
>>> + ret = v4l2_subdev_call(bcap->sd, video,
>>> + try_mbus_fmt,&mbus_fmt);
>>> + if (ret< 0)
>>> + return ret;
>>> + v4l2_fill_pix_format(pixfmt,&mbus_fmt);
>>> + pixfmt->bytesperline = pixfmt->width * fmt->bpp;
>>> + pixfmt->sizeimage = pixfmt->bytesperline
>>> + * pixfmt->height;
>>
>> Still pixfmt->pixelformat isn't filled.
>>
> no here pixfmt->pixelformat is passed in
>
>>> + return 0;
>>> + }
>>> + }
>>> + return -EINVAL;
>>
>> I think you should return some default format, rather than giving up
>> when the fourcc doesn't match. However I'm not 100% sure this is
>> the specification requirement.
>>
> no, there is no default format for bridge driver because it knows
> nothing about this.
> all the format info bridge needs ask subdevice.
It's the bridge driver that exports a device node and is responsible for
setting the default format. It should be possible to start streaming right
after opening the device, without VIDIOC_S_FMT, with some reasonable defaults.
If, as you say, the bridge knows nothing about formats what the bcap_formats[]
array is here for ?
>
>>> +static const struct ppi_ops ppi_ops = {
>>> + .attach_irq = ppi_attach_irq,
>>> + .detach_irq = ppi_detach_irq,
>>> + .start = ppi_start,
>>> + .stop = ppi_stop,
>>> + .set_params = ppi_set_params,
>>> + .update_addr = ppi_update_addr,
>>> +};
>>
>> How about moving this struct to the bottom of the file and getting rid of
>> all the above forward declarations ?
>>
> I'd like to put global varible before function in a file.
>
>>> +
>>> +void delete_ppi_instance(struct ppi_if *ppi)
>>> +{
>>> + peripheral_free_list(ppi->info->pin_req);
>>> + kfree(ppi);
>>> +}
>>
>> As a side note, I was not sure if this is just a resend of your original
>> patches or a second version. It would be good to indicate that in the message
>> subject. I think it's not a big deal and makes the reviewers' life easier.
> if I don't add a version number in subject, it means it is the first version.
Sorry, please ignore this. I got confused by the mailer and thought the same
patches appeared twice on the list.
Thanks,
--
Sylwester Nawrocki
Samsung Poland R&D Center
--
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
