Hi Sakari,
On Fri, 24 Apr 2015 00:52:12 +0300
Sakari Ailus <sakari.ailus@xxxxxx> wrote:
> Hi Jacek,
>
> Jacek Anaszewski wrote:
> ...
> >>> +#define call_flash_op(v4l2_flash, op,
> >>> arg) \
> >>> + (has_flash_op(v4l2_flash,
> >>> op) ? \
> >>> + v4l2_flash->ops->op(v4l2_flash,
> >>> arg) : \
> >>> + -EINVAL)
> >>> +
> >>> +static enum led_brightness __intensity_to_led_brightness(
> >>> + struct v4l2_ctrl *ctrl,
> >>> + s32 intensity)
> >>
> >> Fits on previous line.
> >>
> >>> +{
> >>> + s64 intensity64 = intensity - ctrl->minimum;
> >>
> >> intensity, ctrl->step and ctrl->minimum are 32-bit signed integers.
> >> Do you need a 64-bit integer here?
> >
> > step is u64.
>
> Nevertheless integer controls will not have values outside the s32
> range, using a step value that's outside the range makes no sense
> either. I think you should use s32 instead.
I infer that local u32 variable should be assigned ctrl->step,
and then used as a divisor.
> >
> >>
> >>> +
> >>> + do_div(intensity64, ctrl->step);
> >>> +
> >>> + /*
> >>> + * Indicator LEDs, unlike torch LEDs, are turned on/off
> >>> basing on
> >>> + * the state of V4L2_CID_FLASH_INDICATOR_INTENSITY
> >>> control only.
> >>> + * Therefore it must be possible to set it to 0 level
> >>> which in
> >>> + * the LED subsystem reflects LED_OFF state.
> >>> + */
> >>> + if (ctrl->id != V4L2_CID_FLASH_INDICATOR_INTENSITY)
> >>> + ++intensity64;
> >>
> >> I think the condition could simply be ctrl->minimum instead, that
> >> way I find it easier to understand what's happening here. I'd
> >> expect the minimum for non-intensity controls always to be
> >> non-zero, though, so the end result is the same. Up to you.
> >
> > Minimum for indicator control must be 0 to make possible
> > turning the indicator LED off only with this control.
>
> Would torch be still on if the minimum torch current was 0 mA? I'd
> say no.
>
> Although in that case I'd expect the driver to use a different range,
> and selecting the off mode would then turn it off, I still think
> that's a better condition than relying on the control id.
I didn't catch your point previously. Probably you was thinking
about somethig like this:
if (ctrl->minimum)
++intensity;
If so, I agree.
> ...
>
> >>> +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_flash *fled_cdev =
> >>> v4l2_flash->fled_cdev;
> >>> + bool is_strobing;
> >>> + int ret;
> >>> +
> >>> + switch (c->id) {
> >>> + case V4L2_CID_FLASH_TORCH_INTENSITY:
> >>> + case V4L2_CID_FLASH_INDICATOR_INTENSITY:
> >>> + return
> >>> v4l2_flash_update_led_brightness(v4l2_flash, c);
> >>> + case V4L2_CID_FLASH_INTENSITY:
> >>> + ret = led_update_flash_brightness(fled_cdev);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> + /* no conversion is needed */
> >>
> >> Maybe a stupid question, but why is it not needed?
> >
> > Because LED Flash class also uses microamperes.
>
> Right, I had missed that. It'd be nice if that was said in the
> comment, it might not be obvious to others either.
OK, I will add the comment.
--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
