Message-ID: <ZUDpAR3ZyBVrUyat@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
Date:   Tue, 31 Oct 2023 12:46:09 +0100
From:   Tommaso Merciai <tomm.merciai@...il.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
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

Hi Christophe, Laurent, Sakari,
Thanks for the comments.

On Tue, Oct 31, 2023 at 11:23:24AM +0100, Christophe JAILLET wrote:
> 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

Nitpicker  :)
Thanks

> 
> > +						     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?

Right I can return ret aswell here.
Thanks.

> 
> > +	}
> > +
> > +	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.

Thanks for the comment.
To be honest right now this is clear to me
I prefere to stay on the following :)
Prefer to keep just only one path.

> 
> > +	}
> > +
> > +	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.

Right.

> 
> > +
> > +	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 */

Oooks :)
Thanks.

> 
> > +	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.

Oks I switch here and below to use:
dev_err_probe(dev, ret, "...);


> 
> > +		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?

Right, I miss this.
Thanks.
I put this to remove function like:

static void alvium_remove(struct i2c_client *client)
{
	struct v4l2_subdev *sd = i2c_get_clientdata(client);
	struct alvium_dev *alvium = sd_to_alvium(sd);
	struct device *dev = &alvium->i2c_client->dev;

	/*
	 * Disable runtime PM. In case runtime PM is disabled in the kernel,
	 * make sure to turn power off manually.
	 */
	pm_runtime_disable(dev);
	if (!pm_runtime_status_suspended(dev))
		alvium_power_on(alvium, false);
	pm_runtime_set_suspended(dev);

	alvium_subdev_cleanup(alvium);
	i2c_unregister_device(alvium->i2c_client);
}


If for you Cristophe, Sakari, Laurent,
it's ok I prefer to skip v11 that I sent this morning too early.
I collected hints from Cristophe right now and I plan to send v12
this afternoon/evening if for you all is ok.

https://github.com/avs-sas/linux/blob/tm/media_stage/v6.6.0-rc3/alvium_drv/v12/drivers/media/i2c/alvium-csi2.c

Please let me know.

Thanks again to all! :)

Regards,
Tommaso

> 
> 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