From: Maxime Ripard <maxime.ripard@bootlin.com> To: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Hans Verkuil <hans.verkuil@cisco.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, linux-media@vger.kernel.org, Andrzej Hajda <a.hajda@samsung.com>, Chen-Yu Tsai <wens@csie.org>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, Mark Rutland <mark.rutland@arm.com>, Rob Herring <robh+dt@kernel.org>, Frank Rowand <frowand.list@gmail.com> Subject: Re: [PATCH v2 3/5] media: sunxi: Add A10 CSI driver Date: Wed, 6 Feb 2019 22:16:05 +0100 [thread overview] Message-ID: <20190206211605.27cq2lxuv3gtyyux@flea> (raw) In-Reply-To: <20190129123949.qdmqnfaym3y42dvj@paasikivi.fi.intel.com> Hi Sakari, Thanks for your review, I have a few questions though, and the rest will be addressed in the next version. On Tue, Jan 29, 2019 at 02:39:49PM +0200, Sakari Ailus wrote: > > +static int csi_notify_complete(struct v4l2_async_notifier *notifier) > > +{ > > + struct sun4i_csi *csi = container_of(notifier, struct sun4i_csi, > > + notifier); > > + int ret; > > + > > + ret = v4l2_device_register_subdev_nodes(&csi->v4l); > > + if (ret < 0) > > + return ret; > > + > > + ret = sun4i_csi_v4l2_register(csi); > > + if (ret < 0) > > + return ret; > > + > > + return media_create_pad_link(&csi->src_subdev->entity, csi->src_pad, > > + &csi->vdev.entity, 0, > > + MEDIA_LNK_FL_ENABLED | > > + MEDIA_LNK_FL_IMMUTABLE); > > This appears to create a link directly from the sensor entity to the video > device entity. Is that intentional? I'd expect to see a CSI-2 receiver > sub-device as well, which I don't see being created by the driver. > > This is indeed a novel proposal. I have some concerns though. > > The user doesn't have access to the configured media bus format (reflecting > the format on the CSI-2 bus on receiver's side). It's thus difficult to > figure out whether the V4L2 pixel format configured on the video node > matches what the sensor outputs. Admittedly, we don't have a perfect > solution to that whenever the DMA hardware supports multiple V4L2 pixel > formats on a single media bus format. We might need to have a different > solution for this one, should it be without that receiver sub-device. > > Could you add the CSI-2 receiver sub-device, please? Even though the name of the controller is *very* confusing, this isn't a MIPI-CSI receiver, but a parallel one that supports RGB and BT656 buses. > > + csi->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT; > > Could you make it IMMUTABLE and ENABLED? If there is no need to disable it, > that is. The link is already created with those flags, and as far as I know it doesn't exist for the pads > > +static int csi_release(struct file *file) > > +{ > > + struct sun4i_csi *csi = video_drvdata(file); > > + int ret; > > + > > + mutex_lock(&csi->lock); > > + > > + ret = v4l2_fh_release(file); > > v4l2_fh_release() always returns 0. I guess it could be changed to return > void. The reason it has int is that it could be used as the release > callback as such. > > > + v4l2_pipeline_pm_use(&csi->vdev.entity, 0); > > + pm_runtime_put(csi->dev); > > + > > + mutex_unlock(&csi->lock); > > + > > + return ret; > > +} Do you want me to change the construct then? Thanks! Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
WARNING: multiple messages have this Message-ID (diff)
From: Maxime Ripard <maxime.ripard@bootlin.com> To: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Mark Rutland <mark.rutland@arm.com>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Andrzej Hajda <a.hajda@samsung.com>, Chen-Yu Tsai <wens@csie.org>, Rob Herring <robh+dt@kernel.org>, Hans Verkuil <hans.verkuil@cisco.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Frank Rowand <frowand.list@gmail.com>, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org Subject: Re: [PATCH v2 3/5] media: sunxi: Add A10 CSI driver Date: Wed, 6 Feb 2019 22:16:05 +0100 [thread overview] Message-ID: <20190206211605.27cq2lxuv3gtyyux@flea> (raw) In-Reply-To: <20190129123949.qdmqnfaym3y42dvj@paasikivi.fi.intel.com> Hi Sakari, Thanks for your review, I have a few questions though, and the rest will be addressed in the next version. On Tue, Jan 29, 2019 at 02:39:49PM +0200, Sakari Ailus wrote: > > +static int csi_notify_complete(struct v4l2_async_notifier *notifier) > > +{ > > + struct sun4i_csi *csi = container_of(notifier, struct sun4i_csi, > > + notifier); > > + int ret; > > + > > + ret = v4l2_device_register_subdev_nodes(&csi->v4l); > > + if (ret < 0) > > + return ret; > > + > > + ret = sun4i_csi_v4l2_register(csi); > > + if (ret < 0) > > + return ret; > > + > > + return media_create_pad_link(&csi->src_subdev->entity, csi->src_pad, > > + &csi->vdev.entity, 0, > > + MEDIA_LNK_FL_ENABLED | > > + MEDIA_LNK_FL_IMMUTABLE); > > This appears to create a link directly from the sensor entity to the video > device entity. Is that intentional? I'd expect to see a CSI-2 receiver > sub-device as well, which I don't see being created by the driver. > > This is indeed a novel proposal. I have some concerns though. > > The user doesn't have access to the configured media bus format (reflecting > the format on the CSI-2 bus on receiver's side). It's thus difficult to > figure out whether the V4L2 pixel format configured on the video node > matches what the sensor outputs. Admittedly, we don't have a perfect > solution to that whenever the DMA hardware supports multiple V4L2 pixel > formats on a single media bus format. We might need to have a different > solution for this one, should it be without that receiver sub-device. > > Could you add the CSI-2 receiver sub-device, please? Even though the name of the controller is *very* confusing, this isn't a MIPI-CSI receiver, but a parallel one that supports RGB and BT656 buses. > > + csi->pad.flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_MUST_CONNECT; > > Could you make it IMMUTABLE and ENABLED? If there is no need to disable it, > that is. The link is already created with those flags, and as far as I know it doesn't exist for the pads > > +static int csi_release(struct file *file) > > +{ > > + struct sun4i_csi *csi = video_drvdata(file); > > + int ret; > > + > > + mutex_lock(&csi->lock); > > + > > + ret = v4l2_fh_release(file); > > v4l2_fh_release() always returns 0. I guess it could be changed to return > void. The reason it has int is that it could be used as the release > callback as such. > > > + v4l2_pipeline_pm_use(&csi->vdev.entity, 0); > > + pm_runtime_put(csi->dev); > > + > > + mutex_unlock(&csi->lock); > > + > > + return ret; > > +} Do you want me to change the construct then? Thanks! Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-02-06 21:16 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-01-28 14:52 [PATCH v2 0/5] media: Allwinner A10 CSI support Maxime Ripard 2019-01-28 14:52 ` Maxime Ripard 2019-01-28 14:52 ` [PATCH v2 1/5] dt-bindings: media: Add Allwinner A10 CSI binding Maxime Ripard 2019-01-28 14:52 ` Maxime Ripard 2019-01-28 16:17 ` Rob Herring 2019-01-28 16:17 ` Rob Herring 2019-01-28 16:17 ` Rob Herring 2019-01-29 11:52 ` Sakari Ailus 2019-01-29 11:52 ` Sakari Ailus 2019-01-29 11:52 ` Sakari Ailus 2019-01-28 14:52 ` [PATCH v2 2/5] media: sunxi: Refactor the Makefile and Kconfig Maxime Ripard 2019-01-28 14:52 ` Maxime Ripard 2019-01-28 14:52 ` Maxime Ripard 2019-01-28 14:52 ` [PATCH v2 3/5] media: sunxi: Add A10 CSI driver Maxime Ripard 2019-01-28 14:52 ` Maxime Ripard 2019-01-29 12:39 ` Sakari Ailus 2019-01-29 12:39 ` Sakari Ailus 2019-02-06 21:16 ` Maxime Ripard [this message] 2019-02-06 21:16 ` Maxime Ripard 2019-02-08 23:17 ` Sakari Ailus 2019-02-08 23:17 ` Sakari Ailus 2019-02-06 22:59 ` Ezequiel Garcia 2019-02-06 22:59 ` Ezequiel Garcia 2019-02-08 20:19 ` Maxime Ripard 2019-02-08 20:19 ` Maxime Ripard 2019-01-28 14:52 ` [PATCH v2 4/5] ARM: dts: sun7i: Add CSI0 controller Maxime Ripard 2019-01-28 14:52 ` Maxime Ripard 2019-01-28 14:52 ` [PATCH v2 5/5] DO NOT MERGE: ARM: dts: bananapi: Add Camera support Maxime Ripard 2019-01-28 14:52 ` Maxime Ripard
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190206211605.27cq2lxuv3gtyyux@flea \ --to=maxime.ripard@bootlin.com \ --cc=a.hajda@samsung.com \ --cc=devicetree@vger.kernel.org \ --cc=frowand.list@gmail.com \ --cc=hans.verkuil@cisco.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mchehab@kernel.org \ --cc=robh+dt@kernel.org \ --cc=sakari.ailus@linux.intel.com \ --cc=thomas.petazzoni@bootlin.com \ --cc=wens@csie.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.