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 linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
