Hi Jacopo, thanks for the review On 14/03/2021 10:40, Jacopo Mondi wrote: >> +#define OV5693_REG_CHIP_ID_H 0x300A >> +#define OV5693_REG_CHIP_ID_L 0x300B > Please use lower case letters in hex identifiers Will do >> + >> +/* Miscellaneous */ >> +#define OV5693_NUM_MBUS_FMTS 1 > Nit: Only used in one place, maybe could be removed Okedokey >> +#define OV5693_NUM_RESOLUTIONS ARRAY_SIZE(ov5693_resolutions) >> +struct ov5693_resolution ov5693_resolutions[] = { >> + { >> + .desc = "ov5693_2592x1944_30fps", >> + .fps = 30, >> + >> + .crop_start_x = 16, >> + .offset_x = 0, >> + .output_size_x = 2592, >> + .crop_end_x = 2608, >> + .hts = 2688, > Aren't the crop information here duplicated in the .crop field ? Do > you need to have them separate ? They don't need to separate, but I wonder if it's a bit confusing because only crop.left and crop.top can be used. We couldn't draw the output sizes from crop.width or crop.height because those values are pre-scaling/binning. So it just seemed a bit clearer to treat them separately. >> + >> + .crop_start_y = 6, >> + .offset_y = 0, >> + .output_size_y = 1944, >> + .crop_end_y = 1950, >> + .vts = 1984, >> + >> + .inc_x_odd = 1, >> + .inc_x_even = 1, >> + .inc_y_odd = 1, >> + .inc_y_even = 1, >> + >> + .crop = { >> + .left = 16, >> + .top = 6, >> + .width = 2592, >> + .height = 1944 >> + }, >> + }, >> + { >> + .desc = "ov5693_1920x1080_30fps", > What's desc used for ? Actually it's a leftover from the atomisp version that I've been using as a handy label...it never even crossed my mind to change it. But it should be a comment of course, I'll swap to that. >> +static int ov5693_update_bits(struct ov5693_device *ov5693, u16 address, >> + u16 mask, u16 bits) >> +{ >> + u8 value = 0; >> + int ret; >> + >> + ret = ov5693_read_reg(ov5693, address, &value); >> + if (ret) >> + return ret; >> + >> + value &= ~mask; >> + value |= bits; >> + >> + ov5693_write_reg(ov5693, address, value, &ret); > Shouldn't ret be initialized to 0 ? Isn't that done by the return of ov5693_read_reg()? > >> + if (ret) >> + return ret; >> + >> + return 0; > You can just return ret I guess Oops, yes. Thanks > >> +} >> + >> +/* V4L2 Controls Functions */ >> + >> +static int ov5693_flip_vert_configure(struct ov5693_device *ov5693, bool enable) >> +{ >> + u8 bits = OV5693_FORMAT1_FLIP_VERT_ISP_EN | >> + OV5693_FORMAT1_FLIP_VERT_SENSOR_EN; >> + int ret; >> + >> + ret = ov5693_update_bits(ov5693, OV5693_FORMAT1_REG, bits, >> + enable ? bits : 0); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int ov5693_flip_horz_configure(struct ov5693_device *ov5693, bool enable) >> +{ >> + u8 bits = OV5693_FORMAT2_FLIP_HORZ_ISP_EN | >> + OV5693_FORMAT2_FLIP_HORZ_SENSOR_EN; >> + int ret; >> + >> + ret = ov5693_update_bits(ov5693, OV5693_FORMAT2_REG, bits, >> + enable ? bits : 0); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int ov5693_get_exposure(struct ov5693_device *ov5693, s32 *value) >> +{ > Out of curiosity, what's the unit of the exposure for this chip ? 1/16ths of a line, but the 4 fractional bits are unsupported, so effectively in units of lines. >> + u8 exposure_hh = 0, exposure_h = 0, exposure_l = 0; >> + int ret; >> + >> + ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_HH_REG, &exposure_hh); >> + if (ret) >> + return ret; >> + >> + ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_H_REG, &exposure_h); >> + if (ret) >> + return ret; >> + >> + ret = ov5693_read_reg(ov5693, OV5693_EXPOSURE_L_CTRL_L_REG, &exposure_l); >> + if (ret) >> + return ret; >> + >> + /* The lowest 4 bits are unsupported fractional bits */ >> + *value = ((exposure_hh << 16) | (exposure_h << 8) | exposure_l) >> 4; >> + >> + return 0; >> +} >> + >> +static int ov5693_exposure_configure(struct ov5693_device *ov5693, u32 exposure) >> +{ >> + int ret = 0; > No intialization required here, it will be overwritten below. Thanks >> + >> + gpiod_set_value_cansleep(ov5693->reset, 1); >> + gpiod_set_value_cansleep(ov5693->powerdown, 1); >> + >> + ret = clk_prepare_enable(ov5693->clk); >> + if (ret) { >> + dev_err(ov5693->dev, "Failed to enable clk\n"); >> + goto fail_power; >> + } >> + >> + ret = regulator_bulk_enable(OV5693_NUM_SUPPLIES, ov5693->supplies); >> + if (ret) { >> + dev_err(ov5693->dev, "Failed to enable regulators\n"); >> + goto fail_power; >> + } >> + >> + gpiod_set_value_cansleep(ov5693->reset, 0); >> + gpiod_set_value_cansleep(ov5693->powerdown, 0); > I would release powerdown first then reset OK, I'll switch to that. >> + >> + usleep_range(20000, 25000); > Quite a long delay, does the chip manual quantify it ? No actually, it's 8192 xvclk cycles - I increased this whilst debugging something and and forgot to change it back. I'll drop it down. > +static int ov5693_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *format) > +{ > + struct ov5693_device *ov5693 = to_ov5693_sensor(sd); > + const struct ov5693_resolution *mode; > + int exposure_max; > + int ret = 0; > + int hblank; > + > + if (format->pad) > + return -EINVAL; > The core checks this for you Ah - TIL, thanks. >> + >> + mutex_lock(&ov5693->lock); >> + >> + mode = v4l2_find_nearest_size(ov5693_resolutions, >> + OV5693_NUM_RESOLUTIONS, output_size_x, >> + output_size_y, format->format.width, >> + format->format.height); >> + >> + if (!mode) >> + return -EINVAL; > Bailing out without releseing the mutex ? > I would actually only lock when V4L2_SUBDEV_FORMAT_ACTIVE ... Ah whoops, sorry. Yes that's a good idea, I'll do that. >> + ov5693->mode = mode; >> + >> + /* Update limits and set FPS to default */ >> + __v4l2_ctrl_modify_range(ov5693->ctrls.vblank, >> + OV5693_TIMING_MIN_VTS, >> + OV5693_TIMING_MAX_VTS - mode->output_size_y, >> + 1, mode->vts - mode->output_size_y); >> + __v4l2_ctrl_s_ctrl(ov5693->ctrls.vblank, >> + mode->vts - mode->output_size_y); >> + >> + hblank = mode->hts - mode->output_size_x; >> + __v4l2_ctrl_modify_range(ov5693->ctrls.hblank, hblank, hblank, 1, >> + hblank); > Aren't there any VTS or HTS margins for this chip ? Not that I'm aware of, but it could easily simply be me not understanding something. I assume you're referring to something like the minimum VTS defined for modes in some drivers, like the ov5670 [2]: { .width = 1296, .height = 972, .vts_def = OV5670_VTS_30FPS, .vts_min = 996, .reg_list = { .num_of_regs = ARRAY_SIZE(mode_1296x972_regs), .regs = mode_1296x972_regs, }, .link_freq_index = OV5670_LINK_FREQ_422MHZ_INDEX, }, Right? I thought they just reflected what the author saw as sensible minimums rather than a hardware limitation or anything. > >> + >> + exposure_max = mode->vts - OV5693_INTEGRATION_TIME_MARGIN; >> + __v4l2_ctrl_modify_range(ov5693->ctrls.exposure, >> + ov5693->ctrls.exposure->minimum, exposure_max, >> + ov5693->ctrls.exposure->step, >> + ov5693->ctrls.exposure->val < exposure_max ? >> + ov5693->ctrls.exposure->val : exposure_max); > min(ov5693->ctrls.exposure->val, exposure_max) ? Hah derp, of course. That was silly - thanks! >> +static int ov5693_s_stream(struct v4l2_subdev *sd, int enable) >> +{ >> + struct ov5693_device *ov5693 = to_ov5693_sensor(sd); >> + int ret; >> + >> + if (enable) { >> + ret = pm_runtime_get_sync(ov5693->dev); >> + if (ret < 0) >> + goto err_power_down; >> + } >> + >> + ret = __v4l2_ctrl_handler_setup(&ov5693->ctrls.handler); > Do you want this when !enable ? No I guess not, good point - thanks >> + if (ret) >> + goto err_power_down; >> + >> + mutex_lock(&ov5693->lock); >> + ret = ov5693_sw_standby(ov5693, !enable); > if this fail the ov5693->streaming flag should probably not be updated It shouldn't be right? It has the goto err_power_down below. > >> + mutex_unlock(&ov5693->lock); >> + >> + if (ret) >> + goto err_power_down; >> + ov5693->streaming = !!enable; >> + >> + if (!enable) >> + pm_runtime_put(ov5693->dev); >> + >> + return 0; >> +err_power_down: >> + pm_runtime_put_noidle(ov5693->dev); >> + return ret; >> +} >> + >> +/* Sensor and Driver Configuration Functions */ >> + >> +static int ov5693_init_controls(struct ov5693_device *ov5693) >> +{ >> + const struct v4l2_ctrl_ops *ops = &ov5693_ctrl_ops; >> + struct v4l2_fwnode_device_properties props; >> + int vblank_max, vblank_def; >> + int exposure_max; >> + int hblank; >> + int ret; >> + >> + ret = v4l2_ctrl_handler_init(&ov5693->ctrls.handler, 14); > I do count 10 controls, am I mistkaen ? 12, v4l2_ctrl_new_fwnode_properties() creates rotation and orientation controls. I'll drop to that. >> +static int ov5693_probe(struct i2c_client *client) >> +{ >> + struct fwnode_handle *fwnode = dev_fwnode(&client->dev); >> + struct fwnode_handle *endpoint; >> + struct ov5693_device *ov5693; >> + u32 clk_rate; >> + int ret = 0; >> + >> + endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL); >> + if (!endpoint && !IS_ERR_OR_NULL(fwnode->secondary)) >> + endpoint = fwnode_graph_get_next_endpoint(fwnode->secondary, NULL); >> + if (!endpoint) >> + return -EPROBE_DEFER; > mmm, if the endpoint is not specified in firmware it's an > unrecoverable error, deplaying probe won't help ? Or is this due to > the 'secondary' (which I'm not actually sure what it represents :) Not so unrecoverable any more; the ipu3-cio2 driver can now build endpoints from software nodes [1], which is what's happening for the Microsoft Surface devices. The sensor driver can actually probe fine before the ipu3-cio2 one, but in that case because the firmware node properties won't have been created yet, v4l2_ctrl_new_fwnode_properties() can't create the rotation and orientation controls from them. So that's why it defers, to give ipu3-cio2 time to probe. I think it's probably better to extend fwnode_graph_get_next_endpoint() to do the check of fwnode->secondary though in retrospect. >> + >> + ov5693 = devm_kzalloc(&client->dev, sizeof(*ov5693), GFP_KERNEL); >> + if (!ov5693) >> + return -ENOMEM; >> + >> + ov5693->client = client; >> + ov5693->dev = &client->dev; >> + >> + mutex_init(&ov5693->lock); >> + >> + v4l2_i2c_subdev_init(&ov5693->sd, client, &ov5693_ops); >> + >> + ov5693->clk = devm_clk_get(&client->dev, "xvclk"); >> + if (IS_ERR(ov5693->clk)) { >> + dev_err(&client->dev, "Error getting clock\n"); >> + return PTR_ERR(ov5693->clk); >> + } >> + >> + clk_rate = clk_get_rate(ov5693->clk); >> + if (clk_rate != OV5693_XVCLK_FREQ) { >> + dev_err(&client->dev, "Unsupported clk freq %u, expected %u\n", >> + clk_rate, OV5693_XVCLK_FREQ); >> + return -EINVAL; >> + } >> + >> + ret = ov5693_configure_gpios(ov5693); >> + if (ret) >> + return ret; >> + >> + ret = ov5693_get_regulators(ov5693); >> + if (ret) { >> + dev_err(&client->dev, "Error fetching regulators\n"); >> + return ret; >> + } >> + >> + ov5693->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; >> + ov5693->pad.flags = MEDIA_PAD_FL_SOURCE; >> + ov5693->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; >> + ov5693->mode = &ov5693_resolutions[OV5693_NUM_RESOLUTIONS - 1]; >> + >> + ret = ov5693_init_controls(ov5693); >> + if (ret) >> + return ret; >> + >> + ret = media_entity_pads_init(&ov5693->sd.entity, 1, &ov5693->pad); >> + if (ret) >> + goto err_ctrl_handler_free; >> + >> + /* >> + * We need the driver to work in the event that pm runtime is disable in >> + * the kernel, so power up and verify the chip now. In the event that >> + * runtime pm is disabled this will leave the chip on, so that streaming >> + * will work. >> + */ >> + >> + ret = ov5693_sensor_powerup(ov5693); >> + if (ret) >> + goto err_media_entity_cleanup; >> + >> + ret = ov5693_detect(ov5693); >> + if (ret) >> + goto err_powerdown; >> + >> + pm_runtime_set_active(&client->dev); >> + pm_runtime_get_noresume(&client->dev); >> + pm_runtime_enable(&client->dev); >> + >> + ret = v4l2_async_register_subdev_sensor_common(&ov5693->sd); >> + if (ret) { >> + dev_err(&client->dev, "failed to register V4L2 subdev: %d", >> + ret); >> + goto err_pm_runtime; >> + } >> + >> + pm_runtime_set_autosuspend_delay(&client->dev, 1000); >> + pm_runtime_use_autosuspend(&client->dev); >> + pm_runtime_put_autosuspend(&client->dev); > This sequence smells funny to me, but that's maybe beacuse of the > usage of v4l2_async_register_subdev_sensor_common(). And it's anyway > the same as in the CCS driver, so I assume it's ok I also assume it's ok, on the grounds that it's on Laurent's advice :D >> + >> + return ret; >> + >> +err_pm_runtime: >> + pm_runtime_disable(&client->dev); >> + pm_runtime_put_noidle(&client->dev); >> +err_powerdown: >> + ov5693_sensor_powerdown(ov5693); >> +err_media_entity_cleanup: >> + media_entity_cleanup(&ov5693->sd.entity); >> +err_ctrl_handler_free: >> + v4l2_ctrl_handler_free(&ov5693->ctrls.handler); >> + >> + return ret; >> +} >> + >> +static int ov5693_remove(struct i2c_client *client) >> +{ >> + struct v4l2_subdev *sd = i2c_get_clientdata(client); >> + struct ov5693_device *ov5693 = to_ov5693_sensor(sd); >> + >> + v4l2_async_unregister_subdev(sd); >> + media_entity_cleanup(&ov5693->sd.entity); >> + v4l2_ctrl_handler_free(&ov5693->ctrls.handler); >> + mutex_destroy(&ov5693->lock); >> + >> + /* >> + * Disable runtime PM. In case runtime PM is disabled in the kernel, >> + * make sure to turn power off manually. >> + */ >> + pm_runtime_disable(&client->dev); >> + if (!pm_runtime_status_suspended(&client->dev)) >> + ov5693_sensor_powerdown(ov5693); >> + pm_runtime_set_suspended(&client->dev); >> + >> + return 0; >> +} >> + >> +static const struct dev_pm_ops ov5693_pm_ops = { >> + SET_RUNTIME_PM_OPS(ov5693_sensor_suspend, ov5693_sensor_resume, NULL) >> +}; >> + >> +static const struct acpi_device_id ov5693_acpi_match[] = { >> + {"INT33BE"}, >> + {}, >> +}; >> +MODULE_DEVICE_TABLE(acpi, ov5693_acpi_match); >> + >> +static struct i2c_driver ov5693_driver = { >> + .driver = { >> + .name = "ov5693", >> + .acpi_match_table = ov5693_acpi_match, >> + .pm = &ov5693_pm_ops, >> + }, >> + .probe_new = ov5693_probe, >> + .remove = ov5693_remove, >> +}; >> +module_i2c_driver(ov5693_driver); >> + >> +MODULE_DESCRIPTION("A low-level driver for OmniVision 5693 sensors"); >> +MODULE_LICENSE("GPL"); > Maybe you want MODULE_AUTHOR too ? I was going to just leave it as the "contributions by" comment at the top > > Questions for maintainers, drivers for sensors developed with ACPI > support do not need bindings, right ? > > Mostly minor comments though, the driver looks good to me, and with > minor fixes addressed you can add > Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx> Thanks! Much appreciated. > > Thanks > j > >> -- >> 2.25.1 >> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=803abec64ef9d31ba068088e90fc20556ab5f605 [2] https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov5670.c#L1751