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

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

Hi Andy,

On Mon, 2019-11-04 at 14:26 +0200, Andy Shevchenko wrote:
> On Mon, Nov 04, 2019 at 06:57:13PM +0800, Dongchun Zhu wrote:
> > Add a V4L2 sub-device driver for OV02A10 image sensor.
> > The OV02A10 is a 1/5" CMOS sensor from Omnivision, supporting
> > 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.
> 
> > +/* Bit[7] clock HS mode enable
> > + * 0: Clock continue
> > + * 1: Clock HS
> > + * Bit[6:2] HS VOD adjust
> > + * Bit[1:0] P VHI adjust
> > + */
> > +#define REG_HS_MODE_BLC					0x9d
> > +
> > +#define CLOCK_HS_MODE_ENABLE				BIT(7)
> 
> > +#define CLOCK_HS_VOD_ADJUST	(BIT(6) | BIT(5) | BIT(4) | BIT(3) | BIT(2))
> 
> GENMASK()

Fixed in next release.

> 
> > +#define OV02A10_MASK_8_BITS				0xff
> 
> GENMASK()

Fixed in next release.

> 
> > +#define OV02A10_LINK_FREQ_390MHZ			390000000ULL
> 
> Can't we publish HZ_PER_MHZ for wider use (currently in phy-stm32-usbphyc.c and
> hid-sensor-attributes.c) and use here something like (390 * HZ_PER_MHZ)?
> 
> > +#define OV02A10_ECLK_FREQ				24000000
> 
> Ditto.

Fixed in next release.

> 
> > +static const s64 link_freq_menu_items[] = {
> 
> > +	OV02A10_LINK_FREQ_390MHZ
> 
> Better to keep comma here.

Fixed in next release.

> 
> > +};
> 
> > +{
> > +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_byte_data(client, reg);
> > +	if (ret >= 0) {
> > +		*val = (unsigned char)ret;
> > +		ret = 0;
> > +	}
> 
> Why not to use classical pattern
> 
> 	if (ret < 0)
> 		return ret;
> 	...
> 	return 0;
> 
> ?
> 

Many thanks for reminder. Fixed in next release.

> > +
> > +	return ret;
> > +}
> 
> > +static int ov02a10_mod_reg(struct ov02a10 *ov02a10, u8 reg, u8 mask, u8 val)
> > +{
> > +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +	u8 readval;
> > +	int ret;
> > +
> > +	ret = ov02a10_read_smbus(ov02a10, reg, &readval);
> > +	if (ret)
> > +		return ret;
> > +
> 
> > +	readval &= ~mask;
> > +	val &= mask;
> > +	val |= readval;
> 
> Why not to use classical pattern
> 	val = (readval & ~mask) | (val & mask);
> ?
> 

Thanks for reminder. Fixed in next release.

> > +
> > +	return i2c_smbus_write_byte_data(client, reg, val);
> > +}
> 
> > +{
> > +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +	u16 id;
> 
> > +	u8 pid = 0;
> > +	u8 ver = 0;
> 
> Useless assignments.
> 

Fixed in next release.

> > +	int ret;
> > +
> > +	/* Check sensor revision */
> > +	ret = ov02a10_read_smbus(ov02a10, OV02A10_REG_CHIP_ID_H, &pid);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ov02a10_read_smbus(ov02a10, OV02A10_REG_CHIP_ID_L, &ver);
> > +	if (ret)
> > +		return ret;
> > +
> > +	id = OV02A10_ID(pid, ver);
> > +	if (id != CHIP_ID) {
> > +		dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_dbg(&client->dev, "Detected OV%04X sensor\n", id);
> > +
> > +	return 0;
> > +}
> 
> > +	int ret = 0;
> 
> Useless assignment...
> 

Fixed in next release.

> > +
> > +	dev_dbg(&client->dev, "ov02a10 s_stream (%d)\n", on);
> > +	mutex_lock(&ov02a10->mutex);
> > +
> > +	if (ov02a10->streaming == on)
> > +		goto unlock_and_return;
> > +
> > +	if (on) {
> > +		ret = pm_runtime_get_sync(&client->dev);
> > +		if (ret < 0) {
> > +			pm_runtime_put_noidle(&client->dev);
> > +			goto unlock_and_return;
> > +		}
> > +
> > +		ret = __ov02a10_start_stream(ov02a10);
> > +		if (ret) {
> > +			__ov02a10_stop_stream(ov02a10);
> > +			ov02a10->streaming = !on;
> > +			goto err_rpm_put;
> > +		}
> > +	} else {
> > +		__ov02a10_stop_stream(ov02a10);
> > +		pm_runtime_put(&client->dev);
> > +	}
> > +
> > +	ov02a10->streaming = on;
> > +	mutex_unlock(&ov02a10->mutex);
> > +
> 
> > +	return ret;
> 
> ...simple use
> 	return 0;
> here.
> 

Fixed in next release.

> > +
> > +err_rpm_put:
> > +	pm_runtime_put(&client->dev);
> > +unlock_and_return:
> > +	mutex_unlock(&ov02a10->mutex);
> > +
> > +	return ret;
> > +}
> 
> > +static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct ov02a10 *ov02a10 = container_of(ctrl->handler,
> > +					       struct ov02a10, ctrl_handler);
> > +	struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > +	s64 max_expo;
> 
> > +	int ret = 0;
> 
> Wouldn't be better to have this under `default` case below?
> 

