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
