Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Maxime Ripard <maxime@cerno.tech>
To: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
	linux-sunxi@googlegroups.com,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	Yong Deng <yong.deng@magewell.com>,
	Kishon Vijay Abraham I <kishon@ti.com>,
	Vinod Koul <vkoul@kernel.org>,
	Helen Koike <helen.koike@collabora.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Hans Verkuil <hans.verkuil@cisco.com>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	kevin.lhopital@hotmail.com
Subject: Re: [PATCH 08/14] media: sunxi: Add support for the A31 MIPI CSI-2 controller
Date: Wed, 4 Nov 2020 19:56:50 +0100
Message-ID: <20201104185650.ii7dlekjtfar2xpp@gilmour.lan> (raw)
In-Reply-To: <20201104113458.GC287014@aptenodytes>


[-- Attachment #1: Type: text/plain, Size: 5883 bytes --]

On Wed, Nov 04, 2020 at 12:34:58PM +0100, Paul Kocialkowski wrote:
> > > +	regmap_write(regmap, SUN6I_MIPI_CSI2_CFG_REG,
> > > +		     SUN6I_MIPI_CSI2_CFG_CHANNEL_MODE(1) |
> > > +		     SUN6I_MIPI_CSI2_CFG_LANE_COUNT(lanes_count));
> > 
> > It's not really clear what the channel is here? The number of virtual
> > channels? Something else?
> 
> That's somewhat described in the controller documentation. Channels refers to
> physical channels of the controller, which can be used to redirect data
> matching either a specific data type, a specific virtual channel, or both.
> There's a somewhat similar concept of channels in the CSI controller too.
> 
> We're currently only using one...
> 
> > > +	regmap_write(regmap, SUN6I_MIPI_CSI2_VCDT_RX_REG,
> > > +		     SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(3, 3) |
> > > +		     SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(2, 2) |
> > > +		     SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(1, 1) |
> > > +		     SUN6I_MIPI_CSI2_VCDT_RX_CH_VC(0, 0) |
> > > +		     SUN6I_MIPI_CSI2_VCDT_RX_CH_DT(0, data_type));
> 
> ... but it's safer to configure them all to virtual channel numbers so we don't
> end up with multiple channels matching virtual channel 0.
> 
> I'll add a comment about that.

Maybe we should have pads for all of them then, even if we don't support
changing anything?

> > > +static const struct v4l2_subdev_pad_ops sun6i_mipi_csi2_subdev_pad_ops = {
> > > +	.enum_mbus_code		= sun6i_mipi_csi2_enum_mbus_code,
> > > +	.get_fmt		= sun6i_mipi_csi2_get_fmt,
> > > +	.set_fmt		= sun6i_mipi_csi2_set_fmt,
> > > +	.enum_frame_size	= sun6i_mipi_csi2_enum_frame_size,
> > > +	.enum_frame_interval	= sun6i_mipi_csi2_enum_frame_interval,
> > > +};
> > > +
> > > +/* Subdev */
> > > +
> > > +static const struct v4l2_subdev_ops sun6i_mipi_csi2_subdev_ops = {
> > > +	.core		= &sun6i_mipi_csi2_subdev_core_ops,
> > > +	.video		= &sun6i_mipi_csi2_subdev_video_ops,
> > > +	.pad		= &sun6i_mipi_csi2_subdev_pad_ops,
> > > +};
> > > +
> > > +/* Notifier */
> > > +
> > > +static int sun6i_mipi_csi2_notifier_bound(struct v4l2_async_notifier *notifier,
> > > +					  struct v4l2_subdev *remote_subdev,
> > > +					  struct v4l2_async_subdev *remote_subdev_async)
> > > +{
> > > +	struct v4l2_subdev *subdev = notifier->sd;
> > > +	struct sun6i_mipi_csi2_video *video =
> > > +		sun6i_mipi_csi2_subdev_video(subdev);
> > > +	struct sun6i_mipi_csi2_dev *cdev = sun6i_mipi_csi2_video_dev(video);
> > > +	int source_pad;
> > > +	int ret;
> > > +
> > > +	source_pad = media_entity_get_fwnode_pad(&remote_subdev->entity,
> > > +						 remote_subdev->fwnode,
> > > +						 MEDIA_PAD_FL_SOURCE);
> > > +	if (source_pad < 0)
> > > +		return source_pad;
> > > +
> > > +	ret = media_create_pad_link(&remote_subdev->entity, source_pad,
> > > +				    &subdev->entity, 0,
> > > +				    MEDIA_LNK_FL_ENABLED |
> > > +				    MEDIA_LNK_FL_IMMUTABLE);
> > > +	if (ret) {
> > > +		dev_err(cdev->dev, "failed to create %s:%u -> %s:%u link\n",
> > > +			remote_subdev->entity.name, source_pad,
> > > +			subdev->entity.name, 0);
> > > +		return ret;
> > > +	}
> > > +
> > > +	video->remote_subdev = remote_subdev;
> > > +	video->remote_pad_index = source_pad;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct v4l2_async_notifier_operations sun6i_mipi_csi2_notifier_ops = {
> > > +	.bound		= sun6i_mipi_csi2_notifier_bound,
> > > +};
> > > +
> > > +/* Media Entity */
> > > +
> > > +static int sun6i_mipi_csi2_link_validate(struct media_link *link)
> > > +{
> > > +	struct v4l2_subdev *subdev =
> > > +		container_of(link->sink->entity, struct v4l2_subdev, entity);
> > > +	struct sun6i_mipi_csi2_video *video =
> > > +		sun6i_mipi_csi2_subdev_video(subdev);
> > > +	struct v4l2_subdev *remote_subdev;
> > > +	struct v4l2_subdev_format format = { 0 };
> > > +	int ret;
> > > +
> > > +	if (!is_media_entity_v4l2_subdev(link->source->entity))
> > > +		return -EINVAL;
> > > +
> > > +	remote_subdev = media_entity_to_v4l2_subdev(link->source->entity);
> > > +
> > > +	format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +	format.pad = link->source->index;
> > > +
> > > +	ret = v4l2_subdev_call(remote_subdev, pad, get_fmt, NULL, &format);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	video->mbus_code = format.format.code;
> > > +
> > > +	return 0;
> > > +}
> > 
> > I'm not really sure what you're trying to validate here?
> 
> The whole purpose is to retreive video->mbus_code from the subdev, like it's
> done in the sun6i-csi driver. Maybe there is a more appropriate op to do it?

I'm not sure why you need to do that in the link_validate though?

You just need to init the pad format, and then you'll have a
get_fmt/set_fmt for your pads.

> > > +	cdev->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "bus", io_base,
> > > +						 &sun6i_mipi_csi2_regmap_config);
> > > +	if (IS_ERR(cdev->regmap)) {
> > > +		dev_err(&pdev->dev, "failed to init register map\n");
> > > +		return PTR_ERR(cdev->regmap);
> > > +	}
> > 
> > Yeah, so that won't work. regmap expects to have access to those
> > registers when you enable that clock, but that won't happen since the
> > reset line can be disabled. You would be better off using runtime_pm
> > here.
> 
> I don't understand what you mean here or what the problem could be.
> Here we're just initializing regmap and while this is done before the
> registers are available for I/O, I don't see why it would cause any
> issue at this point.

The regmap here is supposed to take care of the resources, except it
only does it for some of the resources here, which kind of breaks the
expectations. And it doesn't allow you to have the reset / clock
sequence properly done.

> The exact same thing is done in the CSI driver.

That's not an argument though, is it? :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

  reply index

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-23 17:45 [PATCH 00/14] Allwinner MIPI CSI-2 support for A31/V3s/A83T Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 01/14] phy: Distinguish between Rx and Tx for MIPI D-PHY with submodes Paul Kocialkowski
2020-10-23 18:18   ` [linux-sunxi] " Jernej Škrabec
2020-10-24  8:31     ` Paul Kocialkowski
2020-10-30 22:44   ` Helen Koike
2020-10-23 17:45 ` [PATCH 02/14] phy: allwinner: phy-sun6i-mipi-dphy: Support D-PHY Rx mode for MIPI CSI-2 Paul Kocialkowski
2020-10-26 15:38   ` Maxime Ripard
2020-10-27  9:23     ` Paul Kocialkowski
2020-10-27 18:28       ` Maxime Ripard
2020-11-04 10:53         ` Paul Kocialkowski
2020-10-30 22:44   ` Helen Koike
2020-11-04 10:54     ` Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 03/14] media: sun6i-csi: Support an optional dedicated memory pool Paul Kocialkowski
2020-10-26 15:41   ` Maxime Ripard
2020-10-27  9:26     ` Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 04/14] media: sun6i-csi: Fix the image storage bpp for 10/12-bit Bayer formats Paul Kocialkowski
2020-10-30 22:45   ` Helen Koike
2020-11-04 10:56     ` Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 05/14] media: sun6i-csi: Only configure the interface data width for parallel Paul Kocialkowski
2020-10-26 16:00   ` Maxime Ripard
2020-10-27  9:31     ` Paul Kocialkowski
2020-10-27 18:31       ` Maxime Ripard
2020-10-23 17:45 ` [PATCH 06/14] media: sun6i-csi: Support feeding from the MIPI CSI-2 controller Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 07/14] dt-bindings: media: i2c: Add A31 MIPI CSI-2 bindings documentation Paul Kocialkowski
2020-10-26 16:14   ` Maxime Ripard
2020-10-27  9:52     ` Paul Kocialkowski
2020-10-27 18:44       ` Maxime Ripard
2020-11-04 10:48         ` Paul Kocialkowski
2020-11-04 16:53           ` Maxime Ripard
2020-10-30 16:33   ` Rob Herring
2020-10-30 16:56   ` Sakari Ailus
2020-10-23 17:45 ` [PATCH 08/14] media: sunxi: Add support for the A31 MIPI CSI-2 controller Paul Kocialkowski
2020-10-26  8:39   ` Dan Carpenter
2020-10-26 16:54   ` Maxime Ripard
2020-11-04 11:34     ` Paul Kocialkowski
2020-11-04 18:56       ` Maxime Ripard [this message]
2020-11-05 14:52         ` Paul Kocialkowski
2020-10-30 22:45   ` Helen Koike
2020-11-02  9:21     ` Maxime Ripard
2020-11-04 11:17       ` Paul Kocialkowski
2020-11-04 16:38         ` Helen Koike
2020-11-04 18:45           ` Maxime Ripard
2020-11-05 14:14             ` Helen Koike
2020-11-05  8:45   ` Sakari Ailus
2020-11-05 14:55     ` Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 09/14] ARM: dts: sun8i: v3s: Add CSI0 camera interface node Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 10/14] ARM: dts: sun8i: v3s: Add MIPI D-PHY and MIPI CSI-2 interface nodes Paul Kocialkowski
2020-10-26 16:55   ` Maxime Ripard
2020-10-23 17:45 ` [PATCH 11/14] dt-bindings: media: i2c: Add A83T MIPI CSI-2 bindings documentation Paul Kocialkowski
2020-10-26 16:56   ` Maxime Ripard
2020-11-04 10:33     ` Paul Kocialkowski
2020-11-05  8:48   ` Sakari Ailus
2020-10-23 17:45 ` [PATCH 12/14] media: sunxi: Add support for the A83T MIPI CSI-2 controller Paul Kocialkowski
2020-10-26  8:53   ` Dan Carpenter
2020-10-26 17:00   ` Maxime Ripard
2020-11-04 10:37     ` Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 13/14] ARM: dts: sun8i: a83t: Add MIPI CSI-2 controller node Paul Kocialkowski
2020-10-23 17:45 ` [PATCH 14/14] media: sunxi: sun8i-a83t-mipi-csi2: Avoid using the (unsolicited) interrupt Paul Kocialkowski
2020-10-26 16:57   ` Maxime Ripard
2020-10-26 17:20 ` [PATCH 00/14] Allwinner MIPI CSI-2 support for A31/V3s/A83T Maxime Ripard
2020-10-30 22:44 ` Helen Koike
2020-11-02  9:17   ` Maxime Ripard
2020-11-04 11:11   ` Paul Kocialkowski
2020-11-04 11:14     ` Paul Kocialkowski
2020-11-04 16:36     ` Helen Koike
2020-11-05 14:58       ` Paul Kocialkowski

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=20201104185650.ii7dlekjtfar2xpp@gilmour.lan \
    --to=maxime@cerno.tech \
    --cc=devel@driverdev.osuosl.org \
    --cc=devicetree@vger.kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hans.verkuil@cisco.com \
    --cc=helen.koike@collabora.com \
    --cc=hverkuil@xs4all.nl \
    --cc=kevin.lhopital@hotmail.com \
    --cc=kishon@ti.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=mchehab@kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=paul.kocialkowski@bootlin.com \
    --cc=robh+dt@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=vkoul@kernel.org \
    --cc=wens@csie.org \
    --cc=yong.deng@magewell.com \
    /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: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git