lkml.org 
[lkml]   [2023]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v10 3/3] media: i2c: Add support for alvium camera
    Hi Cristophe, Sakari,

    On Wed, Nov 01, 2023 at 06:16:55PM +0100, Tommaso Merciai wrote:
    > Hi Cristophe,
    >
    > On Wed, Nov 01, 2023 at 03:22:18PM +0100, Christophe JAILLET wrote:
    > > 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()?
    >
    > Mmmm good point.
    > I think actually I'm missing this.
    > You are completely right. Thanks.

    Checking imx219.c/imx290.c as reference
    I think the right remove function would be:

    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;

    v4l2_async_unregister_subdev(sd);
    alvium_subdev_cleanup(alvium);
    /*
    * 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);
    }

    Regards,
    Tommaso

    >
    > Regards,
    > Tommaso
    >
    > >
    > > 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! :)
    > > >
    > >

    \
     
     \ /
      Last update: 2023-11-01 21:13    [W:2.893 / U:0.536 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site