Hi Jacek,
On Fri, Apr 24, 2015 at 12:29:17PM +0200, Jacek Anaszewski wrote:
> 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.
You could cast explicitly as well. Either is fine for me.
>
> > >
> > >>
> > >>> +
> > >>> + 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.
Yes, that's what I meant.
>
> > ...
> >
> > >>> +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.
Thanks!
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
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
