Re: [V6, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Dongchun Zhu <dongchun.zhu@mediatek.com>]

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*********************