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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Dongchun,

On Wed, Apr 08, 2020 at 07:53:44PM +0800, Dongchun Zhu wrote:
> 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.

I don't remember discussing this, but could be because it was quite some
time ago.

It doesn't seem to be documented. What is it for?

> > *********************MEDIATEK Confidential/Internal Use*********************

Is this intentional?

-- 
Regards,

Sakari Ailus

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek