linux-kernel - Re: [PATCH v10 3/3] media: i2c: Add support for alvium camera

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7fc8435a-6a96-447b-af85-8ae5242cabfd@wanadoo.fr>
Date:   Wed, 1 Nov 2023 15:22:18 +0100
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Tommaso Merciai <tomm.merciai@...il.com>
Cc:     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 01/11/2023 à 15:13, Sakari Ailus a écrit :
> Hi Tommaso,
> 
> On Tue, Oct 31, 2023 at 12:46:09PM +0100, Tommaso Merciai wrote:
>>>> +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.
> 
> You can safely call v4l2_fwnode_endpoint_free() on an unparsed endpoint (or
> on and endpoint the parsing of which failed). I prefer this too.
> 
>>>> +		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);
> 
> This doesn't belong here (as you didn't register it). It was missed in the
> review earlier.

Should be v4l2_async_unregister_subdev()?

CJ

> 
>> }
>>
>>
>> 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! :)
>