On 21/01/18 23:48, Sakari Ailus wrote: > Hi Hans, > > On Sun, Jan 21, 2018 at 11:46:46AM +0100, Hans Verkuil wrote: >> Hi Sakari, >> >> I looked a bit closer at how atomisp uses g/s_parm. They abuse the capturemode field >> to select video/preview/still modes on the sensor, which actually changes the list >> of supported resolutions. >> >> The following files use this: >> >> i2c/atomisp-gc0310.c >> i2c/atomisp-gc2235.c >> i2c/atomisp-ov2680.c >> i2c/atomisp-ov2722.c >> i2c/ov5693/atomisp-ov5693.c >> pci/atomisp2/atomisp_file.c >> pci/atomisp2/atomisp_tpg.c >> >> The last two have a dummy g/s_parm implementation, so are easy to fix. >> The gc0310 and 0v2680 have identical resolution lists for all three modes, so >> the capturemode can just be ignored and these two drivers can be simplified. >> >> Looking at the higher level code it turns out that this atomisp driver appears >> to be in the middle of a conversion from using s_parm to a V4L2_CID_RUN_MODE >> control. If the control is present, then that will be used to set the mode, >> otherwise it falls back to s_parm. >> >> So the best solution would be if Intel can convert the remaining drivers from >> using s_parm to the new control and then drop all code that uses s_parm to do >> this, so g/s_parm is then only used to get/set the framerate. >> >> Is this something you or a colleague can take on? > > I've stabbed the atomisp sensor drivers enough to remove the s_parm and > g_parm usage there. This effectively removes the s_parm abuse, as there was > nothing else it was being used for. > > The patches are here; there are no changes to your patches in the branch > you pointed me to: > > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=sparm> > > I've split dropping support for certain modes in the drivers into separate > patch; it's easy to bring them back by just reverting the patch ("staging: > atomisp: i2c: Disable non-preview configurations") or removing the ifdefs. > I don't object merging this with the previous patch either. > > What comes to the run mode control --- this logic should have always > resided in user space; that control (and s_parm hack) is basically getting > around lack of support for MC / V4L2 sub-device interface in the driver. So > that control isn't the right solution either going forward. > > Cc Andy and Alan. > Looks good. Just one note: in atomisp_ioctl.c the atomisp_g_parm function still abuses this API (setting capturemode) but more importantly, it never calls g_frame_interval. The atomisp_s_parm function *does* call s_frame_interval. So this is inconsistent. However, this was always there, so it's not something that was introduced by these changes. Regards, Hans
