Messages in this thread | | | Date | Wed, 07 May 2014 09:20:17 +0200 | From | Jacek Anaszewski <> | Subject | 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
| |