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