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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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?

...

> +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.

> +		}
> +	};
> +
> +	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.

> +	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...

> +	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...


> +	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?

> +	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.

> +	};
> +	unsigned int i, j;
> +	int ret;

> +	if (!fwnode)
> +		return -ENXIO;

A bit strange error code here.

> +
> +	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.

> +
> +	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?

> +		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.

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