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,
> >>+ V4L2_CID_FLASH_STROBE_SOURCE,
> >>+ max,
> >>+ ~mask,
> >>+ V4L2_FLASH_STROBE_SOURCE_SOFTWARE);
> >>+
> >>+ /* Configure FLASH_STROBE ctrl */
> >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops,
> >>+ V4L2_CID_FLASH_STROBE, 0, 1, 1, 0);
> >>+
> >>+ /* Configure FLASH_STROBE_STOP ctrl */
> >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops,
> >>+ V4L2_CID_FLASH_STROBE_STOP,
> >>+ 0, 1, 1, 0);
> >>+
> >>+ /* Configure FLASH_STROBE_STATUS ctrl */
> >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops,
> >>+ V4L2_CID_FLASH_STROBE_STATUS,
> >>+ 0, 1, 1, 1);
> >>+ if (ctrl)
> >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE |
> >>+ V4L2_CTRL_FLAG_READ_ONLY;
> >>+
> >>+ /* Configure FLASH_TIMEOUT ctrl */
> >>+ ctrl_cfg = &flash->timeout;
> >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops,
> >>+ V4L2_CID_FLASH_TIMEOUT, ctrl_cfg->min,
> >>+ ctrl_cfg->max, ctrl_cfg->step,
> >>+ ctrl_cfg->val);
> >>+ if (ctrl)
> >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
> >>+
> >>+ /* Configure FLASH_INTENSITY ctrl */
> >>+ ctrl_cfg = &flash->brightness;
> >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl, &v4l2_flash_ctrl_ops,
> >>+ V4L2_CID_FLASH_INTENSITY,
> >>+ ctrl_cfg->min, ctrl_cfg->max,
> >>+ ctrl_cfg->step, ctrl_cfg->val);
> >>+ if (ctrl)
> >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
> >>+
> >>+ if (flash->fault_flags) {
> >>+ /* Configure FLASH_FAULT ctrl */
> >>+ ctrl = v4l2_ctrl_new_std(&v4l2_flash->hdl,
> >>+ &v4l2_flash_ctrl_ops,
> >>+ V4L2_CID_FLASH_FAULT, 0,
> >>+ flash->fault_flags,
> >>+ 0, 0);
> >>+ if (ctrl)
> >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE |
> >>+ V4L2_CTRL_FLAG_READ_ONLY;
> >>+ }
> >>+ if (has_indicator) {
> >
> >In theory it's possible to have an indicator without the flash. So I'd keep
> >the two independent.
>
> OK.
>
> >>+ /* Configure FLASH_INDICATOR_INTENSITY ctrl */
> >>+ ctrl_cfg = flash->indicator_brightness;
> >>+ ctrl = v4l2_ctrl_new_std(
> >>+ &v4l2_flash->hdl, &v4l2_flash_ctrl_ops,
> >>+ V4L2_CID_FLASH_INDICATOR_INTENSITY,
> >>+ ctrl_cfg->min, ctrl_cfg->max,
> >>+ ctrl_cfg->step, ctrl_cfg->val);
> >>+ if (ctrl)
> >>+ ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
> >>+ }
> >>+ }
> >>+
> >>+ if (v4l2_flash->hdl.error) {
> >>+ ret = v4l2_flash->hdl.error;
> >>+ goto error_free;
> >>+ }
> >>+
> >>+ ret = v4l2_ctrl_handler_setup(&v4l2_flash->hdl);
> >>+ if (ret < 0)
> >>+ goto error_free;
> >>+
> >>+ v4l2_flash->subdev.ctrl_handler = &v4l2_flash->hdl;
> >>+
> >>+ return 0;
> >>+
> >>+error_free:
> >>+ v4l2_ctrl_handler_free(&v4l2_flash->hdl);
> >>+ return ret;
> >>+}
> >>+
> >>+/*
> >>+ * 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.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
