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