Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
On Fri, May 09, 2014 at 09:18:55AM +0200, Jacek Anaszewski wrote:
Hi Sakari,
On 05/07/2014 09:58 AM, Sakari Ailus wrote:
Hi Jacek,
On Wed, May 07, 2014 at 09:20:17AM +0200, Jacek Anaszewski wrote:
On 05/06/2014 11:10 AM, Sakari Ailus wrote:
Hi Jacek,
On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote:
Hi Sakari,
On 05/02/2014 01:06 PM, Sakari Ailus wrote:
[...]
+static inline enum led_brightness
v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
Brightness level 0 means that led is off. Let's consider following
case:
intensity - 15625
config-step - 15625
intensity / config-step = 1 (the lowest possible current level)
In V4L2 controls the minimum is not off, and zero might not be a
possible
value since minimum isn't divisible by step.
I wonder how to best take that into account.
I've assumed that in MODE_TORCH a led is always on. Switching
the mode to MODE_FLASH or MODE_OFF turns the led off.
This way we avoid the problem with converting 0 uA value to
led_brightness, as available torch brightness levels start from
the minimum current level value and turning the led off is
accomplished on transition to MODE_OFF or MODE_FLASH, by
calling brightness_set op with led_brightness = 0.
I'm not sure if we understood the issue the same way. My concern was
that if
the intensity isn't a multiple of step (but intensity - min is), the
above
formula won't return a valid result (unless I miss something).
Please note that v4l2_flash_intensity_to_led_brightness is called only
from s_ctrl callback, and thus it expects to get the intensity aligned
to the step value, so it will always be a multiple of step.
Is it possible that s_ctrl callback would be passed a non-aligned
control value?
In a nutshell: value - min is aligned but value is not. Please see
validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c .
Still, to my mind, value is aligned.
Below I execute the calculation steps one by one
according to the V4L2_CTRL_TYPE_INTEGER case in the
validate_new function:
c-value = 35000
val = c-value + step / 2; // 35000 + 15625 / 2 = 42812
val = clamp(val, min, max); // val = 42812
offset = val - min; // 42812 - 15625 = 27187
offset = step * (offset / step); // 15625 * (27187 / 15625) = 15625
c-value = min + offset; // 15625 + 15625 = 31250
Value is aligned to the nearest step.
Please spot any discrepancies in my way of thinking if there
are any :)
min is aligned to step above. This is not necessarily the case. And if min
is not aligned, neither is value.
Thanks for spotting this. Below are improved versions of the conversion
functions. Please let me know if you have any comments.
static inline
enum led_brightnessv4l2_flash_intensity_to_led_brightness(
struct led_ctrl *config,
u32 intensity)
{
return ((intensity - config-min) / config-step) + 1;
}
static inline
u32 v4l2_flash_led_brightness_to_intensity(
struct led_ctrl *config,
enum led_brightness brightness)
{
return ((brightness - 1) * config-step) + config-min;
V4L2 control integer values are signed, thus s32 instead of u32. Otherwise
looks good to me.
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
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
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Sakari,
On 05/07/2014 09:58 AM, Sakari Ailus wrote:
Hi Jacek,
On Wed, May 07, 2014 at 09:20:17AM +0200, Jacek Anaszewski wrote:
On 05/06/2014 11:10 AM, Sakari Ailus wrote:
Hi Jacek,
On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote:
Hi Sakari,
On 05/02/2014 01:06 PM, Sakari Ailus wrote:
[...]
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
Brightness level 0 means that led is off. Let's consider following case:
intensity - 15625
config-step - 15625
intensity / config-step = 1 (the lowest possible current level)
In V4L2 controls the minimum is not off, and zero might not be a possible
value since minimum isn't divisible by step.
I wonder how to best take that into account.
I've assumed that in MODE_TORCH a led is always on. Switching
the mode to MODE_FLASH or MODE_OFF turns the led off.
This way we avoid the problem with converting 0 uA value to
led_brightness, as available torch brightness levels start from
the minimum current level value and turning the led off is
accomplished on transition to MODE_OFF or MODE_FLASH, by
calling brightness_set op with led_brightness = 0.
I'm not sure if we understood the issue the same way. My concern was that if
the intensity isn't a multiple of step (but intensity - min is), the above
formula won't return a valid result (unless I miss something).
Please note that v4l2_flash_intensity_to_led_brightness is called only
from s_ctrl callback, and thus it expects to get the intensity aligned
to the step value, so it will always be a multiple of step.
Is it possible that s_ctrl callback would be passed a non-aligned
control value?
In a nutshell: value - min is aligned but value is not. Please see
validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c .
Still, to my mind, value is aligned.
Below I execute the calculation steps one by one
according to the V4L2_CTRL_TYPE_INTEGER case in the
validate_new function:
c-value = 35000
val = c-value + step / 2; // 35000 + 15625 / 2 = 42812
val = clamp(val, min, max); // val = 42812
offset = val - min; // 42812 - 15625 = 27187
offset = step * (offset / step); // 15625 * (27187 / 15625) = 15625
c-value = min + offset; // 15625 + 15625 = 31250
Value is aligned to the nearest step.
Please spot any discrepancies in my way of thinking if there
are any :)
min is aligned to step above. This is not necessarily the case. And if min
is not aligned, neither is value.
Thanks for spotting this. Below are improved versions of the conversion
functions. Please let me know if you have any comments.
static inline
enum led_brightnessv4l2_flash_intensity_to_led_brightness(
struct led_ctrl *config,
u32 intensity)
{
return ((intensity - config-min) / config-step) + 1;
}
static inline
u32 v4l2_flash_led_brightness_to_intensity(
struct led_ctrl *config,
enum led_brightness brightness)
{
return ((brightness - 1) * config-step) + config-min;
}
Regards,
Jacek Anaszewski
--
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
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
On 05/06/2014 11:10 AM, Sakari Ailus wrote:
Hi Jacek,
On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote:
Hi Sakari,
On 05/02/2014 01:06 PM, Sakari Ailus wrote:
[...]
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
Brightness level 0 means that led is off. Let's consider following case:
intensity - 15625
config-step - 15625
intensity / config-step = 1 (the lowest possible current level)
In V4L2 controls the minimum is not off, and zero might not be a possible
value since minimum isn't divisible by step.
I wonder how to best take that into account.
I've assumed that in MODE_TORCH a led is always on. Switching
the mode to MODE_FLASH or MODE_OFF turns the led off.
This way we avoid the problem with converting 0 uA value to
led_brightness, as available torch brightness levels start from
the minimum current level value and turning the led off is
accomplished on transition to MODE_OFF or MODE_FLASH, by
calling brightness_set op with led_brightness = 0.
I'm not sure if we understood the issue the same way. My concern was that if
the intensity isn't a multiple of step (but intensity - min is), the above
formula won't return a valid result (unless I miss something).
Please note that v4l2_flash_intensity_to_led_brightness is called only
from s_ctrl callback, and thus it expects to get the intensity aligned
to the step value, so it will always be a multiple of step.
Is it possible that s_ctrl callback would be passed a non-aligned
control value?
In a nutshell: value - min is aligned but value is not. Please see
validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c .
Still, to my mind, value is aligned.
Below I execute the calculation steps one by one
according to the V4L2_CTRL_TYPE_INTEGER case in the
validate_new function:
c-value = 35000
val = c-value + step / 2; // 35000 + 15625 / 2 = 42812
val = clamp(val, min, max); // val = 42812
offset = val - min; // 42812 - 15625 = 27187
offset = step * (offset / step); // 15625 * (27187 / 15625) = 15625
c-value = min + offset; // 15625 + 15625 = 31250
Value is aligned to the nearest step.
Please spot any discrepancies in my way of thinking if there
are any :)
Regards,
Jacek Anaszewski
--
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
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
On Wed, May 07, 2014 at 09:20:17AM +0200, Jacek Anaszewski wrote:
On 05/06/2014 11:10 AM, Sakari Ailus wrote:
Hi Jacek,
On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote:
Hi Sakari,
On 05/02/2014 01:06 PM, Sakari Ailus wrote:
[...]
+static inline enum led_brightness
v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
Brightness level 0 means that led is off. Let's consider following case:
intensity - 15625
config-step - 15625
intensity / config-step = 1 (the lowest possible current level)
In V4L2 controls the minimum is not off, and zero might not be a possible
value since minimum isn't divisible by step.
I wonder how to best take that into account.
I've assumed that in MODE_TORCH a led is always on. Switching
the mode to MODE_FLASH or MODE_OFF turns the led off.
This way we avoid the problem with converting 0 uA value to
led_brightness, as available torch brightness levels start from
the minimum current level value and turning the led off is
accomplished on transition to MODE_OFF or MODE_FLASH, by
calling brightness_set op with led_brightness = 0.
I'm not sure if we understood the issue the same way. My concern was that
if
the intensity isn't a multiple of step (but intensity - min is), the above
formula won't return a valid result (unless I miss something).
Please note that v4l2_flash_intensity_to_led_brightness is called only
from s_ctrl callback, and thus it expects to get the intensity aligned
to the step value, so it will always be a multiple of step.
Is it possible that s_ctrl callback would be passed a non-aligned
control value?
In a nutshell: value - min is aligned but value is not. Please see
validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c .
Still, to my mind, value is aligned.
Below I execute the calculation steps one by one
according to the V4L2_CTRL_TYPE_INTEGER case in the
validate_new function:
c-value = 35000
val = c-value + step / 2; // 35000 + 15625 / 2 = 42812
val = clamp(val, min, max); // val = 42812
offset = val - min; // 42812 - 15625 = 27187
offset = step * (offset / step); // 15625 * (27187 / 15625) = 15625
c-value = min + offset; // 15625 + 15625 = 31250
Value is aligned to the nearest step.
Please spot any discrepancies in my way of thinking if there
are any :)
min is aligned to step above. This is not necessarily the case. And if min
is not aligned, neither is value.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
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
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Sakari,
On 05/02/2014 01:06 PM, Sakari Ailus wrote:
[...]
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
Brightness level 0 means that led is off. Let's consider following case:
intensity - 15625
config-step - 15625
intensity / config-step = 1 (the lowest possible current level)
In V4L2 controls the minimum is not off, and zero might not be a possible
value since minimum isn't divisible by step.
I wonder how to best take that into account.
I've assumed that in MODE_TORCH a led is always on. Switching
the mode to MODE_FLASH or MODE_OFF turns the led off.
This way we avoid the problem with converting 0 uA value to
led_brightness, as available torch brightness levels start from
the minimum current level value and turning the led off is
accomplished on transition to MODE_OFF or MODE_FLASH, by
calling brightness_set op with led_brightness = 0.
I'm not sure if we understood the issue the same way. My concern was that if
the intensity isn't a multiple of step (but intensity - min is), the above
formula won't return a valid result (unless I miss something).
Please note that v4l2_flash_intensity_to_led_brightness is called only
from s_ctrl callback, and thus it expects to get the intensity aligned
to the step value, so it will always be a multiple of step.
Is it possible that s_ctrl callback would be passed a non-aligned
control value?
Regards,
Jacek Anaszewski
--
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
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
On Tue, May 06, 2014 at 08:44:41AM +0200, Jacek Anaszewski wrote:
Hi Sakari,
On 05/02/2014 01:06 PM, Sakari Ailus wrote:
[...]
+static inline enum led_brightness
v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
Brightness level 0 means that led is off. Let's consider following case:
intensity - 15625
config-step - 15625
intensity / config-step = 1 (the lowest possible current level)
In V4L2 controls the minimum is not off, and zero might not be a possible
value since minimum isn't divisible by step.
I wonder how to best take that into account.
I've assumed that in MODE_TORCH a led is always on. Switching
the mode to MODE_FLASH or MODE_OFF turns the led off.
This way we avoid the problem with converting 0 uA value to
led_brightness, as available torch brightness levels start from
the minimum current level value and turning the led off is
accomplished on transition to MODE_OFF or MODE_FLASH, by
calling brightness_set op with led_brightness = 0.
I'm not sure if we understood the issue the same way. My concern was that if
the intensity isn't a multiple of step (but intensity - min is), the above
formula won't return a valid result (unless I miss something).
Please note that v4l2_flash_intensity_to_led_brightness is called only
from s_ctrl callback, and thus it expects to get the intensity aligned
to the step value, so it will always be a multiple of step.
Is it possible that s_ctrl callback would be passed a non-aligned
control value?
In a nutshell: value - min is aligned but value is not. Please see
validate_new() in drivers/media/v4l2-core/v4l2-ctrls.c .
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
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
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
On Mon, Apr 28, 2014 at 01:25:09PM +0200, Jacek Anaszewski wrote:
Hi Sakari,
On 04/23/2014 05:24 PM, Sakari Ailus wrote:
Hi Jacek,
On Thu, Apr 17, 2014 at 10:26:44AM +0200, Jacek Anaszewski wrote:
Hi Sakari,
Thanks for the review.
On 04/16/2014 08:21 PM, Sakari Ailus wrote:
Hi Jacek,
Thanks for the update!
[...]
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
Brightness level 0 means that led is off. Let's consider following case:
intensity - 15625
config-step - 15625
intensity / config-step = 1 (the lowest possible current level)
In V4L2 controls the minimum is not off, and zero might not be a possible
value since minimum isn't divisible by step.
I wonder how to best take that into account.
I've assumed that in MODE_TORCH a led is always on. Switching
the mode to MODE_FLASH or MODE_OFF turns the led off.
This way we avoid the problem with converting 0 uA value to
led_brightness, as available torch brightness levels start from
the minimum current level value and turning the led off is
accomplished on transition to MODE_OFF or MODE_FLASH, by
calling brightness_set op with led_brightness = 0.
I'm not sure if we understood the issue the same way. My concern was that if
the intensity isn't a multiple of step (but intensity - min is), the above
formula won't return a valid result (unless I miss something).
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
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
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Sakari,
On 04/23/2014 05:24 PM, Sakari Ailus wrote:
Hi Jacek,
On Thu, Apr 17, 2014 at 10:26:44AM +0200, Jacek Anaszewski wrote:
Hi Sakari,
Thanks for the review.
On 04/16/2014 08:21 PM, Sakari Ailus wrote:
Hi Jacek,
Thanks for the update!
[...]
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
Brightness level 0 means that led is off. Let's consider following case:
intensity - 15625
config-step - 15625
intensity / config-step = 1 (the lowest possible current level)
In V4L2 controls the minimum is not off, and zero might not be a possible
value since minimum isn't divisible by step.
I wonder how to best take that into account.
I've assumed that in MODE_TORCH a led is always on. Switching
the mode to MODE_FLASH or MODE_OFF turns the led off.
This way we avoid the problem with converting 0 uA value to
led_brightness, as available torch brightness levels start from
the minimum current level value and turning the led off is
accomplished on transition to MODE_OFF or MODE_FLASH, by
calling brightness_set op with led_brightness = 0.
[...]
+/*
+ * V4L2 subdev internal operations
+ */
+
+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 *led_cdev = v4l2_flash-led_cdev;
+
+ mutex_lock(led_cdev-led_lock);
+ v4l2_call_flash_op(sysfs_lock, led_cdev);
Have you thought about device power management yet?
Having in mind that the V4L2 Flash sub-device is only a wrapper
for LED driver, shouldn't power management be left to the
drivers?
How does the LED controller driver know it needs to power the device up in
that case?
I think an s_power() op which uses PM runtime to set the power state until
V4L2 sub-device switches to it should be enough. But I'm fine leaving it out
from this patchset.
This solution looks reasonable.
Regards,
Jacek Anaszewski
--
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
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
On Thu, Apr 17, 2014 at 10:26:44AM +0200, Jacek Anaszewski wrote:
Hi Sakari,
Thanks for the review.
On 04/16/2014 08:21 PM, Sakari Ailus wrote:
Hi Jacek,
Thanks for the update!
[...]
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
Brightness level 0 means that led is off. Let's consider following case:
intensity - 15625
config-step - 15625
intensity / config-step = 1 (the lowest possible current level)
In V4L2 controls the minimum is not off, and zero might not be a possible
value since minimum isn't divisible by step.
I wonder how to best take that into account.
+}
+
+static inline u32 v4l2_flash_led_brightness_to_intensity(
+ struct led_ctrl *config,
+ enum led_brightness brightness)
+{
+ return brightness * config-step;
And do the opposite here?
..
+ return -EINVAL;
+ return v4l2_call_flash_op(strobe_set, led_cdev, true);
+ case V4L2_CID_FLASH_STROBE_STOP:
+ return v4l2_call_flash_op(strobe_set, led_cdev, false);
+ case V4L2_CID_FLASH_TIMEOUT:
+ ret = v4l2_call_flash_op(timeout_set, led_cdev, c-val);
+ case V4L2_CID_FLASH_INTENSITY:
+ /* no conversion is needed */
+ return v4l2_call_flash_op(flash_brightness_set, led_cdev,
+ c-val);
+ case V4L2_CID_FLASH_INDICATOR_INTENSITY:
+ /* no conversion is needed */
+ return v4l2_call_flash_op(indicator_brightness_set, led_cdev,
+ c-val);
+ case V4L2_CID_FLASH_TORCH_INTENSITY:
+ if (ctrl-led_mode-val == V4L2_FLASH_LED_MODE_TORCH) {
+ torch_brightness =
+ v4l2_flash_intensity_to_led_brightness(
+ led_cdev-brightness_ctrl,
+ ctrl-torch_intensity-val);
+ v4l2_call_flash_op(brightness_set, led_cdev,
+ torch_brightness);
I could be missing something but don't torch and indicator require similar
handling?
Why? Torch units need conversion whereas indicator units don't.
Moreover they have different LED API.
I missed it was already in micro-Amps.
+ }
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static const struct v4l2_ctrl_ops v4l2_flash_ctrl_ops = {
+ .g_volatile_ctrl = v4l2_flash_g_volatile_ctrl,
+ .s_ctrl = v4l2_flash_s_ctrl,
+};
+
+static int v4l2_flash_init_controls(struct v4l2_flash *v4l2_flash)
+
+{
+ struct led_classdev *led_cdev = v4l2_flash-led_cdev;
+ struct led_flash *flash = led_cdev-flash;
+ bool has_indicator = flash-indicator_brightness;
+ struct v4l2_ctrl *ctrl;
+ struct led_ctrl *ctrl_cfg;
+ unsigned int mask;
+ int ret, max, num_ctrls;
+
+ num_ctrls = flash-has_flash_led ? 8 : 2;
+ if (flash-fault_flags)
+ ++num_ctrls;
+ if (has_indicator)
+ ++num_ctrls;
+
+ v4l2_ctrl_handler_init(v4l2_flash-hdl, num_ctrls);
+
+ mask = 1 V4L2_FLASH_LED_MODE_NONE |
+ 1 V4L2_FLASH_LED_MODE_TORCH;
+ if (flash-has_flash_led)
+ mask |= 1 V4L2_FLASH_LED_MODE_FLASH;
+
+ /* Configure FLASH_LED_MODE ctrl */
+ v4l2_flash-ctrl.led_mode = v4l2_ctrl_new_std_menu(
+ v4l2_flash-hdl,
+ v4l2_flash_ctrl_ops, V4L2_CID_FLASH_LED_MODE,
+ V4L2_FLASH_LED_MODE_TORCH, ~mask,
+ V4L2_FLASH_LED_MODE_NONE);
+
+ /* Configure TORCH_INTENSITY ctrl */
+ ctrl_cfg = led_cdev-brightness_ctrl;
+ ctrl = v4l2_ctrl_new_std(v4l2_flash-hdl, v4l2_flash_ctrl_ops,
+V4L2_CID_FLASH_TORCH_INTENSITY,
+ctrl_cfg-min, ctrl_cfg-max,
+ctrl_cfg-step, ctrl_cfg-val);
+ if (ctrl)
+ ctrl-flags |= V4L2_CTRL_FLAG_VOLATILE;
+ v4l2_flash-ctrl.torch_intensity = ctrl;
+
+ if (flash-has_flash_led) {
+ /* Configure FLASH_STROBE_SOURCE ctrl */
+ mask = 1 V4L2_FLASH_STROBE_SOURCE_SOFTWARE;
+
+ if (flash-has_external_strobe) {
+ mask |= 1 V4L2_FLASH_STROBE_SOURCE_EXTERNAL;
+ max = V4L2_FLASH_STROBE_SOURCE_EXTERNAL;
+ } else {
+ max = V4L2_FLASH_STROBE_SOURCE_SOFTWARE;
+ }
+
+ v4l2_flash-ctrl.source = v4l2_ctrl_new_std_menu(
+ v4l2_flash-hdl,
+ v4l2_flash_ctrl_ops,
+
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Sakari,
Thanks for the review.
On 04/16/2014 08:21 PM, Sakari Ailus wrote:
Hi Jacek,
Thanks for the update!
[...]
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
Brightness level 0 means that led is off. Let's consider following case:
intensity - 15625
config-step - 15625
intensity / config-step = 1 (the lowest possible current level)
+}
+
+static inline u32 v4l2_flash_led_brightness_to_intensity(
+ struct led_ctrl *config,
+ enum led_brightness brightness)
+{
+ return brightness * config-step;
And do the opposite here?
+}
+
+static int v4l2_flash_g_volatile_ctrl(struct v4l2_ctrl *c)
+
+{
+ struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
+ struct led_classdev *led_cdev = v4l2_flash-led_cdev;
+ struct led_flash *flash = led_cdev-flash;
+ struct v4l2_flash_ctrl *ctrl = v4l2_flash-ctrl;
+ u32 fault;
+ int ret;
+
+ switch (c-id) {
+ case V4L2_CID_FLASH_TORCH_INTENSITY:
+ if (ctrl-led_mode-val == V4L2_FLASH_LED_MODE_TORCH) {
+ ret = v4l2_call_flash_op(brightness_update, led_cdev);
+ if (ret 0)
+ return ret;
+ ctrl-torch_intensity-val =
+ v4l2_flash_led_brightness_to_intensity(
+ led_cdev-brightness_ctrl,
+ led_cdev-brightness);
+ }
+ return 0;
+ case V4L2_CID_FLASH_INTENSITY:
+ ret = v4l2_call_flash_op(flash_brightness_update, led_cdev);
+ if (ret 0)
+ return ret;
+ /* no conversion is needed */
+ c-val = flash-brightness.val;
+ return 0;
+ case V4L2_CID_FLASH_INDICATOR_INTENSITY:
+ ret = v4l2_call_flash_op(indicator_brightness_update, led_cdev);
+ if (ret 0)
+ return ret;
+ /* no conversion is needed */
+ c-val = flash-indicator_brightness-val;
+ return 0;
+ case V4L2_CID_FLASH_STROBE_STATUS:
+ ret = v4l2_call_flash_op(strobe_get, led_cdev);
+ if (ret 0)
+ return ret;
+ c-val = !!ret;
+ return 0;
+ case V4L2_CID_FLASH_FAULT:
+ /* led faults map directly to V4L2 flash faults */
+ ret = v4l2_call_flash_op(fault_get, led_cdev, fault);
+ if (!ret)
The return value seems to come all the way from regmap_read() and the bus
related implementatio. I would just consider negative values errors, as
above.
I think that ret would be returned if it wasn't equal to 0. But indeed
it should be done the same way as for the other cases.
+ c-val = fault;
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int v4l2_flash_s_ctrl(struct v4l2_ctrl *c)
+{
+ struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
+ struct led_classdev *led_cdev = v4l2_flash-led_cdev;
+ struct v4l2_flash_ctrl *ctrl = v4l2_flash-ctrl;
+ enum led_brightness torch_brightness;
+ bool external_strobe;
+ int ret;
+
+ switch (c-id) {
+ case V4L2_CID_FLASH_LED_MODE:
+ switch (c-val) {
+ case V4L2_FLASH_LED_MODE_NONE:
+ v4l2_call_flash_op(brightness_set, led_cdev, 0);
+ return v4l2_call_flash_op(strobe_set, led_cdev, false);
+ case V4L2_FLASH_LED_MODE_FLASH:
+ /* Turn off torch LED */
+ v4l2_call_flash_op(brightness_set, led_cdev, 0);
+ external_strobe = (ctrl-source-val ==
+ V4L2_FLASH_STROBE_SOURCE_EXTERNAL);
+ return v4l2_call_flash_op(external_strobe_set, led_cdev,
+ external_strobe);
+ case V4L2_FLASH_LED_MODE_TORCH:
+ /* Stop flash strobing */
+ ret = v4l2_call_flash_op(strobe_set, led_cdev, false);
+ if (ret)
+ return ret;
+ /* torch is always triggered by software */
+ ret = v4l2_call_flash_op(external_strobe_set, led_cdev,
+ false);
Does the LED API not assume this at the moment? I'm not saying it should,
though. :-)
Actually strobe
Re: [PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
Thanks for the update!
On Fri, Apr 11, 2014 at 04:56:56PM +0200, Jacek Anaszewski wrote:
This patch adds helper functions for registering/unregistering
LED class flash devices as V4L2 subdevs. 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.anaszew...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/media/v4l2-core/Kconfig | 10 +
drivers/media/v4l2-core/Makefile |2 +
drivers/media/v4l2-core/v4l2-flash.c | 393
++
include/media/v4l2-flash.h | 119 ++
4 files changed, 524 insertions(+)
create mode 100644 drivers/media/v4l2-core/v4l2-flash.c
create mode 100644 include/media/v4l2-flash.h
diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index 2189bfb..1f8514d 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -35,6 +35,16 @@ config V4L2_MEM2MEM_DEV
tristate
depends on VIDEOBUF2_CORE
+# Used by LED subsystem flash drivers
+config V4L2_FLASH
+ bool Enable support for Flash sub-devices
+ depends on LEDS_CLASS_FLASH
Also:
depends on VIDEO_V4L2 VIDEO_V4L2_SUBDEV_API MEDIA_CAMERA_SUPPORT
The last one might be a matter of opinion.
+ ---help---
+ Say Y here to enable support for Flash sub-devices, which allow
+ to control LED class devices with use of V4L2 Flash controls.
+
+ When in doubt, say N.
+
# Used by drivers that need Videobuf modules
config VIDEOBUF_GEN
tristate
diff --git a/drivers/media/v4l2-core/Makefile
b/drivers/media/v4l2-core/Makefile
index c6ae7ba..8e37ab4 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -22,6 +22,8 @@ obj-$(CONFIG_VIDEO_TUNER) += tuner.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
+obj-$(CONFIG_V4L2_FLASH) += v4l2-flash.o
+
obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o
obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o
obj-$(CONFIG_VIDEOBUF_DMA_CONTIG) += videobuf-dma-contig.o
diff --git a/drivers/media/v4l2-core/v4l2-flash.c
b/drivers/media/v4l2-core/v4l2-flash.c
new file mode 100644
index 000..f1be332
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-flash.c
@@ -0,0 +1,393 @@
+/*
+ * V4L2 Flash LED sub-device registration helpers.
+ *
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd
+ * Author: Jacek Anaszewski j.anaszew...@samsung.com
+ *
+ * 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.
+ */
+
+#include linux/leds_flash.h
+#include media/v4l2-ctrls.h
+#include media/v4l2-dev.h
+#include media/v4l2-device.h
+#include media/v4l2-event.h
+#include media/v4l2-ioctl.h
+#include media/v4l2-flash.h
+
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
Fits on a single line.
+{
+ return intensity / config-step;
Shouldn't you first decrement the minimum before the division?
+}
+
+static inline u32 v4l2_flash_led_brightness_to_intensity(
+ struct led_ctrl *config,
+ enum led_brightness brightness)
+{
+ return brightness * config-step;
And do the opposite here?
+}
+
+static int v4l2_flash_g_volatile_ctrl(struct v4l2_ctrl *c)
+
+{
+ struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
+ struct led_classdev *led_cdev = v4l2_flash-led_cdev;
+ struct led_flash *flash = led_cdev-flash;
+ struct v4l2_flash_ctrl *ctrl = v4l2_flash-ctrl;
+ u32 fault;
+ int ret;
+
+ switch (c-id) {
+ case V4L2_CID_FLASH_TORCH_INTENSITY:
+ if (ctrl-led_mode-val == V4L2_FLASH_LED_MODE_TORCH) {
+ ret = v4l2_call_flash_op(brightness_update, led_cdev);
+ if (ret 0)
+ return ret;
+ ctrl-torch_intensity-val =
+ v4l2_flash_led_brightness_to_intensity(
+ led_cdev-brightness_ctrl,
+ led_cdev-brightness);
+ }
+ return 0;
+ case V4L2_CID_FLASH_INTENSITY:
+ ret = v4l2_call_flash_op(flash_brightness_update, led_cdev);
+ if (ret 0)
+ return ret;
+ /* no conversion is needed */
+ c-val = flash-brightness.val;
+ return 0;
+ case V4L2_CID_FLASH_INDICATOR_INTENSITY:
+ ret =
[PATCH/RFC v3 5/5] media: Add registration helpers for V4L2 flash sub-devices
This patch adds helper functions for registering/unregistering
LED class flash devices as V4L2 subdevs. 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.anaszew...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
---
drivers/media/v4l2-core/Kconfig | 10 +
drivers/media/v4l2-core/Makefile |2 +
drivers/media/v4l2-core/v4l2-flash.c | 393 ++
include/media/v4l2-flash.h | 119 ++
4 files changed, 524 insertions(+)
create mode 100644 drivers/media/v4l2-core/v4l2-flash.c
create mode 100644 include/media/v4l2-flash.h
diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index 2189bfb..1f8514d 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -35,6 +35,16 @@ config V4L2_MEM2MEM_DEV
tristate
depends on VIDEOBUF2_CORE
+# Used by LED subsystem flash drivers
+config V4L2_FLASH
+ bool Enable support for Flash sub-devices
+ depends on LEDS_CLASS_FLASH
+ ---help---
+ Say Y here to enable support for Flash sub-devices, which allow
+ to control LED class devices with use of V4L2 Flash controls.
+
+ When in doubt, say N.
+
# Used by drivers that need Videobuf modules
config VIDEOBUF_GEN
tristate
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index c6ae7ba..8e37ab4 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -22,6 +22,8 @@ obj-$(CONFIG_VIDEO_TUNER) += tuner.o
obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
+obj-$(CONFIG_V4L2_FLASH) += v4l2-flash.o
+
obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o
obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o
obj-$(CONFIG_VIDEOBUF_DMA_CONTIG) += videobuf-dma-contig.o
diff --git a/drivers/media/v4l2-core/v4l2-flash.c
b/drivers/media/v4l2-core/v4l2-flash.c
new file mode 100644
index 000..f1be332
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-flash.c
@@ -0,0 +1,393 @@
+/*
+ * V4L2 Flash LED sub-device registration helpers.
+ *
+ * Copyright (C) 2014 Samsung Electronics Co., Ltd
+ * Author: Jacek Anaszewski j.anaszew...@samsung.com
+ *
+ * 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.
+ */
+
+#include linux/leds_flash.h
+#include media/v4l2-ctrls.h
+#include media/v4l2-dev.h
+#include media/v4l2-device.h
+#include media/v4l2-event.h
+#include media/v4l2-ioctl.h
+#include media/v4l2-flash.h
+
+static inline enum led_brightness v4l2_flash_intensity_to_led_brightness(
+ struct led_ctrl *config,
+ u32 intensity)
+{
+ return intensity / config-step;
+}
+
+static inline u32 v4l2_flash_led_brightness_to_intensity(
+ struct led_ctrl *config,
+ enum led_brightness brightness)
+{
+ return brightness * config-step;
+}
+
+static int v4l2_flash_g_volatile_ctrl(struct v4l2_ctrl *c)
+
+{
+ struct v4l2_flash *v4l2_flash = v4l2_ctrl_to_v4l2_flash(c);
+ struct led_classdev *led_cdev = v4l2_flash-led_cdev;
+ struct led_flash *flash = led_cdev-flash;
+ struct v4l2_flash_ctrl *ctrl = v4l2_flash-ctrl;
+ u32 fault;
+ int ret;
+
+ switch (c-id) {
+ case V4L2_CID_FLASH_TORCH_INTENSITY:
+ if (ctrl-led_mode-val == V4L2_FLASH_LED_MODE_TORCH) {
+ ret = v4l2_call_flash_op(brightness_update, led_cdev);
+ if (ret 0)
+ return ret;
+ ctrl-torch_intensity-val =
+ v4l2_flash_led_brightness_to_intensity(
+ led_cdev-brightness_ctrl,
+ led_cdev-brightness);
+ }
+ return 0;
+ case V4L2_CID_FLASH_INTENSITY:
+ ret = v4l2_call_flash_op(flash_brightness_update, led_cdev);
+ if (ret 0)
+ return ret;
+ /* no conversion is needed */
+ c-val = flash-brightness.val;
+ return 0;
+ case V4L2_CID_FLASH_INDICATOR_INTENSITY:
+ ret = v4l2_call_flash_op(indicator_brightness_update, led_cdev);
+ if (ret 0)
+ return ret;
+ /* no conversion is needed */
+ c-val = flash-indicator_brightness-val;
+ return 0;
+ case V4L2_CID_FLASH_STROBE_STATUS:
+ ret = v4l2_call_flash_op(strobe_get, led_cdev);
+ if (ret 0)
+
