Hi Manjunath, On Tuesday 31 July 2012 13:15:27 Manju wrote: > On Friday 27 July 2012 04:19 PM, Laurent Pinchart wrote: > > On Friday 27 July 2012 05:49:24 Hadli, Manjunath wrote: > >> On Thu, Jul 26, 2012 at 05:55:31, Laurent Pinchart wrote: > >>> On Tuesday 17 July 2012 10:43:54 Hadli, Manjunath wrote: > >>>> On Sun, Jul 15, 2012 at 18:16:25, Laurent Pinchart wrote: > >>>>> On Wednesday 11 July 2012 21:09:26 Manjunath Hadli wrote: > >>>>>> Add documentation on the Davinci VPFE driver. Document the subdevs, > >>>>>> and private IOTCLs the driver implements > >>>>>> > >>>>>> Signed-off-by: Manjunath Hadli <manjunath.hadli@xxxxxx> > >>>>>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@xxxxxx> > >>> > >>> [snip] > >>> > >>>>>> +Private IOCTLs > >>>>>> +============== > >>>>>> + > >>>>>> +The Davinci Video processing Front End (VPFE) driver supports > >>>>>> standard V4L2 > >>>>>> +IOCTLs and controls where possible and practical. Much of the > >>>>>> functions provided > >>>>>> +by the VPFE, however, does not fall under the standard IOCTLs. > >>>>>> + > >>>>>> +In general, there is a private ioctl for configuring each of the > >>>>>> blocks > >>>>>> +containing hardware-dependent functions. > >>>>>> + > >>>>>> +The following private IOCTLs are supported: > >>>>>> + > >>>>>> +1: IOCTL: PREV_S_PARAM/PREV_G_PARAM > >>>>>> +Description: > >>>>>> + Sets/Gets the parameters required by the previewer module > >>>>>> +Parameter: > >>>>>> + /** > >>>>>> + * struct prev_module_param- structure to configure preview > >>>>>> modules > >>>>>> + * @version: Version of the preview module > >>>>> > >>>>> Who is responsible for filling this field, the application or the > >>>>> driver ? > >>>> > >>>> The application is responsible for filling this info. He would > >>>> enumerate the capabilities first and set them using S_PARAM/G_PARAM. > >>> > >>> And what's the point of the application setting the version field ? How > >>> does the driver use it ? > >> > >> The version may not be required. Will remove it. > >> > >>>>>> + * @len: Length of the module config structure > >>>>>> + * @module_id: Module id > >>>>>> + * @param: pointer to module config parameter. > >>>>> > >>>>> What is module_id for ? What does param point to ? > >>>> > >>>> There are a lot of tiny modules in the previewer/resizer which are > >>>> enumerated as individual modules. The param points to the parameter set > >>>> that the module expects to be set. > >>> > >>> Why don't you implement something similar to > >>> VPFE_CMD_S_CCDC_RAW_PARAMS/VPFE_CMD_G_CCDC_RAW_PARAMS instead ? > >> > >> I feel if we implement direct IOCTLS there might be many of them. To make > >> sure than independent of the number of internal modules present, having > >> the > >> same IOCTL used for all modules is a good idea. > > > > You can set several parameters using a single ioctl, much like > > VPFE_CMD_S_CCDC_RAW_PARAMS does. You don't need one ioctl per parameter. > > > > PREV_ENUM_CAP, PREV_[GS]_PARAM and PREV_[GS]_CONFIG are essentially > > reinventing V4L2 controls, and I don't think that's a good idea. > > Ok. I looked into this, and found that the structure needed to pass > all the parameters is going to be huge. just to avoid a big structure > from the user space, I propose: > > Having a union of structures and a parameter identifying the structure. > > In that way, we will remove the enumeration and all the other > things except for a SET and GET, much like the CCDC_RAW_PARAMS > like you suggested. So essentially we will have only 2 IOCTLS for setting > the private params/configs and remove the rest. I hope that was your > point and this proposal will solve it? What about something like the following structure, from the OMAP3 ISP driver ? struct omap3isp_prev_update_config { __u32 update; __u32 flag; __u32 shading_shift; struct omap3isp_prev_luma __user *luma; struct omap3isp_prev_hmed __user *hmed; struct omap3isp_prev_cfa __user *cfa; struct omap3isp_prev_csup __user *csup; struct omap3isp_prev_wbal __user *wbal; struct omap3isp_prev_blkadj __user *blkadj; struct omap3isp_prev_rgbtorgb __user *rgb2rgb; struct omap3isp_prev_csc __user *csc; struct omap3isp_prev_yclimit __user *yclimit; struct omap3isp_prev_dcor __user *dcor; struct omap3isp_prev_nf __user *nf; struct omap3isp_prev_gtables __user *gamma; }; I'll probably have more comments when I'll see the complete list of parameters you need to expose. > >>>>>> + */ > >>>>>> + struct prev_module_param { > >>>>>> + char version[IMP_MAX_NAME_SIZE]; > >>>>> > >>>>> Is there a need to express the version as a string instead of an > >>>>> integer ? > >>>> > >>>> It could be integer. It is generally a fixed point num, and easy to > >>>> read > >>>> it as a string than an integer. Can I keep it as a string? > >>> > >>> Let's first decide whether a version field is needed at all :-) > >> > >> Will remove. > >> > >>>>>> + unsigned short len; > >>>>>> + unsigned short module_id; > >>>>>> + void *param; > >>>>>> + }; -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html