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()
> +#define OV02A10_MASK_8_BITS 0xff
GENMASK()
> +#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.
> +static const s64 link_freq_menu_items[] = {
> + OV02A10_LINK_FREQ_390MHZ
Better to keep comma here.
> +};
> +{
> + 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;
?
> +
> + 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);
?
> +
> + 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.
> + 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...
> +
> + 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.
> +
> +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?
> +
> + /* 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.
> + 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.
> +
> + 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.
> + 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 {
...
}
> + 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?
> + }
> + 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.
> + }
> + 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.
> + }
> +}
--
With Best Regards,
Andy Shevchenko
