Hi Niklas, Thank you for the patch.
On Tuesday 06 Sep 2016 16:38:56 Niklas Söderlund wrote: > The HGT is a Histogram Generator Two-Dimensions. It computes a weighted > frequency histograms for hue and saturation areas over a configurable > region of the image with optional subsampling. > > Signed-off-by: Niklas Söderlund <[email protected]> > --- > drivers/media/platform/vsp1/Makefile | 2 +- > drivers/media/platform/vsp1/vsp1.h | 3 + > drivers/media/platform/vsp1/vsp1_drv.c | 33 ++++- > drivers/media/platform/vsp1/vsp1_entity.c | 33 +++-- > drivers/media/platform/vsp1/vsp1_entity.h | 1 + > drivers/media/platform/vsp1/vsp1_hgt.c | 217 +++++++++++++++++++++++++++ > drivers/media/platform/vsp1/vsp1_hgt.h | 42 ++++++ > drivers/media/platform/vsp1/vsp1_pipe.c | 16 +++ > drivers/media/platform/vsp1/vsp1_pipe.h | 2 + > drivers/media/platform/vsp1/vsp1_regs.h | 9 ++ > drivers/media/platform/vsp1/vsp1_video.c | 10 +- > 11 files changed, 352 insertions(+), 16 deletions(-) > create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c > create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h [snip] > diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c > b/drivers/media/platform/vsp1/vsp1_hgt.c new file mode 100644 > index 0000000..4e3f762 > --- /dev/null > +++ b/drivers/media/platform/vsp1/vsp1_hgt.c [snip] > +/* ------------------------------------------------------------------------ > + * Controls > + */ > + > +#define V4L2_CID_VSP1_HGT_HUE_AREAS (V4L2_CID_USER_BASE | 0x1001) > + > +static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt, > + ctrls); > + u8 *value = ctrl->p_new.p_u8; Nitpicking, I'd call the variable values. > + unsigned int i; > + bool ok = true; > + > + /* > + * Make sure values meet one of two possible hardware constrains s/constrains/constraints./ > + * 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U > + * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 0L > + */ > + > + if ((value[0] > value[1]) && (value[11] > value[0])) > + ok = false; > + for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) > + if (value[i] > value[i+1]) > + ok = false; > + > + /* Values do not match hardware, adjust to valid settings. */ > + if (!ok) { > + for (i = 0; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) { > + if (value[i] > value[i+1]) > + value[i] = value[i+1]; > + } > + } I'm afraid this won't work. Let's assume value[0] = 100, value[1] = 50, value[2] = 25. The loop will unroll to if (value[0] /* 100 */ > value[1] /* 50 */) value[0] = value[1] /* 50 */; if (value[1] /* 50 */ > value[2] /* 25 */) value[1] = value[2] /* 25 */; You will end up with value[0] = 50, value[1] = 25, value[2] = 25, which doesn't match the hardware constraints. How about the following, which tests and fixes the values in a single operation ? static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl) { struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt, ctrls); u8 *values = ctrl->p_new.p_u8; unsigned int i; /* * Adjust the values if they don't meet the hardware constraints: * * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U */ for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) { if (values[i] > values[i+1]) values[i+1] = values[i]; } /* 0L <= 0U or 5U <= 0L */ if (values[0] > values[1] && values[11] > values[0]) values[0] = values[1]; memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas)); return 0; } I'm also beginning to wonder whether it wouldn't make sense to return -EINVAL when the values don't match the constraints instead of trying to fix them. > + memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas)); > + > + return 0; > +} [snip] -- Regards, Laurent Pinchart
