Hi, Jacek! On Tue, Jun 02, 2015 at 11:13:54AM +0200, Jacek Anaszewski wrote: > Hi Sakari, > > On 06/01/2015 10:59 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Mon, May 25, 2015 at 05:13:57PM +0200, Jacek Anaszewski wrote: > >>This patch adds helper functions for registering/unregistering > >>LED Flash class devices as V4L2 sub-devices. The functions should > >>be called from the LED subsystem device driver. In case the > >>support for V4L2 Flash sub-devices is disabled in the kernel > >>config the functions' empty versions will be used. > >> > >>Signed-off-by: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx> > >>Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> > >>Cc: Sakari Ailus <sakari.ailus@xxxxxx> > >>Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx> > > > >Thanks for adding indicator support! > > > >Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > > > I missed one thing - sysfs interface of the indicator LED class > also has to be disabled/enabled of v4l2_flash_open/close. Good catch. > > I am planning to reimplement the functions as follows, > please let me know if you see any issues here: > > static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh > *fh) > { > struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd); > > struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev; > > struct led_classdev *led_cdev = &fled_cdev->led_cdev; > struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev; > > struct led_classdev *led_cdev_ind; > > int ret = 0; I think you could spare some newlines above (and below as well). > > > mutex_lock(&led_cdev->led_access); > > > if (!v4l2_fh_is_singular(&fh->vfh)) > > goto unlock; > > > led_sysfs_disable(led_cdev); > led_trigger_remove(led_cdev); > > > if (iled_cdev) { > led_cdev_ind = &iled_cdev->led_cdev; You can also declare led_cdev_ind here as you don't need it outside the basic block. > > > mutex_lock(&led_cdev_ind->led_access); > > > led_sysfs_disable(led_cdev_ind); > led_trigger_remove(led_cdev_ind); > > > mutex_unlock(&led_cdev_ind->led_access); Please don't acquire the indicator mutex while holding the flash mutex. I don't think there's a need to do so, thus creating a dependency between the two. Just remember to check for v4l2_fh_is_singular() in both cases. > > } > > > ret = __sync_device_with_v4l2_controls(v4l2_flash); If ret is < 0 here, you end up disabling the sysfs interface while open() fails (and v4l2_flash_close() will not be run). Shouldn't you handle that? > > > unlock: > mutex_unlock(&led_cdev->led_access); > > return ret; > > } > > > static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh > *fh) > { > struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd); > > struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev; > > struct led_classdev *led_cdev = &fled_cdev->led_cdev; > struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev; > > struct led_classdev *led_cdev_ind; > > int ret = 0; > > > mutex_lock(&led_cdev->led_access); > > > if (v4l2_fh_is_singular(&fh->vfh)) { > if (v4l2_flash->ctrls[STROBE_SOURCE]) > ret = v4l2_ctrl_s_ctrl(v4l2_flash->ctrls[STROBE_SV4L2_FLASH_STROBE_SOURCE_SOFTWARE); > > led_sysfs_enable(led_cdev); > > > if (iled_cdev) { > led_cdev_ind = &iled_cdev->led_cdev; > > > mutex_lock(&led_cdev_ind->led_access); Ditto. > > > led_sysfs_enable(led_cdev_ind); > > > mutex_unlock(&led_cdev_ind->led_access); > > } > > > } > > > mutex_unlock(&led_cdev->led_access); > > > return ret; > > } > > -- Kind regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html