| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a98bca80-944c-493b-9872-75b94cd24eea@wanadoo.fr>
Date: Tue, 31 Oct 2023 11:23:24 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Tommaso Merciai <tomm.merciai@...il.com>
Cc: sakari.ailus@...ux.intel.com, martin.hecht@...et.eu,
michael.roeder@...et.eu, mhecht73@...il.com,
linuxfancy@...glegroups.com,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Hans de Goede <hdegoede@...hat.com>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
Marco Felsch <m.felsch@...gutronix.de>,
Gerald Loacker <gerald.loacker@...fvision.net>,
Andy Shevchenko <andy.shevchenko@...il.com>,
Daniel Scally <djrscally@...il.com>,
Shawn Tu <shawnx.tu@...el.com>,
Linus Walleij <linus.walleij@...aro.org>,
linux-kernel@...r.kernel.org,
Linux Media Mailing List <linux-media@...r.kernel.org>
Subject: Re: [PATCH v10 3/3] media: i2c: Add support for alvium camera
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@...il.com>
> ---
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;
> +}
> +
...
Powered by blists - more mailing lists