pkarashchenko commented on code in PR #9045:
URL: https://github.com/apache/nuttx/pull/9045#discussion_r1172422327


##########
drivers/video/isx012.c:
##########
@@ -1981,6 +1995,70 @@ static int isx012_get_supported_value(FAR struct 
imgsensor_s *sensor,
   return ret;
 }
 
+static void get_current_framesize(FAR struct isx012_dev_s *priv,
+                                  FAR uint16_t *w,
+                                  FAR uint16_t *h)
+{
+  uint16_t w_addr = HSIZE_MONI;
+  uint16_t h_addr = VSIZE_MONI;
+
+  switch (priv->mode)
+    {
+      case REGVAL_MODESEL_MON:
+        w_addr = HSIZE_MONI;
+        h_addr = VSIZE_MONI;
+        break;
+
+      case REGVAL_MODESEL_MOV:
+        w_addr = HSIZE_MOVIE;
+        h_addr = VSIZE_MOVIE;
+        break;
+
+      case REGVAL_MODESEL_CAP:
+        w_addr = HSIZE_CAP;
+        h_addr = VSIZE_CAP;
+        break;
+
+      default:
+
+        /* It does not come here due to register specification. */
+
+        break;
+    }
+
+  *w = isx012_getreg(priv, w_addr, 2);
+  *h = isx012_getreg(priv, h_addr, 2);
+}
+
+static uint16_t restore_spot_position(uint8_t regval,
+                                      uint16_t w,
+                                      uint16_t split)
+{
+  return ((regval * w) / split + (w / split) / 2);
+}
+
+static int32_t get_spot_position(FAR struct isx012_dev_s *priv)
+{
+  uint16_t regval;
+  uint16_t reg_x;
+  uint16_t reg_y;
+  uint16_t x;
+  uint16_t y;
+  uint16_t w;
+  uint16_t h;
+
+  regval = isx012_getreg(priv, SPOT_FRM_NUM, 1);
+  reg_x = regval % ISX012_SPOT_POSITION_SPLIT_NUM_X;
+  reg_y = regval / ISX012_SPOT_POSITION_SPLIT_NUM_X;
+
+  get_current_framesize(priv, &w, &h);
+
+  x = restore_spot_position(reg_x, w, ISX012_SPOT_POSITION_SPLIT_NUM_X);
+  y = restore_spot_position(reg_y, h, ISX012_SPOT_POSITION_SPLIT_NUM_Y);
+
+  return ((x << 16) | y);

Review Comment:
   maybe change `x` to `uint32_t`. I'm sure that promotion to int will work 
with `<< 16`, but better to state it explicitly



##########
drivers/video/isx019.c:
##########
@@ -2364,6 +2382,158 @@ static int set_meter(FAR isx019_dev_t *priv,
   return OK;
 }
 
+static void get_current_framesize(FAR isx019_dev_t *priv,
+                                  FAR uint16_t *w, FAR uint16_t *h)
+{
+  uint8_t frmsz;
+
+  DEBUGASSERT(w && h);
+
+  fpga_i2c_read(priv, FPGA_FORMAT_AND_SCALE, &frmsz, 1);
+
+  switch (frmsz & 0xf0)
+    {
+      case FPGA_SCALE_1280_960:
+        *w = 1280;
+        *h = 960;
+        break;
+
+      case FPGA_SCALE_640_480:
+        *w = 640;
+        *h = 480;
+        break;
+
+      case FPGA_SCALE_320_240:
+        *w = 320;
+        *h = 240;
+        break;
+
+      case FPGA_SCALE_160_120:
+        *w = 160;
+        *h = 120;
+        break;
+
+      default:
+
+        /* It may not come here due to register specification */
+
+        break;
+    }
+}
+
+static void get_current_clip_setting(FAR isx019_dev_t *priv,
+                                     FAR uint16_t *w,
+                                     FAR uint16_t *h,
+                                     FAR uint16_t *offset_x,
+                                     FAR uint16_t *offset_y)
+{
+  uint8_t sz;
+  uint8_t top;
+  uint8_t left;
+
+  fpga_i2c_read(priv, FPGA_CLIP_SIZE, &sz, 1);
+  fpga_i2c_read(priv, FPGA_CLIP_TOP,  &top, 1);
+  fpga_i2c_read(priv, FPGA_CLIP_LEFT, &left, 1);
+
+  *offset_x = left * FPGA_CLIP_UNIT;
+  *offset_y = top  * FPGA_CLIP_UNIT;
+
+  switch (sz)
+    {
+      case FPGA_CLIP_NON:
+        *w = 0;
+        *h = 0;
+        *offset_x = 0;
+        *offset_y = 0;
+        break;
+
+      case FPGA_CLIP_1280_720:
+        *w = 1280;
+        *h = 720;
+        break;
+
+      case FPGA_CLIP_640_360:
+        *w = 640;
+        *h = 360;
+        break;
+
+      default:
+
+        /* It may not come here due to register specification */
+
+        break;
+    }
+}
+
+static int calc_spot_position_regval(uint16_t val,
+                                     uint16_t basis,
+                                     uint16_t sz,
+                                     uint16_t offset,
+                                     int      split)
+{
+  int ret;
+  int ratio;
+
+  /* Change basis from `sz` to `basis` about `val` and `offset`. */
+
+  ratio = basis / sz;
+  ret = val * ratio;
+  ret += (offset * FPGA_CLIP_UNIT * ratio);
+
+  return (ret * split) / basis;
+}
+
+static int set_spot_position(FAR isx019_dev_t *priv,
+                             imgsensor_value_t val)
+{
+  uint8_t regval;
+  uint8_t reg_x;
+  uint8_t reg_y;
+  uint16_t w;
+  uint16_t h;
+  uint16_t clip_w;
+  uint16_t clip_h;
+  uint16_t offset_x;
+  uint16_t offset_y;
+  uint16_t x = (uint16_t)((val.value32 & 0xffff0000) >> 16);

Review Comment:
   I think
   ```suggestion
     uint16_t x = (uint16_t)(val.value32 >> 16);
   ```
   should give the same result



##########
drivers/video/isx019.c:
##########
@@ -2972,6 +3146,83 @@ static int get_meter(FAR isx019_dev_t *priv,
   return OK;
 }
 
+static uint16_t restore_spot_position(uint16_t regval,
+                                      uint16_t basis,
+                                      uint16_t sz,
+                                      uint16_t clip_sz,
+                                      uint16_t offset,
+                                      uint16_t split)
+{
+  uint16_t ret;
+  uint16_t unit;
+  uint16_t border;
+
+  /* First, convert register value to coordinate value. */
+
+  unit = basis / split;
+
+  ret = (regval * unit) + (unit / 2);
+
+  /* Second, consider the ratio between basis size and frame size. */
+
+  ret = ret * sz / basis;
+
+  /* Third, consider offset value of clip setting. */
+
+  if  (ret > offset)
+    {
+      ret = ret - offset;
+    }
+  else
+    {
+      ret = 0;
+    }
+
+  /* If the coordinate protrudes from the frame,
+   * regard it as the boader of the frame.
+   */
+
+  border = (clip_sz != 0) ? (clip_sz - 1) : (sz - 1);
+  if (ret > border)
+    {
+      ret = border;
+    }
+
+  return ret;
+}
+
+static int get_spot_position(FAR isx019_dev_t *priv,
+                             FAR imgsensor_value_t *val)
+{
+  uint8_t regval;
+  uint8_t regx;
+  uint8_t regy;
+  uint16_t w;
+  uint16_t h;
+  uint16_t x;
+  uint16_t y;
+  uint16_t clip_w;
+  uint16_t clip_h;
+  uint16_t offset_x;
+  uint16_t offset_y;
+  int split;
+
+  isx019_i2c_read(priv, CAT_CATAE, SPOT_FRM_NUM, &regval, 1);
+
+  regx = regval % ISX019_SPOT_POSITION_SPLIT_NUM_X;
+  regy = regval / ISX019_SPOT_POSITION_SPLIT_NUM_X;
+
+  get_current_framesize(priv, &w, &h);
+  get_current_clip_setting(priv, &clip_w, &clip_h, &offset_x, &offset_y);
+  split = ISX019_SPOT_POSITION_SPLIT_NUM_X;
+  x = restore_spot_position(regx, ISX019_WIDTH,  w, clip_w, offset_x, split);
+  split = ISX019_SPOT_POSITION_SPLIT_NUM_Y;
+  y = restore_spot_position(regy, ISX019_HEIGHT, h, clip_h, offset_y, split);
+
+  val->value32 = (x << 16) | y;

Review Comment:
   ditto



##########
drivers/video/isx012.c:
##########
@@ -2356,6 +2438,31 @@ static int set_clip(uint32_t size,
   return OK;
 }
 
+static int set_spot_position(FAR struct isx012_dev_s *priv, int32_t val)
+{
+  uint16_t w;
+  uint16_t h;
+  uint16_t x = (uint16_t)((val & 0xffff0000) >> 16);

Review Comment:
   Optional
   ```suggestion
     uint16_t x = (uint16_t)(val >> 16);
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to