Le 20/10/2023 à 16:13, Tommaso Merciai a écrit :
The Alvium camera is shipped with sensor + isp in the same housing. The camera can be equipped with one out of various sensor and abstract the user from this. Camera is connected via MIPI CSI-2. Most of the camera module features are supported, with the main exception being fw update. The driver provides all mandatory, optional and recommended V4L2 controls for maximum compatibility with libcamera References: - https://www.alliedvision.com/en/products/embedded-vision-solutions Signed-off-by: Tommaso Merciai <tomm.merciai@xxxxxxxxx> ---
Hi, a few nits and a question at the end.
+static int alvium_setup_mipi_fmt(struct alvium_dev *alvium) +{ + int avail_fmt_cnt = 0; + int sz = 0; + int fmt = 0; + + alvium->alvium_csi2_fmt = NULL; + + /* calculate fmt array size */ + for (fmt = 0; fmt < ALVIUM_NUM_SUPP_MIPI_DATA_FMT; fmt++) { + if (alvium->is_mipi_fmt_avail[alvium_csi2_fmts[fmt].fmt_av_bit]) + if ((!alvium_csi2_fmts[fmt].is_raw) || + (alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit])) + sz++; + } + + /* init alvium_csi2_fmt array */ + alvium->alvium_csi2_fmt_n = sz; + alvium->alvium_csi2_fmt = kmalloc_array(sz, + sizeof(struct alvium_pixfmt),
This could be on the previous line.
+ GFP_KERNEL); + + /* Create the alvium_csi2 fmt array from formats available */ + for (fmt = 0; fmt < ALVIUM_NUM_SUPP_MIPI_DATA_FMT; fmt++) { + if (!alvium->is_mipi_fmt_avail[alvium_csi2_fmts[fmt].fmt_av_bit]) + continue; + + if ((!alvium_csi2_fmts[fmt].is_raw) || + (alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit])) { + alvium->alvium_csi2_fmt[avail_fmt_cnt] = + alvium_csi2_fmts[fmt]; + avail_fmt_cnt++; + } + } + + return 0; +}
...
+static int alvium_s_frame_interval(struct v4l2_subdev *sd, + struct v4l2_subdev_frame_interval *fi) +{ + struct alvium_dev *alvium = sd_to_alvium(sd); + int ret; + + if (alvium->streaming) + return -EBUSY; + + ret = alvium_set_frame_interval(alvium, fi); + if (!ret) { + ret = alvium_set_frame_rate(alvium); + if (ret) + return -EIO;
Why not ret?
+ } + + return ret; +}
...
+static int alvium_get_dt_data(struct alvium_dev *alvium) +{ + struct device *dev = &alvium->i2c_client->dev; + struct fwnode_handle *fwnode = dev_fwnode(dev); + struct fwnode_handle *endpoint; + int ret = -EINVAL; + + if (!fwnode) + return -EINVAL; + + /* Only CSI2 is supported for now: */ + alvium->ep.bus_type = V4L2_MBUS_CSI2_DPHY; + + endpoint = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0); + if (!endpoint) { + dev_err(dev, "endpoint node not found\n"); + return -EINVAL; + } + + if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &alvium->ep)) { + dev_err(dev, "could not parse endpoint\n"); + goto error_out;
This could go to another label to be less confusing, but v4l2_fwnode_endpoint_free() looks to be a no-op here, so good enough.
+ } + + if (!alvium->ep.nr_of_link_frequencies) { + dev_err(dev, "no link frequencies defined"); + goto error_out; + } + + return 0; + +error_out: + v4l2_fwnode_endpoint_free(&alvium->ep); + fwnode_handle_put(endpoint); + + return ret; +} + +static int alvium_power_on(struct alvium_dev *alvium, bool on) +{ + int ret = 0;
Useless init.
+ + if (!on) + return regulator_disable(alvium->reg_vcc); + + ret = regulator_enable(alvium->reg_vcc); + if (ret) + return ret; + + /* alvium boot time 7s*/
space missing before */
+ msleep(7000); + return 0; +}
...
+static int alvium_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct alvium_dev *alvium; + int ret; + + alvium = devm_kzalloc(dev, sizeof(*alvium), GFP_KERNEL); + if (!alvium) + return -ENOMEM; + + alvium->i2c_client = client; + + alvium->regmap = devm_cci_regmap_init_i2c(client, 16); + if (IS_ERR(alvium->regmap)) + return PTR_ERR(alvium->regmap); + + ret = alvium_get_dt_data(alvium); + if (ret) + return ret; + + alvium->reg_vcc = devm_regulator_get_optional(dev, "vcc-ext-in"); + if (IS_ERR(alvium->reg_vcc)) + return dev_err_probe(dev, PTR_ERR(alvium->reg_vcc), + "no vcc-ext-in regulator provided\n"); + + ret = alvium_power_on(alvium, true); + if (ret) + goto err_powerdown; + + if (!alvium_is_alive(alvium)) { + dev_err(dev, "Device detection failed: %d\n", ret);
Nit: Here and below, dev_err_probe() could also be used to display the error code in a human readable way.
+ ret = -ENODEV; + goto err_powerdown; + } + + ret = alvium_get_hw_info(alvium); + if (ret) { + dev_err(dev, "get_hw_info fail %d\n", ret); + goto err_powerdown; + } + + ret = alvium_hw_init(alvium); + if (ret) { + dev_err(dev, "hw_init fail %d\n", ret); + goto err_powerdown; + } + + ret = alvium_setup_mipi_fmt(alvium); + if (ret) { + dev_err(dev, "setup_mipi_fmt fail %d\n", ret); + goto err_powerdown; + } + + /* + * Enable runtime PM without autosuspend: + * + * Don't use pm autosuspend (alvium have ~7s boot time). + * Alvium has been powered manually: + * - mark it as active + * - increase the usage count without resuming the device. + */ + pm_runtime_set_active(dev); + pm_runtime_get_noresume(dev); + pm_runtime_enable(dev); + + /* Initialize the V4L2 subdev. */ + ret = alvium_subdev_init(alvium); + if (ret) + goto err_pm; + + ret = v4l2_async_register_subdev(&alvium->sd); + if (ret < 0) { + dev_err(dev, "Could not register v4l2 device\n"); + goto err_subdev; + } + + return 0; + +err_subdev: + alvium_subdev_cleanup(alvium);
Should this also be called by the remove function? Or is it already handled by an un-register mechanism? CJ
+err_pm: + pm_runtime_disable(dev); + pm_runtime_put_noidle(dev); +err_powerdown: + alvium_power_on(alvium, false); + + return ret; +} +
...