| Message ID | 20250328-b4-vd55g1-v1-0-8d16b4a79f29@foss.st.com |
|---|---|
| Headers | show |
| Series | media: Add support for ST VD55G1 camera sensor | expand |
On 28/03/2025 14:40, Benjamin Mugnier wrote: > + > +static void vd55g1_subdev_cleanup(struct vd55g1 *sensor) > +{ > + v4l2_async_unregister_subdev(&sensor->sd); > + v4l2_subdev_cleanup(&sensor->sd); > + media_entity_cleanup(&sensor->sd.entity); > + v4l2_ctrl_handler_free(sensor->sd.ctrl_handler); > +} > + > +static int vd55g1_err_probe(struct device *dev, int ret, char *msg) Drop, it's really useless. Don't create own abstraction layers for other systems. > +{ > + return dev_err_probe(dev, ret, msg); > +} > + > +static int vd55g1_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct vd55g1 *sensor; > + int ret; > + > + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); > + if (!sensor) > + return -ENOMEM; > + > + v4l2_i2c_subdev_init(&sensor->sd, client, &vd55g1_subdev_ops); > + sensor->i2c_client = client; > + > + ret = vd55g1_parse_dt(sensor); > + if (ret) > + return vd55g1_err_probe(dev, ret, > + "Failed to parse Device Tree."); > + > + /* Get (and check) resources : power regs, ext clock, reset gpio */ > + ret = vd55g1_get_regulators(sensor); > + if (ret) > + return vd55g1_err_probe(dev, ret, "Failed to get regulators."); > + > + sensor->xclk = devm_clk_get(dev, NULL); > + if (IS_ERR(sensor->xclk)) { Drop {} Please run scripts/checkpatch.pl on the patches and fix reported warnings. After that, run also 'scripts/checkpatch.pl --strict' on the patches and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. > + return vd55g1_err_probe(dev, PTR_ERR(sensor->xclk), No. Syntax is return dev_err_probe, not some custom wrappers over single function. > + "Failed to get xclk."); > + } > + sensor->xclk_freq = clk_get_rate(sensor->xclk); > + ret = vd55g1_prepare_clock_tree(sensor); > + if (ret) > + return ret; > + > + sensor->reset_gpio = > + devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); Odd wrapping. This should be one line with optionally wrapped last argument. > + if (IS_ERR(sensor->reset_gpio)) > + return vd55g1_err_probe(dev, PTR_ERR(sensor->reset_gpio), > + "Failed to get reset gpio."); > + > + sensor->regmap = devm_cci_regmap_init_i2c(client, 16); > + if (IS_ERR(sensor->regmap)) > + return vd55g1_err_probe(dev, PTR_ERR(sensor->regmap), > + "Failed to init regmap."); > + > + /* Detect if sensor is present and if its revision is supported */ > + ret = vd55g1_power_on(dev); > + if (ret) > + return ret; > + > + ret = vd55g1_subdev_init(sensor); > + if (ret) { > + dev_err(dev, "V4l2 init failed : %d", ret); > + goto err_power_off; > + } > + > + ret = v4l2_async_register_subdev(&sensor->sd); > + if (ret) { > + dev_err(dev, "async subdev register failed %d", ret); > + goto err_subdev; > + } > + > + /* Enable pm_runtime and power off the sensor */ > + pm_runtime_set_active(dev); > + pm_runtime_get_noresume(dev); > + pm_runtime_enable(dev); > + pm_runtime_set_autosuspend_delay(dev, 4000); > + pm_runtime_use_autosuspend(dev); > + pm_runtime_mark_last_busy(dev); > + > + dev_dbg(dev, "vd55g1 probe successfully"); Drop. Kernel already provides you such debugging/tracing. Best regards, Krzysztof
Hi Krzysztof, Thank you for your review. On 3/29/25 06:46, Krzysztof Kozlowski wrote: > On 28/03/2025 14:40, Benjamin Mugnier wrote: >> + >> +static void vd55g1_subdev_cleanup(struct vd55g1 *sensor) >> +{ >> + v4l2_async_unregister_subdev(&sensor->sd); >> + v4l2_subdev_cleanup(&sensor->sd); >> + media_entity_cleanup(&sensor->sd.entity); >> + v4l2_ctrl_handler_free(sensor->sd.ctrl_handler); >> +} >> + >> +static int vd55g1_err_probe(struct device *dev, int ret, char *msg) > > Drop, it's really useless. Don't create own abstraction layers for other > systems. > >> +{ >> + return dev_err_probe(dev, ret, msg); >> +} >> + >> +static int vd55g1_probe(struct i2c_client *client) >> +{ >> + struct device *dev = &client->dev; >> + struct vd55g1 *sensor; >> + int ret; >> + >> + sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL); >> + if (!sensor) >> + return -ENOMEM; >> + >> + v4l2_i2c_subdev_init(&sensor->sd, client, &vd55g1_subdev_ops); >> + sensor->i2c_client = client; >> + >> + ret = vd55g1_parse_dt(sensor); >> + if (ret) >> + return vd55g1_err_probe(dev, ret, >> + "Failed to parse Device Tree."); >> + >> + /* Get (and check) resources : power regs, ext clock, reset gpio */ >> + ret = vd55g1_get_regulators(sensor); >> + if (ret) >> + return vd55g1_err_probe(dev, ret, "Failed to get regulators."); >> + >> + sensor->xclk = devm_clk_get(dev, NULL); >> + if (IS_ERR(sensor->xclk)) { > > Drop {} > > Please run scripts/checkpatch.pl on the patches and fix reported > warnings. After that, run also 'scripts/checkpatch.pl --strict' on the > patches and (probably) fix more warnings. Some warnings can be ignored, > especially from --strict run, but the code here looks like it needs a > fix. Feel free to get in touch if the warning is not clear. > While it should, checkpatch does not complain about that. Maybe because the function call is on two lines ? Anyway thanks a lot for pointing this. Fixed. >> + return vd55g1_err_probe(dev, PTR_ERR(sensor->xclk), > > No. Syntax is return dev_err_probe, not some custom wrappers over single > function. > >> + "Failed to get xclk."); >> + } >> + sensor->xclk_freq = clk_get_rate(sensor->xclk); >> + ret = vd55g1_prepare_clock_tree(sensor); >> + if (ret) >> + return ret; >> + >> + sensor->reset_gpio = >> + devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); > > Odd wrapping. This should be one line with optionally wrapped last argument. > >> + if (IS_ERR(sensor->reset_gpio)) >> + return vd55g1_err_probe(dev, PTR_ERR(sensor->reset_gpio), >> + "Failed to get reset gpio."); >> + >> + sensor->regmap = devm_cci_regmap_init_i2c(client, 16); >> + if (IS_ERR(sensor->regmap)) >> + return vd55g1_err_probe(dev, PTR_ERR(sensor->regmap), >> + "Failed to init regmap."); >> + >> + /* Detect if sensor is present and if its revision is supported */ >> + ret = vd55g1_power_on(dev); >> + if (ret) >> + return ret; >> + >> + ret = vd55g1_subdev_init(sensor); >> + if (ret) { >> + dev_err(dev, "V4l2 init failed : %d", ret); >> + goto err_power_off; >> + } >> + >> + ret = v4l2_async_register_subdev(&sensor->sd); >> + if (ret) { >> + dev_err(dev, "async subdev register failed %d", ret); >> + goto err_subdev; >> + } >> + >> + /* Enable pm_runtime and power off the sensor */ >> + pm_runtime_set_active(dev); >> + pm_runtime_get_noresume(dev); >> + pm_runtime_enable(dev); >> + pm_runtime_set_autosuspend_delay(dev, 4000); >> + pm_runtime_use_autosuspend(dev); >> + pm_runtime_mark_last_busy(dev); >> + >> + dev_dbg(dev, "vd55g1 probe successfully"); > > Drop. Kernel already provides you such debugging/tracing. Interesting, do you know how to enable this kind of tracing ? > > Best regards, > Krzysztof
Hi, This serie adds support for the STMicroelectronics VD55G1 camera sensor. The VD55G1 is a monochrome global shutter camera with a 804x704 maximum resolution with RAW8 and RAW10 bytes per pixel. Datasheets and other documentation can be found at st.com [1]. A lot of inspiration was taken from the imx219 and the vd56g3 serie. It is compatible with libcamera. Tested on Raspberry Pi 4 and 5, with and without libcamera. [1] https://www.st.com/en/imaging-and-photonics-solutions/vd55g1.html#documentation Regards, Benjamin --- Benjamin Mugnier (2): media: dt-bindings: Add ST VD55G1 camera sensor binding media: i2c: Add driver for ST VD55G1 camera sensor .../devicetree/bindings/media/i2c/st,vd55g1.yaml | 135 ++ MAINTAINERS | 10 + drivers/media/i2c/Kconfig | 8 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/vd55g1.c | 2001 ++++++++++++++++++++ 5 files changed, 2155 insertions(+) --- base-commit: b2c4bf0c102084e77ed1b12090d77a76469a6814 change-id: 20250225-b4-vd55g1-bdb90dbe39b3 Best regards,