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

From: Dongchun Zhu <dongchun.zhu@mediatek.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: <andriy.shevchenko@linux.intel.com>, <mchehab@kernel.org>,
	<robh+dt@kernel.org>, <mark.rutland@arm.com>,
	<matthias.bgg@gmail.com>, <bingbu.cao@intel.com>,
	<srv_heupstream@mediatek.com>,
	<linux-mediatek@lists.infradead.org>,
	<linux-arm-kernel@lists.infradead.org>, <sj.huang@mediatek.com>,
	<linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<louis.kuo@mediatek.com>, <shengnan.wang@mediatek.com>
Subject: Re: [V6, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver
Date: Thu, 9 Apr 2020 14:04:40 +0800	[thread overview]
Message-ID: <1586412280.8804.38.camel@mhfsdcap03> (raw)
In-Reply-To: <20200408122037.GG5206@paasikivi.fi.intel.com>

Hi Sakari,

On Wed, 2020-04-08 at 15:20 +0300, Sakari Ailus wrote:
> 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?
> 

Sorry for not addressing this point on v6.
"ovti,mipi-tx-speed" is one private property in DT, which is abstracted
to handle different register settings for different Chrome projects.

More details please see the Google Issue:
https://partnerissuetracker.corp.google.com/issues/143749215

And it would be documented in next release.

> > > *********************MEDIATEK Confidential/Internal Use*********************
> 
> Is this intentional?
> 