On Sat, May 23, 2020 at 12:50:15PM +0800, Dongchun Zhu wrote:
> Hi Tomasz,
>
> Thanks for the review. My replies are as below.
>
> On Thu, 2020-05-21 at 19:32 +0000, Tomasz Figa wrote:
> > Hi Dongchun,
> >
> > On Sat, May 09, 2020 at 04:06:27PM +0800, Dongchun Zhu wrote:
[snip]
> > > +{
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > > + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > > + int ret;
> > > +
> > > + gpiod_set_value_cansleep(ov02a10->n_rst_gpio, 0);
> > > + gpiod_set_value_cansleep(ov02a10->pd_gpio, 0);
> > > +
> > > + ret = clk_prepare_enable(ov02a10->eclk);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to enable eclk\n");
> > > + return ret;
> > > + }
> > > +
> > > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> > > + if (ret < 0) {
> > > + dev_err(dev, "failed to enable regulators\n");
> > > + goto disable_clk;
> > > + }
> > > + usleep_range(5000, 6000);
> > > +
> > > + gpiod_set_value_cansleep(ov02a10->pd_gpio, 1);
> >
> > This is a "powerdown" GPIO. It must be set to 0 if the sensor is to be
> > powered on.
> >
>
> The value set by gpiod_set_value_cansleep() API actually depends upon
> GPIO polarity defined in DT.
> Since I set GPIO_ACTIVE_LOW to powerdown,
> gpiod_set_value_cansleep(gpio_desc, value) would set !value to
> gpio_desc.
> Thus here powerdown would be low-state when sensor is powered on.
> For GPIO polarity, I also post a comment to the binding patch.
>
That's true. However, this makes the driver really confusing. If someone
reads this code and compares with the datasheet, it looks incorrect,
because in the datasheet the powerdown GPIO needs to be configured low
for the sensor to operate.
I'd recommend defining the binding in a way that makes it clear in the driver code
that it implementes the power sequencing as per the datasheet.
[snip]
> > > +/*
> > > + * ov02a10_set_exposure - Function called when setting exposure time
> > > + * @priv: Pointer to device structure
> > > + * @val: Variable for exposure time, in the unit of micro-second
> > > + *
> > > + * Set exposure time based on input value.
> > > + *
> > > + * Return: 0 on success
> > > + */
> > > +static int ov02a10_set_exposure(struct ov02a10 *ov02a10, int val)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > > + int ret;
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> > > + if (ret < 0)
> > > + return ret;
> >
> > How does this page switch work? According to the documentation I have, the
> > register allows selecting between a few different pages. However, there
> > should be two page pointers - one for the AP and the other for the sensor,
> > so that when the AP is programming page X, the sensor can have consistent
> > settings from page Y. But here we only set one register and always with
> > page 1.
> >
>
> Thanks for the carefully observation.
> The style or requirement of register setting here is suggested by OV
> vendor.
> From hardware signal behavior and effect-test, this setting should be
> right.
> But for your concern, we can also dig into it with OV.
> Let's have time to talk with OV.
>
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_H,
> > > + val >> OV02A10_EXP_SHIFT);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_EXPOSURE_L, val);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > > + REG_ENABLE);
> >
> > This patch defines REG_GLOBAL_EFFECTIVE to 0x01. I don't see such register
> > mentioned in the documentation.
> >
>
> There may be several editions of sensor documentation.
> From OV, 0x01 is one register shall be updated to keep
> exposure/gain/test pattern... register settings effective.
>
Okay, let's try to get some explanation of this offline.
> > > +}
> > > +
> > > +static int ov02a10_set_gain(struct ov02a10 *ov02a10, int val)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > > + int ret;
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN, val);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > > + REG_ENABLE);
> > > +}
> > > +
> > > +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > > + u32 vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE;
> > > + int ret;
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, REG_PAGE_SWITCH, REG_ENABLE);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> > > + vts >> OV02A10_VTS_SHIFT);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > > + REG_ENABLE);
> > > +}
> > > +
> > > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, int pattern)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > > + int ret;
> > > +
> > > + if (pattern)
> > > + pattern = OV02A10_TEST_PATTERN_ENABLE;
> >
> > Is this necessary? Our control can be 0 for disabled and 1 for color bars.
> > The latter is the same as the above macro.
> >
>
> Yes. It looks redundant here.
> Fixed in next release.
>
> > [snip]
> > > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > > + const struct ov02a10_mode *mode;
> > > + struct v4l2_ctrl_handler *handler;
> > > + struct v4l2_ctrl *ctrl;
> > > + u64 exposure_max;
> > > + u32 pixel_rate, h_blank;
> > > + int ret;
> > > +
> > > + handler = &ov02a10->ctrl_handler;
> > > + mode = ov02a10->cur_mode;
> > > + ret = v4l2_ctrl_handler_init(handler, 7);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + handler->lock = &ov02a10->mutex;
> > > +
> > > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ, 0, 0,
> > > + link_freq_menu_items);
> > > + if (ctrl)
> > > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > > + pixel_rate = to_pixel_rate(0);
> > > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE, 0, pixel_rate, 1,
> > > + pixel_rate);
> > > +
> > > + h_blank = mode->hts_def - mode->width;
> > > + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK,
> > > + h_blank, h_blank, 1, h_blank);
> > > + if (ov02a10->hblank)
> > > + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> >
> > Do we need to hold a pointer to this control? We don't seem to ever access
> > it anywhere else in the driver.
> >
>
> No.
> These lines would be removed in next release.
>
> > > + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> > > + V4L2_CID_VBLANK, mode->vts_def -
> > > + mode->height,
> > > + OV02A10_VTS_MAX - mode->height, 1,
> > > + mode->vts_def - mode->height);
> > > +
> >
> > Ditto.
> >
>
> These lines would be removed in next release.
>
> > > + exposure_max = mode->vts_def - 4;
> > > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> > > + V4L2_CID_EXPOSURE,
> > > + OV02A10_EXPOSURE_MIN,
> > > + exposure_max,
> > > + OV02A10_EXPOSURE_STEP,
> > > + mode->exp_def);
> > > +
> > > + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> > > + V4L2_CID_ANALOGUE_GAIN,
> > > + OV02A10_GAIN_MIN,
> > > + OV02A10_GAIN_MAX,
> > > + OV02A10_GAIN_STEP,
> > > + OV02A10_GAIN_DEFAULT);
> >
> > Ditto.
> >
>
> Fields: exposure and anal_gain would be removed in next release.
> But v4l2_ctrl_new_std remains, as user may set exp/gain.
>
I don't mean removing the controls, but just not storing the returned
pointers inside driver data.
Best regards,
Tomasz
