On 08/14/2014 06:34 AM, Sakari Ailus wrote:

Hi Jacek,

On Mon, Aug 11, 2014 at 03:27:22PM +0200, Jacek Anaszewski wrote:

...

quoted

quoted

quoted

quoted

quoted

diff --git a/include/media/v4l2-flash.h b/include/media/v4l2-flash.h
new file mode 100644
index 0000000..effa46b
--- /dev/null
+++ b/include/media/v4l2-flash.h

@@ -0,0 +1,137 @@
+/*
+ * V4L2 Flash LED sub-device registration helpers.
+ *
+ *	Copyright (C) 2014 Samsung Electronics Co., Ltd
+ *	Author: Jacek Anaszewski <j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation."
+ */
+
+#ifndef _V4L2_FLASH_H
+#define _V4L2_FLASH_H
+
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-dev.h>le
+#include <media/v4l2-event.h>
+#include <media/v4l2-ioctl.h>
+
+struct led_classdev_flash;
+struct led_classdev;
+enum led_brightness;
+
+struct v4l2_flash_ops {
+	int (*torch_brightness_set)(struct led_classdev *led_cdev,
+					enum led_brightness brightness);
+	int (*torch_brightness_update)(struct led_classdev *led_cdev);
+	int (*flash_brightness_set)(struct led_classdev_flash *flash,
+					u32 brightness);
+	int (*flash_brightness_update)(struct led_classdev_flash *flash);
+	int (*strobe_set)(struct led_classdev_flash *flash, bool state);
+	int (*strobe_get)(struct led_classdev_flash *flash, bool *state);
+	int (*timeout_set)(struct led_classdev_flash *flash, u32 timeout);
+	int (*indicator_brightness_set)(struct led_classdev_flash *flash,
+					u32 brightness);
+	int (*indicator_brightness_update)(struct led_classdev_flash *flash);
+	int (*external_strobe_set)(struct led_classdev_flash *flash,
+					bool enable);
+	int (*fault_get)(struct led_classdev_flash *flash, u32 *fault);
+	void (*sysfs_lock)(struct led_classdev *led_cdev);
+	void (*sysfs_unlock)(struct led_classdev *led_cdev);

These functions are not driver specific and there's going to be just one
implementation (I suppose). Could you refresh my memory regarding why
the LED framework functions aren't called directly?

These ops are required to make possible building led-class-flash as
a kernel module.

Assuming you'd use the actual implementation directly, what would be the
dependencies? I don't think the LED flash framework has any callbacks
towards the V4L2 (LED) flash framework, does it? Please correct my
understanding if I'm missing something. In Makefile format, assume all
targets are .PHONY:

led-flash-api: led-api

v4l2-flash: led-flash-api

driver: led-flash-api v4l2-flash

LED Class Flash driver gains V4L2 Flash API when
CONFIG_V4L2_FLASH_LED_CLASS is defined. This is accomplished in
the probe function by either calling v4l2_flash_init function
or the macro of this name, when the CONFIG_V4L2_FLASH_LED_CLASS
macro isn't defined.

If the v4l2-flash.c was to call the LED API directly, then the
led-class-flash module symbols would have to be available at
v4l2-flash.o linking time.

Is this an issue? EXPORT_SYMBOL_GPL() for the relevant symbols should be
enough.

It isn't enough. If I call e.g. led_set_flash_brightness
directly from v4l2-flash.c and configure led-class-flash to be built as
a module then I am getting "undefined reference to
led_set_flash_brightness" error during linking phase.

It happens because the linker doesn't take into account
led-flash-class.ko symbols. It is reasonable because initially
the kernel boots up without led-flash-class.ko module and
the processor wouldn't know the address to jump to in the
result of calling a led API function.
The led-class-flash.ko binary code is loaded into memory not
sooner than after executing "insmod led-class-flash.ko".

After linking dynamically with kernel the LED API function
addresses are relocated, and the LED Flash Class core can
initialize the v4l2_flash_ops structure. Then every LED Flash Class
driver can obtain the address of this structure with
led_get_v4l2_flash_ops and pass it to the v4l2_flash_init.

quoted

This requirement cannot be met if the led-class-flash is built
as a module.

Use of function pointers in the v4l2-flash.c allows to compile it
into the kernel and enables the possibility of adding the V4L2 Flash
support conditionally, during driver probing.

I'd simply decide this during kernel compilation time. If you want
something, just enable it. v4l2_flash_init() is called directly by the
driver in any case, so unless that is also called through a wrapper the
driver is still directly dependent on it.

The problem is that v4l2-flash.o would have to depend on
led-class-flash.o, which when built as a module isn't available
during v4l2-flash.o linking time. In order to avoid v4l2-flash.o linking
problem, it would have to be built as a module.

Nevertheless, in this arrangement, the CONFIG_V4L2_FLASH_LED_CLASS
macro would be defined only in v4l2-flash.ko module, and
a LED Flash Class driver couldn't check whether V4L2 Flash support
is enabled. Its dependence on v4l2-flash.o would have to be fixed,
which is not what we want.

I have tested all these cases.

Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html