Re: [RFC, PATCH 1/3] gspca: add LEDs subsystem connection

2010-03-09 Thread Richard Purdie
On Tue, 2010-03-09 at 12:27 +0100, Laurent Pinchart wrote:
 Hi Màrton,
 
 Thanks for the patch.
 
 On Wednesday 03 March 2010 00:42:23 Németh Márton wrote:
  From: Márton Németh nm...@freemail.hu
  
  On some webcams one or more LEDs can be found. One type of these LEDs
  are feedback LEDs: they usually shows the state of streaming mode.
  The LED can be programmed to constantly switched off state (e.g. for
  power saving reasons, preview mode) or on state (e.g. the application
  shows motion detection or on-air).
  
  The second type of LEDs are used to create enough light for the sensor
  for example visible or in infra-red light.
  
  Both type of these LEDs can be handled using the LEDs subsystem. This
  patch add support to connect a gspca based driver to the LEDs subsystem.
 
 They can indeed, but I'm not sure if the LEDs subsystem was designed for that 
 kind of use cases.

The LED subsystem was designed to support LED lights and the above
scenarios can certainly fit that. It provides a nice system where
default use cases should just work (power light on a laptop) but the
system has the control to override than and do other interesting things
with it if it so wishes.

 The LED framework was developed to handle LEDs found in embedded systems 
 (usually connected to GPIOs) that needed to be connected to software triggers 
 or controlled by drivers and/or specific userspace applications.

Its used by many laptops so its not just embedded systems.

  Webcam LEDs 
 seem a bit out of scope to me, especially the light LED that might be 
 better 
 handled by a V4L2 set of controls (we're currently missing controls for 
 camera 
 flashes, be they LEDs or Xenon based).
 
 I'll let Richard speak on this.

I'm not going to push one way or another and its up to individual
subsystems to way up the benefits and drawbacks and make their decision.
There is nothing in the design of the system which would stop it working
or being used in this case though. If the susystsme needs improvements
to work well, I'm happy to accept patches too.

Cheers,

Richard





--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC, PATCH 1/3] gspca: add LEDs subsystem connection

2010-03-02 Thread Németh Márton
From: Márton Németh nm...@freemail.hu

On some webcams one or more LEDs can be found. One type of these LEDs
are feedback LEDs: they usually shows the state of streaming mode.
The LED can be programmed to constantly switched off state (e.g. for
power saving reasons, preview mode) or on state (e.g. the application
shows motion detection or on-air).

The second type of LEDs are used to create enough light for the sensor
for example visible or in infra-red light.

Both type of these LEDs can be handled using the LEDs subsystem. This
patch add support to connect a gspca based driver to the LEDs subsystem.

Signed-off-by: Márton Németh nm...@freemail.hu
---
diff -r d8fafa7d88dc linux/drivers/media/video/gspca/gspca.h
--- a/linux/drivers/media/video/gspca/gspca.h   Thu Feb 18 19:02:51 2010 +0100
+++ b/linux/drivers/media/video/gspca/gspca.h   Wed Mar 03 00:10:19 2010 +0100
@@ -7,6 +7,8 @@
 #include linux/videodev2.h
 #include media/v4l2-common.h
 #include linux/mutex.h
+#include linux/leds.h
+#include linux/workqueue.h
 #include compat.h

 /* compilation option */
@@ -53,14 +55,41 @@
int nrates;
 };

+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+/**
+ * struct gspca_led - data structure for one LED on a camera
+ * @led_cdev: the class device for the LED
+ * @name: place for the LED name which will appear under /sys/class/leds/
+ * @led_trigger: the trigger structure for the same LED
+ * @trigger_name: place for the trigger name which will appear in the file
+ */sys/class/leds/{led name}/trigger
+ * @parent: pointer to the parent gspca_dev
+ */
+struct gspca_led {
+   struct led_classdev led_cdev;
+   char name[32];
+#ifdef CONFIG_LEDS_TRIGGERS
+   struct led_trigger led_trigger;
+   char trigger_name[32];
+#endif
+   struct gspca_dev *parent;
+};
+#endif
+
 /* device information - set at probe time */
 struct cam {
const struct v4l2_pix_format *cam_mode; /* size nmodes */
const struct framerates *mode_framerates; /* must have size nmode,
   * just like cam_mode */
+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+   struct gspca_led *leds;
+#endif
u32 bulk_size;  /* buffer size when image transfer by bulk */
u32 input_flags;/* value for ENUM_INPUT status flags */
u8 nmodes;  /* size of cam_mode */
+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+   u8 nleds;   /* number of LEDs */
+#endif
u8 no_urb_create;   /* don't create transfer URBs */
u8 bulk_nurbs;  /* number of URBs in bulk mode
 * - cannot be  MAX_NURBS
diff -r d8fafa7d88dc linux/drivers/media/video/gspca/gspca.c
--- a/linux/drivers/media/video/gspca/gspca.c   Thu Feb 18 19:02:51 2010 +0100
+++ b/linux/drivers/media/video/gspca/gspca.c   Wed Mar 03 00:10:19 2010 +0100
@@ -4,6 +4,7 @@
  * Copyright (C) 2008-2009 Jean-Francois Moine (http://moinejf.free.fr)
  *
  * Camera button input handling by Márton Németh
+ * LED subsystem connection by Márton Németh
  * Copyright (C) 2009-2010 Márton Németh nm...@freemail.hu
  *
  * This program is free software; you can redistribute it and/or modify it
@@ -2256,6 +2257,76 @@
.release = gspca_release,
 };

+#if defined(CONFIG_LEDS_CLASS) || defined(CONFIG_LEDS_CLASS_MODULE)
+/**
+ * gspca_create_name() - Create a name using format string and a number
+ * @name: format string on input, e.g. video%d::feedback. The format string
+ *may contain one and only one %d format specifier. The created name
+ *will be put on the same location and the original format string will
+ *be overwritten.
+ * @length: the length of the buffer in bytes pointed by the name parameter
+ * @num: the number to use when creating the name
+ *
+ * Returns zero on success.
+ */
+static int gspca_create_name(char *name, unsigned int length, int num)
+{
+   char buffer[32];
+   unsigned int l;
+
+   /* TODO: check format string: it shall contain one and only one %d */
+
+   l = min(length, sizeof(buffer));
+   snprintf(buffer, l, name, num);
+   strlcpy(name, buffer, l);
+
+   return 0;
+}
+
+static void gspca_leds_register(struct gspca_dev *gspca_dev) {
+   int i;
+   int ret;
+
+   for (i = 0; i  gspca_dev-cam.nleds; i++) {
+#ifdef CONFIG_LEDS_TRIGGERS
+   gspca_create_name(gspca_dev-cam.leds[i].trigger_name,
+ sizeof(gspca_dev-cam.leds[i].trigger_name),
+ gspca_dev-vdev.num);
+   gspca_dev-cam.leds[i].led_trigger.name = 
gspca_dev-cam.leds[i].trigger_name;
+   gspca_dev-cam.leds[i].led_cdev.default_trigger = 
gspca_dev-cam.leds[i].trigger_name;
+#endif
+   gspca_create_name(gspca_dev-cam.leds[i].name,
+