Fixed in next release.

> > +
> > +	/* Propagate change of current control to all related controls */
> > +	if (ctrl->id == V4L2_CID_VBLANK) {
> > +		/* Update max exposure while meeting expected vblanking */
> > +		max_expo = ov02a10->cur_mode->height + ctrl->val -
> > +			   OV02A10_EXPOSURE_MAX_MARGIN;
> > +		__v4l2_ctrl_modify_range(ov02a10->exposure,
> > +					 ov02a10->exposure->minimum, max_expo,
> > +					 ov02a10->exposure->step,
> > +					 ov02a10->exposure->default_value);
> > +	}
> > +
> > +	/* V4L2 controls values will be applied only when power is already up */
> > +	if (!pm_runtime_get_if_in_use(&client->dev))
> > +		return 0;
> > +
> > +	switch (ctrl->id) {
> > +	case V4L2_CID_EXPOSURE:
> > +		ret = ov02a10_set_exposure(ov02a10, ctrl->val);
> > +		break;
> > +	case V4L2_CID_ANALOGUE_GAIN:
> > +		ret = ov02a10_set_gain(ov02a10, ctrl->val);
> > +		break;
> > +	case V4L2_CID_VBLANK:
> > +		ret = ov02a10_set_vblank(ov02a10, ctrl->val);
> > +		break;
> > +	case V4L2_CID_TEST_PATTERN:
> > +		ret = ov02a10_set_test_pattern(ov02a10, ctrl->val);
> > +		break;
> > +	};
> > +
> > +	pm_runtime_put(&client->dev);
> > +
> > +	return ret;
> > +}
> 
> > +{
> > +	struct fwnode_handle *ep;
> > +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +	struct v4l2_fwnode_endpoint bus_cfg = {
> > +		.bus_type = V4L2_MBUS_CSI2_DPHY
> > +	};
> > +	unsigned int i, j;
> > +	int ret;
> > +
> 
> I would rather expect fwnode assignment here to understand immediately from
> where it has been derived. Though it's up to maintainers.
> 

This would be remained, as many other drivers keep the same style.

> > +	if (!fwnode)
> > +		return -ENXIO;
> > +
> > +	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;
> > +
> > +	/* Optional indication of mipi clock lane mode */
> > +	if (bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK)
> > +		ov02a10->mipi_clock_hs_mode_enable = true;
> > +
> 
> > +	if (!bus_cfg.nr_of_link_frequencies) {
> > +		dev_err(dev, "no link frequencies defined");
> > +		ret = -EINVAL;
> > +		goto check_hwcfg_error;
> > +	}
> 
> This is redundant check. You have done the similar below when comparing j to
> nr_of_link_frequencies.
> 

bus_cfg.nr_of_link_frequencies should be checked first before using it.
For instance, OV8856 uses the same check style.

> > +
> > +	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);
> 
> It's perfectly one line.
> 

Fixed in next release.

> > +	if (!ret) {
> 
> I would rather do two things here:
> 1/ use the similar pattern as below with messaging
> 	if (!ret) {
> 		...
> 	} else {
> 		dev_dbg(...);
> 	}
> 2/ Actually use positive conditionals
> 	if (ret) {
> 		...
> 	} else {
> 		...
> 	}
> 

Fixed in next release.

> > +		switch (rotation) {
> > +		case 180:
> > +			ov02a10->upside_down = true;
> > +			ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > +			break;
> > +		case 0:
> > +			break;
> > +		default:
> > +			dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n",
> > +				 rotation);
> > +		}
> > +	}
> > +
> > +	/* Optional indication of HS VOD adjust */
> > +	ret = fwnode_property_read_u32(dev_fwnode(dev),
> > +				       "ovti,hs-vod-adjust",
> > +				       &hs_vod_adjust_cnt);
> > +	if (!ret)
> > +		ov02a10->mipi_clock_hs_vod_adjust_cnt = hs_vod_adjust_cnt;
> > +	else
> > +		dev_warn(dev, "failed to get hs vod adjust, using default\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)
> > +		ov02a10->mipi_clock_tx_speed = clock_lane_tx_speed;
> > +	else
> > +		dev_warn(dev, "failed to get mipi tx speed, using default\n");
> 
> > +	/* Get system clock (eclk) */
> > +	ov02a10->eclk = devm_clk_get(dev, "eclk");
> > +	if (IS_ERR(ov02a10->eclk)) {
> > +		dev_err(dev, "failed to get eclk\n");
> 
> > +		return -EINVAL;
> 
> Why shadow error code?
> 

Okay. Fixed in next release.

> > +	}
> 
> > +	ov02a10->pd_gpio = devm_gpiod_get(dev, "powerdown", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(ov02a10->pd_gpio)) {
> > +		dev_err(dev, "failed to get powerdown-gpios\n");
> 
> > +		return -EINVAL;
> 
> Ditto.
> 

Fixed in next release.

> > +	}
> 
> > +	ov02a10->n_rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> > +	if (IS_ERR(ov02a10->n_rst_gpio)) {
> > +		dev_err(dev, "failed to get reset-gpios\n");
> 
> > +		return -EINVAL;
> 
> Ditto.
> 

Fixed in next release.

> > +	}
> 
> > +}
>