Hi Jacek,
On Mon, Jun 08, 2015 at 09:21:10AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
>
> On 06/03/2015 10:59 PM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >On Wed, Jun 03, 2015 at 09:56:39AM +0200, Jacek Anaszewski wrote:
> >>Hi Sakari,
> >>
> >>On 06/02/2015 05:32 PM, Sakari Ailus wrote:
> >>>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 <[email protected]>
> >>>>>>Acked-by: Kyungmin Park <[email protected]>
> >>>>>>Cc: Sakari Ailus <[email protected]>
> >>>>>>Cc: Hans Verkuil <[email protected]>
> >>>>>
> >>>>>Thanks for adding indicator support!
> >>>>>
> >>>>>Acked-by: Sakari Ailus <[email protected]>
> >>>>>
> >>>>
> >>>>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).
> >>
> >>There must have been some issue on thunderbird side,
> >>originally there were no newlines above.
> >
> >Ok. I've used Seamonkey, I believe it uses the same editor. Some bugs were
> >unintroduced a few years ago, and since that I've mostly used a different
> >editor (and MUA) when replying to patches.
> >
> >>>>
> >>>>
> >>>> 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.
> >>
> >>With new approach it will be required also in error path.
> >
> >True.
> >
> >>>>
> >>>>
> >>>> 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.
> >>
> >>I thought that the code would be simpler this way. Nevertheless I
> >>produced a new version by following your advise.
> >>
> >>>
> >>>>
> >>>> }
> >>>>
> >>>>
> >>>> 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?
> >>
> >>Good point.
> >>
> >>Please find the new version of v4l2_flash{open|close} functions below:
> >>
> >>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 = NULL;
> >> int ret = 0;
> >>
> >> mutex_lock(&led_cdev->led_access);
> >>
> >> if (!v4l2_fh_is_singular(&fh->vfh)) {
> >
> >Hmm. I just realised that this might be a bit broken ---
> >v4l2_fh_is_singular() should return the same value independently of where in
> >subdev's open() handler it is called.
> >
> >But adding file handles to the list and checking how many there are of them
> >is not properly serialised, i.e. another process could change the list in
> >between. I'll try to submit a patch for that. No need to wait for that
> >though.
> >
> >So please ignore my earlier request to check for v4l2_fh_is_singular() for
> >multiple times, it won't help. Once is enough, albeit more than that won't
> >hurt.
>
> Let's agree on the new shape of the functions then:
Looks good to me. Could you resend once more, please?
> tatic 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 = NULL;
> int ret = 0;
>
> if (!v4l2_fh_is_singular(&fh->vfh))
> return 0;
>
> mutex_lock(&led_cdev->led_access);
>
> led_sysfs_disable(led_cdev);
> led_trigger_remove(led_cdev);
>
> mutex_unlock(&led_cdev->led_access);
>
> if (iled_cdev) {
> led_cdev_ind = &iled_cdev->led_cdev;
>
> 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);
> }
>
> ret = __sync_device_with_v4l2_controls(v4l2_flash);
> if (ret < 0)
> goto out_sync_device;
>
> return 0;
> out_sync_device:
> mutex_lock(&led_cdev->led_access);
> led_sysfs_enable(led_cdev);
> mutex_unlock(&led_cdev->led_access);
>
> if (led_cdev_ind) {
> mutex_lock(&led_cdev_ind->led_access);
> led_sysfs_enable(led_cdev_ind);
> mutex_unlock(&led_cdev_ind->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;
> int ret = 0;
>
> if (!v4l2_fh_is_singular(&fh->vfh))
> return 0;
>
> mutex_lock(&led_cdev->led_access);
>
> if (v4l2_flash->ctrls[STROBE_SOURCE])
> ret = v4l2_ctrl_s_ctrl(v4l2_flash->ctrls[STROBE_SOURCE],
> V4L2_FLASH_STROBE_SOURCE_SOFTWARE);
> led_sysfs_enable(led_cdev);
>
> mutex_unlock(&led_cdev->led_access);
>
> if (iled_cdev) {
> struct led_classdev *led_cdev_ind = &iled_cdev->led_cdev;
>
> mutex_lock(&led_cdev_ind->led_access);
> led_sysfs_enable(led_cdev_ind);
> mutex_unlock(&led_cdev_ind->led_access);
> }
>
> return ret;
> }
>
>
--
Kind regards,
Sakari Ailus
e-mail: [email protected] XMPP: [email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
