Hello Andy,
Thanks for the review. Sorry for the late reply.
On Mon, 2019-12-11 at 16:36 +0200, Andy Shevchenko wrote:
> On Wed, Dec 11, 2019 at 07:28:49PM +0800, Dongchun Zhu wrote:
> > Add a V4L2 sub-device driver for OV02A10 image sensor. The OV02A10 is
> > a 1/5" CMOS sensor from Omnivision, asupporting output format: 10-bit Raw.
> >
> > This chip has a single MIPI lane interface and use the I2C bus for
> > control and the CSI-2 bus for data.
>
> ...
>
> > +#define OV02A10_MASK_8_BITS 0xff
>
> Besides GENMASK() why do you need a definition here? What's the point?
>
Fixed in next release.
> ...
>
> > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg) {
> > +struct v4l2_subdev_format fmt = {
> > +.which = cfg ? V4L2_SUBDEV_FORMAT_TRY
> > + : V4L2_SUBDEV_FORMAT_ACTIVE,
> > +.format = {
> > +.width = 1600,
>
> > +.height = 1200
>
> Leave comma here.
>
Fixed in next release.
> > +}
> > +};
> > +
> > +ov02a10_set_fmt(sd, cfg, &fmt);
> > +
> > +return 0;
> > +}
>
> ...
>
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_GAIN,
> > +(val & OV02A10_MASK_8_BITS));
>
> Too many parentheses.
>
Fixed in next release.
> > +if (ret < 0)
> > +return ret;
>
> ...
>
> > +static int ov02a10_set_vblank(struct ov02a10 *ov02a10, int val) {
> > +struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
>
> if you do
>
> int vts = val + ov02a10->cur_mode->height - OV02A10_BASIC_LINE;
>
> you may increase readability below...
>
Thanks for the suggestion.
It seems better now.
> > +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,
> > +(((val + ov02a10->cur_mode->height -
> > +OV02A10_BASIC_LINE) >>
> > +OV02A10_VTS_SHIFT) &
> > +OV02A10_MASK_8_BITS));
>
> ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> (vts >> OV02A10_VTS_SHIFT) &
> OV02A10_MASK_8_BITS));
>
> And actually why do you need this mask here? Isn't enough to call
>
> ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_H,
> vts >> OV02A10_VTS_SHIFT);
>
> here...
>
>
Yes. Now we code like this.
> > +if (ret < 0)
> > +return ret;
> > +
> > +ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L,
> > +((val + ov02a10->cur_mode->height -
> > +OV02A10_BASIC_LINE) &
> > +OV02A10_MASK_8_BITS));
>
> ...and
>
> ret = i2c_smbus_write_byte_data(client, OV02A10_REG_VTS_L, vts);
>
> here?
>
Yes. Fixed in next release.
> > +if (ret < 0)
> > +return ret;
> > +
> > +return i2c_smbus_write_byte_data(client, REG_GLOBAL_EFFECTIVE,
> > + REG_ENABLE);
> > +}
>
> ...
>
> > +static int ov02a10_check_hwcfg(struct device *dev, struct ov02a10
> > +*ov02a10) {
> > +struct fwnode_handle *ep;
> > +struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +struct v4l2_fwnode_endpoint bus_cfg = {
>
> > +.bus_type = V4L2_MBUS_CSI2_DPHY
>
> Leave comma here.
>
Fixed in next release.
> > +};
> > +unsigned int i, j;
> > +int ret;
>
> > +if (!fwnode)
> > +return -ENXIO;
>
> A bit strange error code here.
>
This should be reported as -EINVAL.
Fixed in next release.
> > +
> > +ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > +if (!ep)
> > +return -ENXIO;
> > +
> > +ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > +fwnode_handle_put(ep);
> > +if (ret)
> > +return ret;
>
> > +if (!bus_cfg.nr_of_link_frequencies) {
> > +dev_err(dev, "no link frequencies defined");
> > +ret = -EINVAL;
> > +goto check_hwcfg_error;
> > +}
>
> I still think it's redundant check, though it's up to maintainers.
>
We still wanna keep this check.
Keep same as ov2659 and ov8856.
> > +
> > +for (i = 0; i < ARRAY_SIZE(link_freq_menu_items); i++) {
> > +for (j = 0; j < bus_cfg.nr_of_link_frequencies; j++) {
> > +if (link_freq_menu_items[i] ==
> > +bus_cfg.link_frequencies[j])
> > +break;
> > +}
> > +
> > +if (j == bus_cfg.nr_of_link_frequencies) {
> > +dev_err(dev, "no link frequency %lld supported",
> > +link_freq_menu_items[i]);
> > +ret = -EINVAL;
> > +goto check_hwcfg_error;
> > +}
> > +}
> > +
> > +check_hwcfg_error:
> > +v4l2_fwnode_endpoint_free(&bus_cfg);
> > +
> > +return ret;
> > +}
>
> ...
>
> > +static int ov02a10_probe(struct i2c_client *client) {
>
> > +/* Optional indication of physical rotation of sensor */
> > +ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation",
> > +&rotation);
>
> > +if (!ret) {
>
> Why not positive conditional?
>
Okay. Fixed in next release.
> > +ov02a10->upside_down = rotation == 180;
> > +if (rotation == 180) {
> > +ov02a10->upside_down = true;
> > +ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > +}
> > +} else {
> > +dev_warn(dev, "failed to get rotation\n");
> > +}
> > +
> > +/* Optional indication of mipi TX speed */
> > +ret = fwnode_property_read_u32(dev_fwnode(dev), "ovti,mipi-tx-speed",
> > + &clock_lane_tx_speed);
> > +
>
> > +if (!ret)
>
> Ditto.
>
As Sakari mentioned earlier, the property "ovti,mipi-tx-speed" is
optional that shouldn't warn it's missing when ret is 0.
So we would keep the condition like that, just removing else case.
> > +ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed;
> > +else
> > +dev_warn(dev, "failed to get mipi tx speed, using default...\n");
> > +
>
> > +return ret;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
> *********************MEDIATEK Confidential/Internal Use*********************
