Re: [PATCH/RFC v5 2/4] leds: implement sysfs interface locking mechanism

[Jacek Anaszewski]

Hi Bryan,

Thanks for a review.

On 09/12/2014 03:11 AM, Bryan Wu wrote:
> On Wed, Aug 20, 2014 at 6:41 AM, Jacek Anaszewski
> <j.anaszewski@xxxxxxxxxxx> wrote:
> > Add a mechanism for locking LED subsystem sysfs interface.
> > This patch prepares ground for addition of LED Flash Class
> > extension, whose API will be integrated with V4L2 Flash API.
> > Such a fusion enforces introducing a locking scheme, which
> > will secure consistent access to the LED Flash Class device.
> >
> > The mechanism being introduced allows for disabling LED
> > subsystem sysfs interface by calling led_sysfs_lock function
> > and enabling it by calling led_sysfs_unlock. The functions
> > alter the LED_SYSFS_LOCK flag state and must be called
> > under mutex lock. The state of the lock is checked with use
> > of led_sysfs_is_locked function. Such a design allows for
> > providing immediate feedback to the user space on whether
> > the LED Flash Class device is available or is under V4L2 Flash
> > sub-device control.
> >
> > Signed-off-by: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
> > Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > Cc: Bryan Wu <cooloney@xxxxxxxxx>
> > Cc: Richard Purdie <rpurdie@xxxxxxxxx>
> > ---
> >   drivers/leds/led-class.c    |   23 ++++++++++++++++++++---
> >   drivers/leds/led-core.c     |   18 ++++++++++++++++++
> >   drivers/leds/led-triggers.c |   15 ++++++++++++---
> >   include/linux/leds.h        |   32 ++++++++++++++++++++++++++++++++
> >   4 files changed, 82 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> > index 6f82a76..0bc0ba9 100644
> > --- a/drivers/leds/led-class.c
> > +++ b/drivers/leds/led-class.c
> > @@ -39,17 +39,31 @@ static ssize_t brightness_store(struct device *dev,
> >   {
> >          struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >          unsigned long state;
> > -       ssize_t ret = -EINVAL;
> > +       ssize_t ret;
> > +
> > +#ifdef CONFIG_V4L2_FLASH_LED_CLASS
>
> Can we remove this #ifdef? Following code looks good to the common LED class.

Sure.

> > +       mutex_lock(&led_cdev->led_lock);
>
> Can we choose more meaningful name instead of led_lock here?
> Then use led_sysfs_enable() instead of led_sysfs_unlock()
> led_sysfs_disable instead of led_sysfs_lock()
> led_sysfs_is_disabled instead of led_sysfs_is_locked()
>
> And the flag LED_SYSFS_LOCK -> LED_SYSFS_DISABLE
>
> I was just confused by the name lock and unlock and mutex lock.
>
> The idea looks good to me.

Indeed, this naming will be more intuitive.

Regards,
Jacek

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/